GNOME Bugzilla – Bug 745971
gdbus-tool: Add a command to wait for a well-known name on the bus
Last modified: 2017-03-28 11:16:31 UTC
Patch attached.
Created attachment 299006 [details] [review] gdbus-tool: Add a command to wait for a well-known name on the bus This is effectively the mc-wait-for-name tool from telepathy-mission-control; moving it in to gdbus-tool will make it more widely useful without making people depend on telepathy-mission-control for no other reason. The code here is reimplemented from scratch to use GDBus. It blocks until the specified well-known name is owned by some process on the bus (which can be the session, system, or any other bus). By passing --activate, the same (or a different) name can be auto-started on the bus first. There is a 5 minute timeout to ensure the process doesn’t block forever.
Review of attachment 299006 [details] [review]: Looks useful! A few drive-by comments: 1) 5 minutes seems excessive, but maybe thats the point ? 2) Is there really a usecase for activating A when waiting for B ? 3) It would be convenient to allow: gdbus wait --activate NAME as a shorthand for the stuttery gdbus wait --activate NAME NAME
(In reply to Matthias Clasen from comment #2) > Review of attachment 299006 [details] [review] [review]: > > Looks useful! > > A few drive-by comments: > > 1) 5 minutes seems excessive, but maybe thats the point ? Yeah, it’s a case which is never meant to be hit. I’m intending to use this tool in a few automated tests which run on buildbots, so some kind of timeout is needed so the tests can’t get wedged. I’ve made the timeout configurable, and it can be disabled by setting it to 0; but the default can remain at 5 minutes. > 2) Is there really a usecase for activating A when waiting for B ? The original mc-wait-for-name tool allowed this because the service you were waiting for might not actually be auto-activatable. For example, org.freedesktop.Telepathy.AccountManager was not — you had to start org.freedesktop.Telepathy.MissionControl to get it. I figure the functionality is useful enough to keep, especially with suggestion #3. > 3) It would be convenient to allow: > > gdbus wait --activate NAME > > as a shorthand for the stuttery > > gdbus wait --activate NAME NAME Definitely. Done.
Created attachment 299090 [details] [review] gdbus-tool: Add a command to wait for a well-known name on the bus This is effectively the mc-wait-for-name tool from telepathy-mission-control; moving it in to gdbus-tool will make it more widely useful without making people depend on telepathy-mission-control for no other reason. The code here is reimplemented from scratch to use GDBus. It blocks until the specified well-known name is owned by some process on the bus (which can be the session, system, or any other bus). By passing --activate, the same (or a different) name can be auto-started on the bus first. There is a 5 minute timeout to ensure the process doesn’t block forever.
(In reply to Philip Withnall from comment #3) > The original mc-wait-for-name tool allowed this because the service you were > waiting for might not actually be auto-activatable. For example, > org.freedesktop.Telepathy.AccountManager was not — you had to start > org.freedesktop.Telepathy.MissionControl to get it. In fact AccountManager is also activatable, but when we made the AM and MC names activatable and made them both run the MC binary, it was possible to get two copies of the MC binary racing to take the first required name first, among other silly situations. So one of the names now has mc-wait-for-name in its service file instead of the real MC: it service-activates the other name (which makes dbus-daemon start the real MC), then waits for its own name to be taken, then exits. See: http://cgit.freedesktop.org/telepathy/telepathy-mission-control/tree/util/mc-wait-for-name.1.in http://cgit.freedesktop.org/telepathy/telepathy-mission-control/tree/util/wait-for-name.c http://cgit.freedesktop.org/telepathy/telepathy-mission-control/commit?id=ad4b94ecd7768bd952cdcba51fd7039f564e6765 (systemd user services fix this, because systemd understands that one service can implement several D-Bus names; but they also introduce their own problems, e.g. <https://bugs.freedesktop.org/show_bug.cgi?id=89501>, so I don't think we should be recommending user services as the canonical way to do D-Bus activation until issues like that have been fixed.)
Review of attachment 299090 [details] [review]: ::: gio/gdbus-tool.c @@ +2004,3 @@ + { + g_printerr (_("Error: %s is not a valid well-known bus name.\n"), + opt_wait_activate); I'd normally expect a command-line tool to put its own name in error messages like this? @@ +2010,3 @@ + if (opt_wait_activate == NULL && *argc >= 2) + { + wait_service = (*argv)[1]; This logic doesn't seem right. Shouldn't we be using (*argv)[1] (which can also be spelled argv + 1) whenever *argc >= 2, even if opt_wait_activate is non-NULL? Also, if *argc > 2, shouldn't that be an error? At the moment I think "gdbus wait com.example.Foo this-is-ignored" would ignore its last argument entirely?
Created attachment 299199 [details] [review] gdbus-tool: Add a command to wait for a well-known name on the bus This is effectively the mc-wait-for-name tool from telepathy-mission-control; moving it in to gdbus-tool will make it more widely useful without making people depend on telepathy-mission-control for no other reason. The code here is reimplemented from scratch to use GDBus. It blocks until the specified well-known name is owned by some process on the bus (which can be the session, system, or any other bus). By passing --activate, the same (or a different) name can be auto-started on the bus first. There is a 5 minute timeout to ensure the process doesn’t block forever.
Review of attachment 299199 [details] [review]: ::: gio/gdbus-tool.c @@ +1899,3 @@ +static const GOptionEntry wait_entries[] = +{ + { "activate", 'a', 0, G_OPTION_ARG_STRING, &opt_wait_activate, I would expect G_OPTION_FLAG_OPTIONAL_ARG here, to make e.g. the following work: gdbus wait --activate --session org.foo.bar The way you have set things up, this would have to be specified as gdbus wait --session --activate org.foo.bar I prefer options to be not order-dependent like that.
Created attachment 299273 [details] [review] gdbus-tool: Add a command to wait for a well-known name on the bus This is effectively the mc-wait-for-name tool from telepathy-mission-control; moving it in to gdbus-tool will make it more widely useful without making people depend on telepathy-mission-control for no other reason. The code here is reimplemented from scratch to use GDBus. It blocks until the specified well-known name is owned by some process on the bus (which can be the session, system, or any other bus). By passing --activate, the same (or a different) name can be auto-started on the bus first. There is a 5 minute timeout to ensure the process doesn’t block forever.
(In reply to Simon McVittie from comment #6) > Review of attachment 299090 [details] [review] [review]: > > ::: gio/gdbus-tool.c > @@ +2004,3 @@ > + { > + g_printerr (_("Error: %s is not a valid well-known bus name.\n"), > + opt_wait_activate); > > I'd normally expect a command-line tool to put its own name in error > messages like this? I forgot to reply to this. I don’t have an opinion either way, but the patch is consistent with the rest of the g_printerr() calls in gdbus-tool.c. If we’re going to change them, it should be a separate change.
+1 for adding the 'wait' verb - in fact, I just put this patch in ChromeOS' glib package since we needed something like this to fix flakiness with some of our integration tests where we restart Upstart jobs: https://chromium-review.googlesource.com/#/c/262610
Review of attachment 299273 [details] [review]: Looks good to me, FWIW, unless desrt wants to veto it for some reason. I'm never sure to what extent I get to set accepted-commit_now, but it's probably at least the case that (smcv + davidz) >= 1 maintainer :-) (In reply to Philip Withnall from comment #10) > I don’t have an opinion either way, but the patch is consistent with the > rest of the g_printerr() calls in gdbus-tool.c. Sure. I think these are a bug (I know we have a bug for this in Debian, and I think I might have forwarded it already) but this patch isn't a regression in this respect.
Review of attachment 299273 [details] [review]: I like the idea of this (and it seems that there is a fair bit of demand for it) but I think we could get a nicer interface with a bit more work. ::: gio/gdbus-tool.c @@ +1919,3 @@ + { "timeout", 't', 0, G_OPTION_ARG_INT64, &opt_wait_timeout, + N_("Timeout to wait for before exiting with an error (seconds); 0 for " + "no timeout, default is 5 minutes"), "SECS" }, Default seems questionable. I'd expect that we wait forever by default. That would make it consistent with monitoring (which runs forever by default). @@ +2016,3 @@ + /* + * Try and disentangle the command line arguments, with the aim of supporting: + * gdbus wait --session --activate ActivateName WaitName This syntax is not very much like existing gdbus syntax. It's also not quite the same as the syntax that Lars and I had in mind for a total revamp of this tool. Lars wants to get rid of --all --the --flags to things like 'call' and 'introspect' It might make sense to think a bit more about that. Just to write down my thoughts on the matter: (where bus name is required) system:a.b.c (a.b.c on system bus) session:a.b.c (a.b.c on session bus) (where bus name and path is required) session:a.b.c (a.b.c, path /a/b/c implied by name) session:a.b.c/ (a.b.c, path /) session:a.b.c/other (a.b.c, path /other) So then we could do things like: gdbus call session:org.freedesktop.DBus ListNames without all of the --session --dest --object-path --method stuff If we're going to do that anyway, then what you're doing here may as well be consistent with it. We could have: gdbus wait session:a.b.c or gdbus wait --activate session:a.b.c One small functionality nag: you seem to cover the possibility of activating one name and waiting for another, but only on the same bus. What about waiting for a name on a different bus? Also: maybe we could just use GOptionContext here. This cascade of 'else if' is not very nice to read, and I'd be willing to bet there is an error in there somewhere. @@ +2114,3 @@ + } + + g_main_loop_run (loop); use instead: while (running) g_main_context_iteration (NULL, TRUE); and set running = FALSE when you want it to quit. That can be done via global variable or via the user_data (as a gboolean *).
(In reply to Simon McVittie from comment #12) > (In reply to Philip Withnall from comment #10) > > I don’t have an opinion either way, but the patch is consistent with the > > rest of the g_printerr() calls in gdbus-tool.c. > > Sure. I think these are a bug (I know we have a bug for this in Debian, and > I think I might have forwarded it already) but this patch isn't a regression > in this respect. Bug #734576, FWIW
(In reply to Ryan Lortie (desrt) from comment #13) > + { "timeout", 't', 0, G_OPTION_ARG_INT64, &opt_wait_timeout, > + N_("Timeout to wait for before exiting with an error (seconds); 0 for " > + "no timeout, default is 5 minutes"), "SECS" }, > > Default seems questionable. I'd expect that we wait forever by default. > That would make it consistent with monitoring (which runs forever by > default). Is it ever useful to wait indefinitely for a bus name to appear? It isn't useful for activation (both the use-cases so far for mc-wait-for-name have been tied up with activation). > + * gdbus wait --session --activate ActivateName WaitName > > This syntax is not very much like existing gdbus syntax. It's based on mc-wait-for-name, which aimed to follow the rule that positional arguments are required, and --named --arguments are optional (unlike "gdbus call" where many of the --named --arguments are effectively required). In mc-wait-for-name, WaitName was always mandatory even if it was the same as ActivateName, iirc. > It's also not quite the same as the syntax that Lars and I had in mind for a > total revamp of this tool. Lars wants to get rid of --all --the --flags to > things like 'call' and 'introspect' That's reasonable, but does it have to block the addition of functionality that several projects want to rely on? > Just to write down my thoughts on the matter: > > (where bus name is required) > > system:a.b.c (a.b.c on system bus) > session:a.b.c (a.b.c on session bus) How do you do --address in this world, bearing in mind that D-Bus addresses always contain ":"? I would prefer to do this as --session (default), --system, --address=ADDR - it's optional, so it's an option. > (where bus name and path is required) > > session:a.b.c (a.b.c, path /a/b/c implied by name) I see what you're getting at with this, but I don't know how wise it is to perpetuate the (untrue!) meme that D-Bus bus name, object-path and interface names are essentially interchangeable. I would very much prefer to at least make it explicit that some default object path is being used: a.b.c/... or something. "gdbus introspect a.b.c" has a reasonable meaning which does not match this, IMO: introspect recursively from "/". > session:a.b.c/ (a.b.c, path /) > session:a.b.c/other (a.b.c, path /other) a.b.c/whatever is non-standard, but is at least unambiguous. > So then we could do things like: > > gdbus call session:org.freedesktop.DBus ListNames > > without all of the --session --dest --object-path --method stuff I do agree that the destination and object path to "call" would make sense as positional parameters. I don't like "session:" though. There's an order that makes sense for positional parameters, from "largest" to "smallest": bus (but in practice I think that should have a default and hence be optional), destination, object path, interface, method. > One small functionality nag: you seem to cover the possibility of activating > one name and waiting for another, but only on the same bus. What about > waiting for a name on a different bus? The simple answer is that so far, we've never needed this :-) --activate is fundamentally just a shortcut for a suitable "gdbus call", so the worst-case without this feature is that you execute gdbus twice: once to activate the activatable process, and once to wait for the other name that will turn up as a side-effect. > + g_main_loop_run (loop); > > use instead: > > while (running) > g_main_context_iteration (NULL, TRUE); Is this a general effort to deprecate GMainLoop (the object, not the entire GMainContext API) in this sort of thing? I've been avoiding it in regression tests for a while, because I found that being explicit about the exit condition made the loops clearer when it was a composite condition; but my impression had been that this was a weird smcv thing, rather than GLib policy.
(In reply to Ryan Lortie (desrt) from comment #13) > ::: gio/gdbus-tool.c > @@ +1919,3 @@ > + { "timeout", 't', 0, G_OPTION_ARG_INT64, &opt_wait_timeout, > + N_("Timeout to wait for before exiting with an error (seconds); 0 for " > + "no timeout, default is 5 minutes"), "SECS" }, > > Default seems questionable. I'd expect that we wait forever by default. > That would make it consistent with monitoring (which runs forever by > default). More concretely, an infinite wait by default would be consistent with monitoring, but inconsistent with method calls, "udevadm settle", etc. Do you think waiting is more like monitoring than it is like a call? To me, "wait" implies "sooner or later, this should happen", with the failure case where we give up being exceptional.
(In reply to Ryan Lortie (desrt) from comment #13) > ::: gio/gdbus-tool.c > @@ +1919,3 @@ > + { "timeout", 't', 0, G_OPTION_ARG_INT64, &opt_wait_timeout, > + N_("Timeout to wait for before exiting with an error (seconds); 0 for " > + "no timeout, default is 5 minutes"), "SECS" }, > > Default seems questionable. I'd expect that we wait forever by default. > That would make it consistent with monitoring (which runs forever by > default). Sure. Will do so in the next iteration, once the points below are clarified. > @@ +2016,3 @@ > + /* > + * Try and disentangle the command line arguments, with the aim of > supporting: > + * gdbus wait --session --activate ActivateName WaitName > > This syntax is not very much like existing gdbus syntax. I think it only differs in that the WaitName doesn’t take a --double-dash-argument first? > It's also not quite the same as the syntax that Lars and I had in mind for a > total revamp of this tool. Lars wants to get rid of --all --the --flags to > things like 'call' and 'introspect' I agree, that sounds like a reasonable improvement to make to gdbus-tool, and I like your proposal below. However, this patch is consistent with the current implementation, and I don’t really want to block on rewriting the command line handling for gdbus-tool first. > One small functionality nag: you seem to cover the possibility of activating > one name and waiting for another, but only on the same bus. What about > waiting for a name on a different bus? I think there’s less call for that. See comment #5 for the motivation behind --activate. I don’t know of any situations where a daemon would reasonably listen on both the system and session bus. > Also: maybe we could just use GOptionContext here. This cascade of 'else > if' is not very nice to read, and I'd be willing to bet there is an error in > there somewhere. Could do, though I’m willing to bet there is not a bug in there because I’ve worked through the truth table for the cascade. > @@ +2114,3 @@ > + } > + > + g_main_loop_run (loop); > > use instead: > > while (running) > g_main_context_iteration (NULL, TRUE); > > and set running = FALSE when you want it to quit. That can be done via > global variable or via the user_data (as a gboolean *). OOI, how does that differ from the current implementation, where the GMainLoop is passed to the callbacks and g_main_loop_quit() called on it?
(In reply to Ryan Lortie (desrt) from comment #13) > > system:a.b.c (a.b.c on system bus) > session:a.b.c (a.b.c on session bus) > Can you open a different bug for this proposal ? I'll comment on it there...
(In reply to Simon McVittie from comment #15) > How do you do --address in this world, bearing in mind that D-Bus addresses > always contain ":"? > > I would prefer to do this as --session (default), --system, --address=ADDR - > it's optional, so it's an option. I agree with this, minus one point: some day, the obvious default isn't going to be "session", but "user". > I see what you're getting at with this, but I don't know how wise it is to > perpetuate the (untrue!) meme that D-Bus bus name, object-path and interface > names are essentially interchangeable. I don't think that it's true that these are interchangeable, but it's certainly the case that 95% of the time, the "main" object path on a service is equal to the bus name, and there are good reasons for that. Note: nothing said about interfaces here. > "gdbus introspect a.b.c" has a reasonable meaning which does not match this, > IMO: introspect recursively from "/". Maybe, if recursive is a thing. Otherwise, I expect it would be much more useful that this introspected the main object on the service than to just return node / { node org { }; }; (which is what it would do if we default to / without recursion). > I do agree that the destination and object path to "call" would make sense > as positional parameters. I don't like "session:" though. Ya. I guess I can agree with this. Maybe we should keep --session/--system mandatory for now and decide if we want to make --user the implicit default in the future. > --activate is fundamentally just a shortcut for a suitable "gdbus call", so > the worst-case without this feature is that you execute gdbus twice: once to > activate the activatable process, and once to wait for the other name that > will turn up as a side-effect. For what it's worth, I'd like to get rid of the "two names" mode entirely. If we want to activate a separate name, we ought to do that with a separate invocation of gdbus. --activate can be kept, but only for the same name.> I've been avoiding it in regression tests for a while, because I found that > being explicit about the exit condition made the loops clearer when it was a > composite condition; but my impression had been that this was a weird smcv > thing, rather than GLib policy. I've been advocating it for a while for exactly this reason. It's also a bit less racy if there is a case under which g_main_loop_quit() may be called before g_main_loop_run() [in which case the loop will never quit]. We've had quite a lot of these bugs in our tests, which is part of what drove me toward trying to eliminate use of GMainLoop. So ya... in general, I don't think GMainLoop is very useful. (In reply to Philip Withnall from comment #17) > Sure. Will do so in the next iteration, once the points below are clarified. In short: axe the separate-activation-name thing, and otherwise I think everything can more or less stay as it is now (as per the replies to Simon's points above).
hrm, so we're not opening a separate bug, but instead munge discussion of gdbus-entirely-new-UI into this one ?
(In reply to Ryan Lortie (desrt) from comment #19) > I agree with this, minus one point: some day, the obvious default isn't > going to be "session", but "user". I can see that G_BUS_TYPE_MACHINE vs. G_BUS_TYPE_SYSTEM, and G_BUS_TYPE_USER vs. G_BUS_TYPE_SESSION, will matter in a kdbus world: the former aren't guaranteed to respect /etc/dbus-1/s*.d access-control policy, and you can't rely on being able to call o.fd.DBus APIs on them. I'm not sure how relevant that distinction is for a command-line tool though: the former point doesn't matter (because the command-line tool doesn't export APIs of its own), leaving only the latter. Many o.fd.DBus APIs are not useful to something like gdbus or dbus-send, particularly for the sorts of scripted/noninteractive uses where incompatibility is an issue, and those that *are* useful would probably be better done as a new verb (like "gdbus list"). So --user and --machine would probably just be aliases for --session and --system anyway? > For what it's worth, I'd like to get rid of the "two names" mode entirely. > If we want to activate a separate name, we ought to do that with a separate > invocation of gdbus. Maybe... although I should note that the practical use cases we've had for this tool have been for D-Bus activation, which only takes one command-line, and execs it directly rather than invoking a shell; so you'd have to do something like replacing this: Exec=/usr/bin/mc-wait-for-name --activate org.freedesktop.Telepathy.AccountManager org.freedesktop.Telepathy.MissionControl5 with a shell one-liner: Exec=/bin/sh -c "gdbus call --session --dest org.freedesktop.Telepathy.AccountManager --object-path / --method org.freedesktop.DBus.Peer.Ping && gdbus --session wait org.freedesktop.Telepathy.MissionControl5" as opposed to the more direct replacement that Philip's patch would enable: Exec=/usr/bin/gdbus wait --activate org.freedesktop.Telepathy.AccountManager org.freedesktop.Telepathy.MissionControl5 systemd doesn't have that problem because (a) it can have an ExecStartPre, and (b) it understands the concept that a service can be an alias for another service (which isn't considered to be ready until the "other service" is). In systemd-activation, dbus-org.freedesktop.Telepathy.MissionControl5.service would just be a symlink to dbus-org.freedesktop.Telepathy.AccountManager.service. I realise this undermines my justification for not being able to activate on one bus and wait on another, so let me try that again: > > One small functionality nag: you seem to cover the possibility of activating > > one name and waiting for another, but only on the same bus. What about > > waiting for a name on a different bus? The major use cases for this stuff are the well-known buses, of which there are 2½: session or user, accessibility (that's the ½), and system or machine. --activate is mostly there as a way to encode "to get A, activate B, and A will appear as a side-effect" without having to teach dbus-daemon to be a more general service manager like systemd. To avoid layering violations, "larger" buses should never inject services into "smaller" buses: for instance, it would be inappropriate for activation of com.example.SystemFoo on the system bus to result in a connection to "the user's" bus as com.example.SessionFoo, because then you have to answer the question "which user?". So a session .service for SessionFoo can't usefully be implemented in terms of a hypothetical "--system-activate ...SystemFoo", because for that to work, the implementation of SystemFoo would have to connect to someone's session bus. Similarly, a system .service for SystemFoo can't usefully be implemented in terms of "--session-activate ...SessionFoo", because while it would be perfectly OK for the implementation of SessionFoo to connect to the system bus and (try to) claim the SystemFoo name, that doesn't answer the question: whose session do you activate it in?
Created attachment 302061 [details] [review] gdbus-tool: Add a command to wait for a well-known name on the bus This is effectively the mc-wait-for-name tool from telepathy-mission-control; moving it in to gdbus-tool will make it more widely useful without making people depend on telepathy-mission-control for no other reason. The code here is reimplemented from scratch to use GDBus. It blocks until the specified well-known name is owned by some process on the bus (which can be the session, system, or any other bus). By passing --activate, the same (or a different) name can be auto-started on the bus first. A timeout can be specified to ensure the process doesn’t block forever.
(In reply to Philip Withnall from comment #22) > Created attachment 302061 [details] [review] [review] > gdbus-tool: Add a command to wait for a well-known name on the bus Changes from the last patch: • Change timeout default to be infinity (justification below) • Remove use of GMainLoop and use g_main_context_iteration() instead Non-changes: • Haven’t changed the command line syntax; that should be discussed in a separate bug • Haven’t changed the separate-name-activation thing, as per Simon’s explanation in comment #21 (In reply to Simon McVittie from comment #16) > (In reply to Ryan Lortie (desrt) from comment #13) > > ::: gio/gdbus-tool.c > > @@ +1919,3 @@ > > + { "timeout", 't', 0, G_OPTION_ARG_INT64, &opt_wait_timeout, > > + N_("Timeout to wait for before exiting with an error (seconds); 0 for " > > + "no timeout, default is 5 minutes"), "SECS" }, > > > > Default seems questionable. I'd expect that we wait forever by default. > > That would make it consistent with monitoring (which runs forever by > > default). > > More concretely, an infinite wait by default would be consistent with > monitoring, but inconsistent with method calls, "udevadm settle", etc. Do > you think waiting is more like monitoring than it is like a call? > > To me, "wait" implies "sooner or later, this should happen", with the > failure case where we give up being exceptional. I’ve gone with setting the default to infinity, but with the justification that if a script is expecting a timeout, it should always be explicit about the timeout it uses, and not rely on some arbitrary default provided by the tool. Then it knows what it’s getting. So whether `gdbus wait` defaults to timing out or waiting forever doesn’t really matter, because scripts should always be explicit if they want a timeout anyway.
Matthias, got time for a drive-by ack/nack?
Review of attachment 302061 [details] [review]: +1. ::: docs/reference/gio/gdbus.xml @@ +98,3 @@ + <arg choice="plain">--system</arg> + <arg choice="plain">--session</arg> + <arg choice="plain">--address <replaceable>address</replaceable></arg> --timeout is missing here ::: gio/gdbus-tool.c @@ +2132,3 @@ + if (opt_wait_timeout > 0) + { + } better use g_timeout_add() here. this is not what _add_seconds() is for.
Pushed with those fixes. Thanks for the review! Attachment 302061 [details] pushed as 7890573 - gdbus-tool: Add a command to wait for a well-known name on the bus