|
Description
Simon McVittie
2017-05-31 17:50:26 UTC
Plan:
* Extend dbus-daemon's "bus driver" module so an InterfaceHandler can optionally point to an array of named properties, each with a getter function that fills in a variant
* Add an implementation of the o.fd.DBus.Properties interface, with a Get method that looks up a single property from the appropriate InterfaceHandler, a GetAll method that looks up all properties from a single InterfaceHandler, and a Set method that just raises an error
* Add properties to the o.fd.DBus interface
* Features: an array of simple strings, perhaps typically containing ["apparmor", "systemd-activation"] on Apertis
* An array of string interface names, either part of Features or a separate Interfaces property, typically containing ["org.freedesktop.DBus.Stats", "org.freedesktop.DBus.Monitoring"]
* Automated test added to the dbus tests:
* A client program can retrieve the new property or properties with Get()
* A client program can retrieve the new property or properties with GetAll()
* A client program can call Set() and get an error reply
Created attachment 131617 [details] [review] [1/9] asv-util: Expose functions to open an arbitrary entry We'll need this to implement o.fd.DBus.Properties. Created attachment 131618 [details] [review] [2/3] driver: Generate child node elements in introspection This makes the /org/freedesktop/DBus path discoverable. Created attachment 131619 [details] [review] [3/3] driver: Implement Properties and a Features property This still needs documentation and an automated test, but in manual testing with gdbus and d-feet it seems to work. Comment on attachment 131617 [details] [review] [1/9] asv-util: Expose functions to open an arbitrary entry Review of attachment 131617 [details] [review]: ----------------------------------------------------------------- ++, pending your unit tests. Comment on attachment 131618 [details] [review] [2/3] driver: Generate child node elements in introspection Review of attachment 131618 [details] [review]: ----------------------------------------------------------------- ++, pending your unit tests. Created attachment 131669 [details] [review] [2/9] driver: Generate child node elements in introspection This makes the /org/freedesktop/DBus path discoverable. --- Rebased on Bug #101256 Created attachment 131670 [details] [review] [3/9] driver: Implement Properties, with Features and Interfaces properties We recommend using Properties for this sort of thing when designing D-Bus APIs, so it's a bit hypocritical that the reference message bus didn't. The Features and Interfaces properties can be used for feature-discovery as we add new larger features, while the Properties support can be used for finer-grained properties, for example in the interface planned for #100344. --- Changes since Attachment #131619 [details]: * Split Features into Features and Interfaces, making it easier to document * Fix a bug where Get() appended to the wrong message *** Bug 72748 has been marked as a duplicate of this bug. *** Created attachment 131671 [details] [review] [4/9] driver: Implement the Peer interface, for completeness --- See also Bug #72748 which requested this. Created attachment 131672 [details] [review] [5/9] spec: Document the Peer and Properties interfaces for the message bus Created attachment 131673 [details] [review] [6/9] spec: Document the Features and Interfaces properties on o.fd.DBus Created attachment 131674 [details] [review] [7/9] test/dbus-daemon: Fix some memory leaks --- Pre-existing bug, does not affect production code. Created attachment 131675 [details] [review] [8/9] test/dbus-daemon: Exercise the Peer interface Created attachment 131676 [details] [review] [9/9] test/dbus-daemon: Exercise Properties, Features and Interfaces Comment on attachment 131673 [details] [review] [6/9] spec: Document the Features and Interfaces properties on o.fd.DBus Review of attachment 131673 [details] [review]: ----------------------------------------------------------------- ::: doc/dbus-specification.xml @@ +6676,5 @@ > + As a property: > + <programlisting> > + Read-only constant ARRAY of STRING Features > + </programlisting> > + This property lists abstract âfeaturesâ provided by the message These are 66/99 quote marks, but the Splinter extension doesn't render Unicode correctly. Comment on attachment 131673 [details] [review] [6/9] spec: Document the Features and Interfaces properties on o.fd.DBus Review of attachment 131673 [details] [review]: ----------------------------------------------------------------- ::: doc/dbus-specification.xml @@ +6697,5 @@ > + The features currently defined in this specification are as follows: > + <variablelist> > + > + <varlistentry> > + <term><literal>apparmor</literal></term> I'm not sure why I said "apparmor", "selinux" and "systemd_activation" here. Perhaps we should have preferred a property-like case convention with "AppArmor", "SELinux" and "SystemdActivation", so that D-Bus only has one capitalization style? Opinions welcome. Comment on attachment 131669 [details] [review] [2/9] driver: Generate child node elements in introspection Review of attachment 131669 [details] [review]: ----------------------------------------------------------------- ++ (still). Comment on attachment 131670 [details] [review] [3/9] driver: Implement Properties, with Features and Interfaces properties Review of attachment 131670 [details] [review]: ----------------------------------------------------------------- ++ with a couple of nitpicks/questions. ::: bus/driver.c @@ +2500,4 @@ > * Introspectable is also useful at all object paths. */ > INTERFACE_FLAG_ANY_PATH = (1 << 0), > > + /* Set this flag for interfaces that should not show up in Interfaces. */ Nitpick: s/in Interfaces/in the Interfaces property/ to make things a bit clearer? @@ +2529,4 @@ > " <signal name=\"NameAcquired\">\n" > " <arg type=\"s\"/>\n" > " </signal>\n", > + INTERFACE_FLAG_ANY_PATH | INTERFACE_FLAG_UNINTERESTING, ooi, why is this interface not interesting? @@ +2905,5 @@ > + DBUS_TYPE_STRING_AS_STRING, > + &arr_iter)) > + return FALSE; > + > + if (bus_apparmor_enabled ()) Probably could condense all these if-blah-then-append-a-const-string blocks into a loop over an array of { predicate, "feature string" } if you wanted. I’m not blocking the review on this though. Comment on attachment 131671 [details] [review] [4/9] driver: Implement the Peer interface, for completeness Review of attachment 131671 [details] [review]: ----------------------------------------------------------------- ++ with one nitpick. ::: bus/driver.c @@ +2323,5 @@ > + goto oom; > + > + _dbus_assert (dbus_message_has_signature (reply, "s")); > + > + if (! bus_transaction_send_from_driver (transaction, connection, reply)) Nitpick: spurious space after `!`. Comment on attachment 131672 [details] [review] [5/9] spec: Document the Peer and Properties interfaces for the message bus Review of attachment 131672 [details] [review]: ----------------------------------------------------------------- ++ ::: doc/dbus-specification.xml @@ +5243,5 @@ > + In addition to the method calls listed below, the message bus > + should implement the standard Introspectable, Properties and Peer > + interfaces (see <xref linkend="standard-interfaces"/>). > + Support for the Properties and Peer interfaces was added in version > + 1.11.x of the reference implementation of the message bus. Are you going to remember to change ‘1.11.x’ in time for the release? Maybe go with ‘1.11.UNRELEASED’? Comment on attachment 131673 [details] [review] [6/9] spec: Document the Features and Interfaces properties on o.fd.DBus Review of attachment 131673 [details] [review]: ----------------------------------------------------------------- --, some comments. ::: doc/dbus-specification.xml @@ +6679,5 @@ > + </programlisting> > + This property lists abstract âfeaturesâ provided by the message > + bus, and can be used by clients to detect the capabilities > + of the message bus with which they are communicating. > + This property was added in version 1.11.x of the reference Maybe ‘1.11.UNRELEASED’ to make it more greppable before release? @@ +6689,5 @@ > + by this specification. Bus daemon implementors wishing to advertise > + features not mentioned in this document should either contribute > + patches to this specification, or use keys containing â.â and > + starting with their own reversed domain name, for example > + <literal>com.example.subliminal_messages</literal>. Maybe `com.example.MyNewBus.SubliminalMessages` so that the new bus software name is included as the first CamelCase atom? @@ +6697,5 @@ > + The features currently defined in this specification are as follows: > + <variablelist> > + > + <varlistentry> > + <term><literal>apparmor</literal></term> I think I agree, consistent use of net.java.ReverseDns.CamelCase is probably a good idea. @@ +6707,5 @@ > + advertised if AppArmor mediation is enabled and > + active at runtime; merely compiling in support > + for AppArmor should not result in this feature being > + advertised on message bus instances where it is disabled by > + message bus or operating system configuration. Is it worth mentioning whether AppArmor can be turned on/off at runtime? @@ +6720,5 @@ > + This message bus filters messages via the > + <ulink url="https://selinuxproject.org/">SELinux</ulink> > + security framework. Similar to <literal>apparmor</literal>, > + this feature should only be advertised if SELinux mediation > + is enabled and active at runtime. Similarly, this probably should mention what happens when I run `sudo setenforce 0` on my system. @@ +6730,5 @@ > + <term><literal>systemd_activation</literal></term> > + <listitem> > + <para> > + When asked to activate a service that has the > + <literal>SystemdService</literal> field in its .service <filename>.service</filename> @@ +6764,5 @@ > + The standard <literal>org.freedesktop.DBus</literal>, > + <literal>org.freedesktop.DBus.Properties</literal>, > + <literal>org.freedesktop.DBus.Peer</literal> and > + <literal>org.freedesktop.DBus.Introspectable</literal> > + interfaces are not included in the value of this property. Is it worth (briefly) mentioning why not? Comment on attachment 131674 [details] [review] [7/9] test/dbus-daemon: Fix some memory leaks Review of attachment 131674 [details] [review]: ----------------------------------------------------------------- -- ::: test/dbus-daemon.c @@ +371,5 @@ > g_assert_cmpstr (f->e.message, ==, > "Message did not receive a reply (timeout by message bus)"); > + > + dbus_message_unref (reply); > + dbus_pending_call_unref (pc); There’s already a dbus_pending_call_unref() call higher up, and pc doesn’t seem to be set again since then. Are you sure this is right? (I haven’t traced the refcounting further than that, so you might well be right. Or this might be something introduced in an earlier patch in the series? I’m only looking at master atm.) Comment on attachment 131675 [details] [review] [8/9] test/dbus-daemon: Exercise the Peer interface Review of attachment 131675 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 131676 [details] [review] [9/9] test/dbus-daemon: Exercise Properties, Features and Interfaces Review of attachment 131676 [details] [review]: ----------------------------------------------------------------- ++, with one question. ::: test/dbus-daemon.c @@ +1157,5 @@ > +test_get_invalid_iface (Fixture *f, > + gconstpointer context) > +{ > + DBusMessage *m = dbus_message_new_method_call (DBUS_SERVICE_DBUS, > + DBUS_PATH_DBUS, DBUS_INTERFACE_PROPERTIES, "Get"); Is it worth adding a few tests for o.fd.DBus.P.{Get,Set,GetAll} being called with the wrong object path? (In reply to Philip Withnall from comment #20) > Nitpick: s/in Interfaces/in the Interfaces property/ to make things a bit > clearer? Sure > @@ +2529,4 @@ > > " <signal name=\"NameAcquired\">\n" > > " <arg type=\"s\"/>\n" > > " </signal>\n", > > + INTERFACE_FLAG_ANY_PATH | INTERFACE_FLAG_UNINTERESTING, > > ooi, why is this interface not interesting? Because if you can successfully GetAll("o.fd.DBus") or Get("o.fd.DBus", "Interfaces") then it seems obvious that you have the o.fd.DBus interface :-) > Probably could condense all these if-blah-then-append-a-const-string blocks > into a loop over an array of { predicate, "feature string" } if you wanted. > I’m not blocking the review on this though. I think I prefer this way, although I might do as you suggest if we gain a lot of features. (In reply to Philip Withnall from comment #22) > Are you going to remember to change ‘1.11.x’ in time for the release? Maybe > go with ‘1.11.UNRELEASED’? The reason I said 1.11.x is that if I forget to change it, it reads reasonably sensibly anyway :-) (Ideally nobody should be using 1.11.x for longer than the time until 1.11.(x+1) anyway.) (In reply to Philip Withnall from comment #23) > I think I agree, consistent use of net.java.ReverseDns.CamelCase is probably > a good idea. Will do. As a special case I'm intending to skip the namespacing for things defined in dbus-specification, like we did for GetConnectionCredentials() (LinuxSecurityLabel etc.), so you don't have to keep repeating org.freedesktop.DBus. quite so often. > Is it worth mentioning whether AppArmor can be turned on/off at runtime? Maybe. (It can't.) > Similarly, this probably should mention what happens when I run `sudo > setenforce 0` on my system. That puts it in permissive mode, which is analogous to putting all AppArmor profiles in complain mode, right? (Complain about violations but allow them to happen anyway) If so, I think that still counts as mediating things through the LSM. > @@ +6764,5 @@ > > + The standard <literal>org.freedesktop.DBus</literal>, > > + <literal>org.freedesktop.DBus.Properties</literal>, > > + <literal>org.freedesktop.DBus.Peer</literal> and > > + <literal>org.freedesktop.DBus.Introspectable</literal> > > + interfaces are not included in the value of this property. > > Is it worth (briefly) mentioning why not? Yeah, perhaps. (The answer is: because if you can fetch the Properties of the DBus interface then the fact that you have Properties and DBus is already obvious; Peer is a weird pseudo-interface; and Introspectable isn't really a "feature-like interface" either.) Comment on attachment 131674 [details] [review] [7/9] test/dbus-daemon: Fix some memory leaks Review of attachment 131674 [details] [review]: ----------------------------------------------------------------- ::: test/dbus-daemon.c @@ +371,5 @@ > g_assert_cmpstr (f->e.message, ==, > "Message did not receive a reply (timeout by message bus)"); > + > + dbus_message_unref (reply); > + dbus_pending_call_unref (pc); Good catch - it's only reply that's leaked here. (In reply to Philip Withnall from comment #26) > Is it worth adding a few tests for o.fd.DBus.P.{Get,Set,GetAll} being called > with the wrong object path? Yeah, perhaps. Created attachment 131746 [details] [review] [1/10] test/dbus-daemon: Fix some memory leaks --- Don't double-free a pending call as Philip pointed out. Do free leaked queued messages in test_max_replies_per_connection(). Created attachment 131747 [details] [review] [2/10] transport: Don't pile up errors for semicolon-separated components If we somehow get an autolaunch address with multiple semicolon-separated components, and one of them fails, then we will hit an assertion failure when we try the next one. --- I have no idea how to reproduce this specifically, but I saw it happen after terminating my session bus (and hence my GNOME session) with an inadvisable use of pkill, then recovering. Created attachment 131748 [details] [review] [3/10] asv-util: Expose functions to open an arbitrary entry --- Unchanged, was 1/9 Created attachment 131749 [details] [review] [4/10] driver: Generate child node elements in introspection --- Unchanged, was 2/9. Created attachment 131750 [details] [review] [5/10] driver: Implement Properties, with Features and Interfaces properties --- Was 3/9. Now with more comments about why the interfaces have the flags they do, and camel-case feature names. Created attachment 131751 [details] [review] [6/10] driver: Implement the Peer interface, for completeness --- Was 4/9. Adjust comments about why Peer is ANY_PATH and UNINTERESTING. Created attachment 131752 [details] [review] [7/10] spec: Document the Peer and Properties interfaces for the message bus --- Was 5/9. Unchanged, I think. Created attachment 131753 [details] [review] [8/10] spec: Document the Features and Interfaces properties on o.fd.DBus --- Justify why some standard interfaces are omitted from Interfaces. Use camel-case naming convention: AppArmor, SELinux, SystemdActivation, com.example.MyBus.SubliminalMessages. Mention that SELinux mediation in permissive mode is still SELinux mediation. Created attachment 131754 [details] [review] [9/10] test/dbus-daemon: Exercise the Peer interface --- Was 8/9. Unchanged? Created attachment 131755 [details] [review] [10/10] test/dbus-daemon: Exercise Properties, Features and Interfaces --- Expect SystemdActivation in camel-case. Add test cases for otherwise-valid Get, Set, GetAll calls at path "/", which raise UnknownInterface because /org/freedesktop/DBus is the only object with Properties. (In reply to Simon McVittie from comment #29) > (In reply to Philip Withnall from comment #23) > > Is it worth mentioning whether AppArmor can be turned on/off at runtime? > > Maybe. (It can't.) Sorry, I couldn't work out how best to phrase this. Perhaps you can? Comment on attachment 131746 [details] [review] [1/10] test/dbus-daemon: Fix some memory leaks Review of attachment 131746 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 131747 [details] [review] [2/10] transport: Don't pile up errors for semicolon-separated components Review of attachment 131747 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 131750 [details] [review] [5/10] driver: Implement Properties, with Features and Interfaces properties Review of attachment 131750 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 131751 [details] [review] [6/10] driver: Implement the Peer interface, for completeness Review of attachment 131751 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 131753 [details] [review] [8/10] spec: Document the Features and Interfaces properties on o.fd.DBus Review of attachment 131753 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 131755 [details] [review] [10/10] test/dbus-daemon: Exercise Properties, Features and Interfaces Review of attachment 131755 [details] [review]: ----------------------------------------------------------------- ++ (In reply to Simon McVittie from comment #42) > (In reply to Simon McVittie from comment #29) > > (In reply to Philip Withnall from comment #23) > > > Is it worth mentioning whether AppArmor can be turned on/off at runtime? > > > > Maybe. (It can't.) > > Sorry, I couldn't work out how best to phrase this. Perhaps you can? If it can’t be turned on/off at runtime then there’s probably no need to mention it. Similarly for SELinux. I think the docs patch is good as it stands. 👌 (In reply to Simon McVittie from comment #40) > [9/10] test/dbus-daemon: Exercise the Peer interface This failed on Travis-CI because the build worker doesn't have /etc/machine-id. We need to avoid using dbus_get_local_machine_id(), which (depending how dbus was compiled) either crashes out or returns a new random value on such systems. I'm increasingly tempted to expose a replacement API that has a DBusError parameter and doesn't second-guess this stuff, but for now we can use dbus_internal_do_not_use_get_uuid(), which is part of the implementation of the dbus-uuidgen executable (its weird name comes from the time when dbus-uuidgen could only use dbus_-prefixed libdbus APIs). Perhaps this? dbus_bool_t dbus_try_get_local_machine_id (char **uuid_p, DBusError *error); (Equivalent to dbus_internal_do_not_use_get_uuid (NULL, uuid_p, FALSE, error), where the NULL means use the system-wide machine ID files, and the FALSE means don't try to create it if it doesn't already exist. That's basically the implementation of `dbus-uuidgen --get`.) (In reply to Simon McVittie from comment #50) > (In reply to Simon McVittie from comment #40) > > [9/10] test/dbus-daemon: Exercise the Peer interface > > This failed on Travis-CI because the build worker doesn't have > /etc/machine-id. We need to avoid using dbus_get_local_machine_id(), which > (depending how dbus was compiled) either crashes out or returns a new random > value on such systems. > > I'm increasingly tempted to expose a replacement API that has a DBusError > parameter and doesn't second-guess this stuff, but for now we can use > dbus_internal_do_not_use_get_uuid(), which is part of the implementation of > the dbus-uuidgen executable (its weird name comes from the time when > dbus-uuidgen could only use dbus_-prefixed libdbus APIs). Perhaps this? > > dbus_bool_t dbus_try_get_local_machine_id (char **uuid_p, DBusError > *error); > > (Equivalent to dbus_internal_do_not_use_get_uuid (NULL, uuid_p, FALSE, > error), where the NULL means use the system-wide machine ID files, and the > FALSE means don't try to create it if it doesn't already exist. That's > basically the implementation of `dbus-uuidgen --get`.) For the sake of keeping this bug from going on forever, I suggest dbus_try_get_local_machine_id() is discussed and added in a separate bug. Using dbus_internal_do_not_use_get_uuid() from the test/dbus-daemon test seems reasonable as an interim measure. Created attachment 131774 [details] [review] [9/11] Unix sysdeps: Only copy /etc/machine-id to ${sysconfdir} in "ensure" mode System integration scripts use dbus-uuidgen --ensure, so they are unaffected by this, and in particular the solution to Bug #77941 is still valid. The shared library is typically loaded by unprivileged users, so trying to write out the machine-id file is not going to work anyway. However, if we *can* write to our ${sysconfdir} - notably during `make distcheck` - then it is unexpected that merely reading the machine ID has the side-effect of writing to ${sysconfdir}, and in particular it will make the check for a complete uninstall fail. We definitely must not delete the machine ID during `make uninstall`. Created attachment 131775 [details] [review] [10/11] test/dbus-daemon: Exercise the Peer interface We have to skip the GetMachineId() part during build-time testing if it wouldn't work - there is no guarantee that dbus has ever been installed on the build system. However, we can insist on it during installed-tests, if we make sure to complete the installation for the Travis-CI build by running dbus-uuidgen. Created attachment 131776 [details] [review] [11/11] test/dbus-daemon: Exercise Properties, Features and Interfaces --- Essentially unchanged, just some unfuzzing of the diff after the revised 10/11 (In reply to Philip Withnall from comment #51) > For the sake of keeping this bug from going on forever, I suggest > dbus_try_get_local_machine_id() is discussed and added in a separate bug. Sure. > Using dbus_internal_do_not_use_get_uuid() from the test/dbus-daemon test > seems reasonable as an interim measure. In the end I had to use _dbus_read_local_machine_uuid() directly, because we don't compile dbus_internal_do_not_use_get_uuid() on Windows; but other than that, yes. Comment on attachment 131751 [details] [review] [6/10] driver: Implement the Peer interface, for completeness Review of attachment 131751 [details] [review]: ----------------------------------------------------------------- ::: bus/driver.c @@ +2307,5 @@ > + return FALSE; > + } > + > + if (!_dbus_get_local_machine_uuid_encoded (&uuid, error)) > + return FALSE; This leaks the initialized DBusString. Bug #89104 is why we can't have nice things. Comment on attachment 131774 [details] [review] [9/11] Unix sysdeps: Only copy /etc/machine-id to ${sysconfdir} in "ensure" mode Review of attachment 131774 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-unix.c @@ +3920,4 @@ > _dbus_string_init_const (&filename, "/etc/machine-id"); > b = _dbus_read_uuid_file (&filename, machine_id, FALSE, error); > > + if (create_if_not_found && b) This is completely the wrong logic: it effectively means we ignore /etc/machine-id if we are not willing to copy the file. Fixing. Created attachment 131781 [details] [review] [6/11] driver: Implement the Peer interface, for completeness --- Don't leak the empty string 'uuid' if we fail to populate it Created attachment 131782 [details] [review] [09/11] Unix sysdeps: Only copy /etc/machine-id to ${sysconfdir} in "ensure" mode --- Fix logic error previously noted (In reply to Philip Withnall from comment #51) > For the sake of keeping this bug from going on forever, I suggest > dbus_try_get_local_machine_id() is discussed and added in a separate bug. Bug #13194 is very relevant, so I attached patches for that change there. Comment on attachment 131775 [details] [review] [10/11] test/dbus-daemon: Exercise the Peer interface Review of attachment 131775 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 131776 [details] [review] [11/11] test/dbus-daemon: Exercise Properties, Features and Interfaces Review of attachment 131776 [details] [review]: ----------------------------------------------------------------- > Essentially unchanged, just some unfuzzing of the diff after the revised 10/11 ++ on that basis then; I haven’t re-reviewed it. Comment on attachment 131781 [details] [review] [6/11] driver: Implement the Peer interface, for completeness Review of attachment 131781 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 131782 [details] [review] [09/11] Unix sysdeps: Only copy /etc/machine-id to ${sysconfdir} in "ensure" mode Review of attachment 131782 [details] [review]: ----------------------------------------------------------------- ++ for sure. This is probably a change to note in the NEWS file when the time comes to update that, just in case people are (erroneously) relying on that side-effect. Fixed in git for 1.11.14. Thanks for reviewing! |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.