Bug 101257 - Implement o.fd.Properties on bus driver
Summary: Implement o.fd.Properties on bus driver
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other All
: medium enhancement
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL: https://github.com/smcv/dbus/commits/...
Whiteboard: review+
Keywords: patch
: 72748 (view as bug list)
Depends on: 101256
Blocks: 100344 13194
  Show dependency treegraph
 
Reported: 2017-05-31 17:50 UTC by Simon McVittie
Modified: 2017-06-08 16:41 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
[1/9] asv-util: Expose functions to open an arbitrary entry (2.94 KB, patch)
2017-05-31 19:38 UTC, Simon McVittie
Details | Splinter Review
[2/3] driver: Generate child node elements in introspection (3.17 KB, patch)
2017-05-31 19:38 UTC, Simon McVittie
Details | Splinter Review
[3/3] driver: Implement Properties and a Features property (13.46 KB, patch)
2017-05-31 19:38 UTC, Simon McVittie
Details | Splinter Review
[2/9] driver: Generate child node elements in introspection (3.29 KB, patch)
2017-06-02 12:51 UTC, Simon McVittie
Details | Splinter Review
[3/9] driver: Implement Properties, with Features and Interfaces properties (14.48 KB, patch)
2017-06-02 12:52 UTC, Simon McVittie
Details | Splinter Review
[4/9] driver: Implement the Peer interface, for completeness (4.42 KB, patch)
2017-06-02 12:55 UTC, Simon McVittie
Details | Splinter Review
[5/9] spec: Document the Peer and Properties interfaces for the message bus (1.11 KB, patch)
2017-06-02 12:55 UTC, Simon McVittie
Details | Splinter Review
[6/9] spec: Document the Features and Interfaces properties on o.fd.DBus (5.29 KB, patch)
2017-06-02 12:56 UTC, Simon McVittie
Details | Splinter Review
[7/9] test/dbus-daemon: Fix some memory leaks (1.58 KB, patch)
2017-06-02 12:56 UTC, Simon McVittie
Details | Splinter Review
[8/9] test/dbus-daemon: Exercise the Peer interface (4.17 KB, patch)
2017-06-02 12:57 UTC, Simon McVittie
Details | Splinter Review
[9/9] test/dbus-daemon: Exercise Properties, Features and Interfaces (19.35 KB, patch)
2017-06-02 12:57 UTC, Simon McVittie
Details | Splinter Review
[1/10] test/dbus-daemon: Fix some memory leaks (1.75 KB, patch)
2017-06-06 21:56 UTC, Simon McVittie
Details | Splinter Review
[2/10] transport: Don't pile up errors for semicolon-separated components (1.14 KB, patch)
2017-06-06 21:57 UTC, Simon McVittie
Details | Splinter Review
[3/10] asv-util: Expose functions to open an arbitrary entry (3.04 KB, patch)
2017-06-06 21:58 UTC, Simon McVittie
Details | Splinter Review
[4/10] driver: Generate child node elements in introspection (3.30 KB, patch)
2017-06-06 21:58 UTC, Simon McVittie
Details | Splinter Review
[5/10] driver: Implement Properties, with Features and Interfaces properties (15.14 KB, patch)
2017-06-06 21:59 UTC, Simon McVittie
Details | Splinter Review
[6/10] driver: Implement the Peer interface, for completeness (4.58 KB, patch)
2017-06-06 22:01 UTC, Simon McVittie
Details | Splinter Review
[7/10] spec: Document the Peer and Properties interfaces for the message bus (1.12 KB, patch)
2017-06-06 22:01 UTC, Simon McVittie
Details | Splinter Review
[8/10] spec: Document the Features and Interfaces properties on o.fd.DBus (5.83 KB, patch)
2017-06-06 22:02 UTC, Simon McVittie
Details | Splinter Review
[9/10] test/dbus-daemon: Exercise the Peer interface (4.17 KB, patch)
2017-06-06 22:03 UTC, Simon McVittie
Details | Splinter Review
[10/10] test/dbus-daemon: Exercise Properties, Features and Interfaces (24.09 KB, patch)
2017-06-06 22:04 UTC, Simon McVittie
Details | Splinter Review
[9/11] Unix sysdeps: Only copy /etc/machine-id to ${sysconfdir} in "ensure" mode (1.52 KB, patch)
2017-06-07 14:54 UTC, Simon McVittie
Details | Splinter Review
[10/11] test/dbus-daemon: Exercise the Peer interface (7.75 KB, patch)
2017-06-07 14:55 UTC, Simon McVittie
Details | Splinter Review
[11/11] test/dbus-daemon: Exercise Properties, Features and Interfaces (24.13 KB, patch)
2017-06-07 14:56 UTC, Simon McVittie
Details | Splinter Review
[6/11] driver: Implement the Peer interface, for completeness (4.62 KB, patch)
2017-06-07 16:46 UTC, Simon McVittie
Details | Splinter Review
[09/11] Unix sysdeps: Only copy /etc/machine-id to ${sysconfdir} in "ensure" mode (1.86 KB, patch)
2017-06-07 16:47 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2017-05-31 17:50:26 UTC
The dbus-daemon has added more functionality over time. Some software targets various different versions of dbus-daemon, and some functionality is compile-time optional and so cannot be gated by a version check.

The design that I have in mind for Bug #100344 requires the ability to discover which features have been implemented, preferably via the standard Properties interface. This is something that dbus-daemon has been missing for some time. Before doing the rest of Bug #100344, we can add feature-discovery for the features that already exist and use that as a test of the same code.
Comment 1 Simon McVittie 2017-05-31 17:51:23 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
Comment 2 Simon McVittie 2017-05-31 19:38:23 UTC
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.
Comment 3 Simon McVittie 2017-05-31 19:38:40 UTC
Created attachment 131618 [details] [review]
[2/3] driver: Generate child node elements in introspection

This makes the /org/freedesktop/DBus path discoverable.
Comment 4 Simon McVittie 2017-05-31 19:38:56 UTC
Created attachment 131619 [details] [review]
[3/3] driver: Implement Properties and a Features property
Comment 5 Simon McVittie 2017-05-31 19:39:46 UTC
This still needs documentation and an automated test, but in manual testing with gdbus and d-feet it seems to work.
Comment 6 Philip Withnall 2017-06-01 11:09:56 UTC
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 7 Philip Withnall 2017-06-01 11:09:58 UTC
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.
Comment 8 Simon McVittie 2017-06-02 12:51:06 UTC
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
Comment 9 Simon McVittie 2017-06-02 12:52:07 UTC
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
Comment 10 Simon McVittie 2017-06-02 12:54:53 UTC
*** Bug 72748 has been marked as a duplicate of this bug. ***
Comment 11 Simon McVittie 2017-06-02 12:55:12 UTC
Created attachment 131671 [details] [review]
[4/9] driver: Implement the Peer interface, for completeness

---

See also Bug #72748 which requested this.
Comment 12 Simon McVittie 2017-06-02 12:55:45 UTC
Created attachment 131672 [details] [review]
[5/9] spec: Document the Peer and Properties interfaces for the  message bus
Comment 13 Simon McVittie 2017-06-02 12:56:02 UTC
Created attachment 131673 [details] [review]
[6/9] spec: Document the Features and Interfaces properties on  o.fd.DBus
Comment 14 Simon McVittie 2017-06-02 12:56:29 UTC
Created attachment 131674 [details] [review]
[7/9] test/dbus-daemon: Fix some memory leaks

---

Pre-existing bug, does not affect production code.
Comment 15 Simon McVittie 2017-06-02 12:57:20 UTC
Created attachment 131675 [details] [review]
[8/9] test/dbus-daemon: Exercise the Peer interface
Comment 16 Simon McVittie 2017-06-02 12:57:39 UTC
Created attachment 131676 [details] [review]
[9/9] test/dbus-daemon: Exercise Properties, Features and  Interfaces
Comment 17 Simon McVittie 2017-06-02 13:01:18 UTC
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 18 Simon McVittie 2017-06-02 13:17:11 UTC
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 19 Philip Withnall 2017-06-06 11:45:32 UTC
Comment on attachment 131669 [details] [review]
[2/9] driver: Generate child node elements in introspection

Review of attachment 131669 [details] [review]:
-----------------------------------------------------------------

++ (still).
Comment 20 Philip Withnall 2017-06-06 11:55:11 UTC
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 21 Philip Withnall 2017-06-06 11:57:44 UTC
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 22 Philip Withnall 2017-06-06 11:58:31 UTC
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 23 Philip Withnall 2017-06-06 12:04:20 UTC
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 24 Philip Withnall 2017-06-06 12:07:49 UTC
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 25 Philip Withnall 2017-06-06 12:13:18 UTC
Comment on attachment 131675 [details] [review]
[8/9] test/dbus-daemon: Exercise the Peer interface

Review of attachment 131675 [details] [review]:
-----------------------------------------------------------------

++
Comment 26 Philip Withnall 2017-06-06 12:18:32 UTC
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?
Comment 27 Simon McVittie 2017-06-06 14:16:06 UTC
(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.
Comment 28 Simon McVittie 2017-06-06 14:17:29 UTC
(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.)
Comment 29 Simon McVittie 2017-06-06 14:22:08 UTC
(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 30 Simon McVittie 2017-06-06 14:23:58 UTC
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.
Comment 31 Simon McVittie 2017-06-06 14:24:38 UTC
(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.
Comment 32 Simon McVittie 2017-06-06 21:56:28 UTC
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().
Comment 33 Simon McVittie 2017-06-06 21:57:55 UTC
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.
Comment 34 Simon McVittie 2017-06-06 21:58:20 UTC
Created attachment 131748 [details] [review]
[3/10] asv-util: Expose functions to open an arbitrary entry

---

Unchanged, was 1/9
Comment 35 Simon McVittie 2017-06-06 21:58:57 UTC
Created attachment 131749 [details] [review]
[4/10] driver: Generate child node elements in introspection

---

Unchanged, was 2/9.
Comment 36 Simon McVittie 2017-06-06 21:59:53 UTC
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.
Comment 37 Simon McVittie 2017-06-06 22:01:11 UTC
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.
Comment 38 Simon McVittie 2017-06-06 22:01:39 UTC
Created attachment 131752 [details] [review]
[7/10] spec: Document the Peer and Properties interfaces for  the message bus

---

Was 5/9. Unchanged, I think.
Comment 39 Simon McVittie 2017-06-06 22:02:54 UTC
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.
Comment 40 Simon McVittie 2017-06-06 22:03:30 UTC
Created attachment 131754 [details] [review]
[9/10] test/dbus-daemon: Exercise the Peer interface

---
Was 8/9. Unchanged?
Comment 41 Simon McVittie 2017-06-06 22:04:49 UTC
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.
Comment 42 Simon McVittie 2017-06-06 22:06:08 UTC
(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 43 Philip Withnall 2017-06-06 22:40:33 UTC
Comment on attachment 131746 [details] [review]
[1/10] test/dbus-daemon: Fix some memory leaks

Review of attachment 131746 [details] [review]:
-----------------------------------------------------------------

++
Comment 44 Philip Withnall 2017-06-06 22:41:47 UTC
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 45 Philip Withnall 2017-06-06 22:43:15 UTC
Comment on attachment 131750 [details] [review]
[5/10] driver: Implement Properties, with Features and  Interfaces properties

Review of attachment 131750 [details] [review]:
-----------------------------------------------------------------

++
Comment 46 Philip Withnall 2017-06-06 22:43:43 UTC
Comment on attachment 131751 [details] [review]
[6/10] driver: Implement the Peer interface, for completeness

Review of attachment 131751 [details] [review]:
-----------------------------------------------------------------

++
Comment 47 Philip Withnall 2017-06-06 22:45:38 UTC
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 48 Philip Withnall 2017-06-06 22:47:26 UTC
Comment on attachment 131755 [details] [review]
[10/10] test/dbus-daemon: Exercise Properties, Features and  Interfaces

Review of attachment 131755 [details] [review]:
-----------------------------------------------------------------

++
Comment 49 Philip Withnall 2017-06-06 22:48:59 UTC
(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. 👌
Comment 50 Simon McVittie 2017-06-07 11:59:08 UTC
(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`.)
Comment 51 Philip Withnall 2017-06-07 12:25:06 UTC
(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.
Comment 52 Simon McVittie 2017-06-07 14:54:56 UTC
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`.
Comment 53 Simon McVittie 2017-06-07 14:55:27 UTC
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.
Comment 54 Simon McVittie 2017-06-07 14:56:19 UTC
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
Comment 55 Simon McVittie 2017-06-07 14:57:41 UTC
(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 56 Simon McVittie 2017-06-07 15:23:01 UTC
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 57 Simon McVittie 2017-06-07 15:56:03 UTC
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.
Comment 58 Simon McVittie 2017-06-07 16:46:59 UTC
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
Comment 59 Simon McVittie 2017-06-07 16:47:35 UTC
Created attachment 131782 [details] [review]
[09/11] Unix sysdeps: Only copy /etc/machine-id to  ${sysconfdir} in "ensure" mode

---

Fix logic error previously noted
Comment 60 Simon McVittie 2017-06-07 17:22:50 UTC
(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 61 Philip Withnall 2017-06-07 22:43:19 UTC
Comment on attachment 131775 [details] [review]
[10/11] test/dbus-daemon: Exercise the Peer interface

Review of attachment 131775 [details] [review]:
-----------------------------------------------------------------

++
Comment 62 Philip Withnall 2017-06-07 22:43:58 UTC
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 63 Philip Withnall 2017-06-07 22:45:50 UTC
Comment on attachment 131781 [details] [review]
[6/11] driver: Implement the Peer interface, for completeness

Review of attachment 131781 [details] [review]:
-----------------------------------------------------------------

++
Comment 64 Philip Withnall 2017-06-07 22:52:54 UTC
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.
Comment 65 Simon McVittie 2017-06-08 16:41:22 UTC
Fixed in git for 1.11.14. Thanks for reviewing!


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct.