Bug 88994

Created attachment 113193 [details] [review]
doc: WIP work to add an API guidelines document

Also available in this git branch: http://cgit.collabora.com/git/user/pwith/dbus.git/log/?h=api-guidelines

(In reply to Philip Withnall from comment #0)
> This does add `ducktype` and `yelp-build` as hard dependencies of the
> documentation build, just like `xmlto`. I can refactor the build system
> changes to hide them under a separate --enable-ducktype-documentation
> configure flag if desired.

Yes please: I don't think packagers are all going to want to add this dependency. In particular, Ducktype isn't in Debian yet.

I don't mind it being mandatory for DBUS_CAN_UPLOAD_DOCS, but it should be a new thing in parallel with DBUS_DOXYGEN_DOCS_ENABLED and DBUS_XML_DOCS_ENABLED.

Created attachment 113206 [details] [review]
doc: Add a guide to designing D-Bus APIs

Updated version.

This version of the document is complete. There is one FIXME left for the future about mentioning kdbus 

Any integration with the CMake build system still remains to be done.

Created attachment 113207 [details] [review]
doc: Add a guide to designing D-Bus APIs

Updated to fix a few Ducktype escaping issues.

This version of the document is complete. There is one FIXME left for the future about mentioning kdbus 

Any integration with the CMake build system still remains to be done.

Comment on attachment 113207 [details] [review]
doc: Add a guide to designing D-Bus APIs

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

::: configure.ac
@@ +150,4 @@
>  AC_ARG_ENABLE(checks, AS_HELP_STRING([--enable-checks],[include sanity checks on public API]),enable_checks=$enableval,enable_checks=yes)
>  AC_ARG_ENABLE(xml-docs, AS_HELP_STRING([--enable-xml-docs],[build XML documentation (requires xmlto)]),enable_xml_docs=$enableval,enable_xml_docs=auto)
>  AC_ARG_ENABLE(doxygen-docs, AS_HELP_STRING([--enable-doxygen-docs],[build DOXYGEN documentation (requires Doxygen)]),enable_doxygen_docs=$enableval,enable_doxygen_docs=auto)
> +AC_ARG_ENABLE(ducktype-docs, AS_HELP_STRING([--enable-ducktype-docs],[build Ducktype documentation (requires Ducktype)]),enable_ducktype_docs=$enableval,enable_ducktype_docs=auto)

This is underquoted: it should have one level of [] per level of () (none of the arguments to AC_ARG_ENABLE are quoted).

(I'd be happy to review patches to fix underquoting elsewhere, but it's sufficient that new lines aren't underquoted.)

I'd prefer consistently having spaces after commas, with any reasonable line-breaking point of your choice.

@@ +1432,5 @@
> +AS_IF([test "$DUCKTYPE" = "no"],[have_ducktype=no],[have_ducktype=yes])
> +AS_IF([test "$YELP_BUILD" = "no"],[have_yelp_build=no],[have_yelp_build=yes])
> +
> +AS_IF([test "$enable_ducktype_docs" = "auto"],[
> +    AS_IF([test "$have_ducktype" = "no" -o "$have_yelp_build" = "no"],[

test EXPR || test EXPR, please. "test EXPR -o EXPR" is a GNUism.

::: doc/dbus-api-design.duck
@@ +24,5 @@
> +  $link[>>https://developer.gnome.org/gio/stable/gdbus-convenience.html](GDBus),
> +  but detailed implementation instructions are left to the librariesâ
> +  documentation. Note that you should $em(not) use libdbus-1 or libdbus-glib to
> +  implement D-Bus services, as they are awkward to use correctly, deprecated
> +  and unmaintained.

It seems kinda bad to be saying libdbus is unmaintained in a patch that, if libdbus was unmaintained, wouldn't be merged :-P

Perhaps something like this?

  Note that you should $em(not) use dbus-glib to implement D-Bus services,
  as it is deprecated and unmaintained. Most services should also avoid
  libdbus (dbus-1), which is a low-level library and is awkward to use
  correctly: it is designed to be used via a language binding such as QtDBus.

@@ +57,5 @@
> +$link[>>http://dbus.freedesktop.org/doc/dbus-specification.html#introspection-format](D-Bus
> +specification), and is supported by tools such as $cmd(gdbus-codegen).
> +
> +Interface files should be installed to $code($var($$(datadir$))/dbus-1/interfaces)
> +so that other services can load them. There should be one file installed

"Interface files for interfaces intended to be public API" or something - if an interface is intended to be package-private it should certainly not be installed like this.

@@ +60,5 @@
> +Interface files should be installed to $code($var($$(datadir$))/dbus-1/interfaces)
> +so that other services can load them. There should be one file installed
> +per D-Bus interface, named after the interface, containing a single top-level
> +$code(<node>) element with a single $code(<interface>) beneath it. For example,
> +interface $code(org.example.MyService1.Manager) would be described by file

Minor: can we use com.example or net.example? I'm trying to avoid encouraging the incorrect meme "all D-Bus interface names start with org, and to make a D-Bus interface name you take a random string and put org in front" in favour of actual namespacing :-)

@@ +100,5 @@
> +If an interface defined by service A needs to be used by client B, client B
> +should declare a build time dependency on service A, and use the installed copy
> +of the interface file for any code generation it has to do. It should $em(not)
> +have a local copy of the interface, as that could then go out of sync with the
> +canonical copy in service Aâs git repository.

This paragraph seems inconsistent with your advice to cope gracefully with UnknownMethod - client B would FTBFS if service A is older than the newest version it has code to deal with.

Best-practice for how to propagate interface definitions around is not really clear, unfortunately.

@@ +214,5 @@
> +parsed; both are complex operations which are prone to bugs.
> +
> +A common, useful approach is to use enumerated values transmitted as unsigned
> +integers over the bus, with the interface documentation describing what each
> +value means, and how unrecognized values should be handled. This allows for

I'm not sure that this is clearly best-practice, in fact. The more constrained your environment, and the more closely-related your projects, the more sense an enum makes; the more you need to be extensible by third parties, the more you need strings. It's a trade-off.

Telepathy has a recurring pattern of (u, s) or (u, s, s) (either as separate arguments, or a struct, or members of a larger struct) where the u is defined centrally by the Telepathy spec, the first s is a domain-namespaced string providing more detail about the u, and the optional second s (mainly used for errors) is a debug message in either the C locale (programmer English) or the user's locale or the server's locale. For instance we might report a failure to connect as

    (NETWORK_ERROR, "org.freedesktop.Telepathy.Errors.NameResolutionError", "jabbre.org: no such domain")

or (I can't remember the actual Nokia-specific errors we had in Maemo but this is along the right lines)

    (NETWORK_ERROR, "com.nokia.Telepathy.Errors.NoSIMCard", "emergency calls only")

(This is only necessary in specifications where you anticipate needing this much extensibility.)

@@ +243,5 @@
> +    <property name="Status" type="u" access="read" />
> +
> +Similarly, enumerated values should be used instead of booleans, as they allow
> +extra values to be added in future, and there is no ambiguity about the sense of
> +the boolean value.

Specifications should be clear about whether an enum can be extended in future versions or not.

For an enum that can be extended, 0 being Unknown, Normal, Failed or another generic value is a good convention, and the documentation should mention unknown values (usually by saying to treat them as equivalent to Unknown).

For an enum that will not be extended (like Telepathy's ConnectionStatus), ideally the specification would say so (and yes I know Telepathy doesn't always).

@@ +278,5 @@
> +      <arg name="direction" type="u" direction="in" />
> +    </method>
> +
> +Enumerated values should also be used instead of $em(human readable) strings,
> +which should not be sent over the bus if possible. The service and client could

There is a subtlety here. Human-readable strings that you have received from elsewhere are better than nothing: for instance, if Telepathy gets a human-readable error message (presumably in either English or the user's language) from an IM server, it'll try to pass it out to the UI alongside the machine-readable enum/string pair.

But yes in general you're right.

@@ +321,5 @@
> +than signed values.
> +
> +Structures can be used almost anywhere in a D-Bus type, and arrays of structures
> +are particularly useful. Structures should be used wherever data fields are
> +related.

If you anticipate that something is going to be extended (by you or third parties) without breaking API, building in extension points is an excellent idea:

- a flags bitfield (only the spec author can extend this)
- a{sv} instead of struct, or as the last thing in a struct (anyone can extend this if you namespace the keys)

Again, it's a trade-off: do you pay the up-front cost of a more complex API so that extending it later becomes cheap?

@@ +356,5 @@
> +       e-mail addresses.
> +    -->
> +    <method name="AddContacts">
> +      <arg name="details" type="a(ss)" direction="in" />
> +      <arg name="ids" type="au" direction="out" />

I know this is just a toy example, but in real life I would always use something like aa{sv} -> au or aa{ss} -> au, because you're clearly going to want to add phone numbers or postal addresses or whatever in a later version.

(Actually I'd probably use a(sasas) from Telepathy's ContactInfo, because I've already been asked to design how to send arbitrary vCards over D-Bus, and that was what I came up with.)

@@ +375,5 @@
> +call; they should schedule other work (in a main loop) and handle the reply when
> +it is received. Even though method replies may take a while, the caller is
> +$em(guaranteed) to receive exactly one reply eventually. This reply could be the
> +return value from the method, an error from the method, or an error from the
> +D-Bus daemon indicating the service failed in some way (e.g. due to crashing).

... or a synthetic timeout from the client binding, if you don't specifically ask for the timeout to be infinite.

The default in most bindings is 25 seconds, arbitrarily chosen to be long enough that if "most" IPC (including smallish Internet requests like those in IM) has not completed by then, it probably isn't going to happen, but at the same time the user's app probably gives up and reports an error before the user decides it's the app's fault.

@@ +380,5 @@
> +
> +Because of this, service implementations should be careful to always reply
> +exactly once to each method call. Replying at the end of a long-running
> +operation is correct â the client will patiently wait until the operation has
> +finished and the reply is received.

Most (all?) client bindings set a default 25 second timeout, so clients of long-running operations will have to set a non-default timeout, and specifications with long-running operations should say so.

@@ +395,5 @@
> +reply. This means that a reply always indicates success, and an error always
> +indicates failure â rather than a reply indicating either depending on its
> +parameters, and having to return dummy values in the other parameters. Using
> +D-Bus error replies also means such failures can be highlighted in debugging
> +tools, simplifying debugging.

One of my personal rules for D-Bus is "anything can fail for any reason, so cope with that". Might be worth mentioning here.

@@ +446,5 @@
> +
> +The specification also defines the
> +$link[>>http://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-objectmanager]($code(org.freedesktop.DBus.ObjectManager))
> +interface, which should be used whenever a service needs to expose a variable
> +number of objects of the same class in a flat or tree-like structure. For

Careful - this is nearly right, but not 100% of the time.

ObjectManager should be used whenever a service needs to expose a bunch of objects of the same class, *and clients are expected to be interested in most of them*.

For instance, in Telepathy, the AccountManager should have been an ObjectManager<Account>, but the Connection probably still shouldn't be an ObjectManager<Channel> because Telepathy is designed to accommodate clients that care about calls but not text chat, or file transfers but nothing else, or whatever.

@@ +737,5 @@
> +   [id="security-policies"]
> +
> +Full coverage of securing D-Bus services is beyond the scope of this guide,
> +however there are some steps which you can take when designing an API to ease
> +security policy implementation.

I think it's worth mentioning the broad security model, which is this:

* There's a system bus and 0 or more session buses.

* Anyone may connect to the system bus. The system bus limits who can own names or send method calls, and only privileged users can receive unicast messages not intended for them. Everyone may receive broadcasts.

* Each session bus has an owner (a user). Only its owner may connect; on general-purpose Linux, it is not treated as a privilege boundary, so there is no further privilege separation between their processes.

The single most important thing to say about security policies for the system bus is that an <allow> or <deny> installed by an application should have the own, send_destination or receive_sender attribute unless there's a very good reason why not. Otherwise, the application affects other apps, usually negatively (CVE-2014-8148, CVE-2014-8156).

(The "very good reason why not" is that it is OK to have a policy allowing root to send messages with a send_interface but no send_destination, for the "agent" pattern, but I think that's beyond the scope of this guide.)

@@ +751,5 @@
> +Secondly, the default D-Bus security policy is restrictive enough to allow
> +sensitive data, such as passwords, to be safely sent over the bus in unicast
> +messages (including unicast signals); so there is no need to complicate APIs by
> +implementing extra security. Note, however, that sensitive data must $em(not) be
> +sent in broadcast signals, as they can be seen by all peers on the bus.

The default D-Bus security policy *for the system bus* is this restrictive.

The default policy for the session bus is not at all restrictive, but the security model says it isn't a privilege boundary anyway.

OSs where the session bus has been made a privilege boundary (typically those with LSMs) need to write their own security model, and audit their apps and D-Bus security policies to make sure their security model is satisfied.

@@ +771,5 @@
> +$link[>>http://dbus.freedesktop.org/doc/dbus-specification.html#message-bus-routing-match-rules](D-Bus
> +match rule) to make the stream more manageable.
> +
> +Previous versions of D-Bus have required the busâ security policy to be manually
> +relaxed to allow eavesdropping on all messages, especially on the system bus.

Correction: only on the system bus. The session bus has always allowed eavesdropping, at least upstream.

(In recent-ish versions the eavesdropper does have to opt-in to "yes I really do want to eavesdrop" with a special match rule, because we realised it was too easy to shoot yourself in the foot by eavesdropping when you didn't intend to, and trying to reply to other people's messages, but there is no security around that match rule.)

Created attachment 113221 [details] [review]
fixup [1/3]

Fix quoting in build system.

Created attachment 113222 [details] [review]
fixup [2/3]

Fix general review comments on the text body.

Created attachment 113223 [details] [review]
fixup [3/3]

Change from org.example to com.example and add a note about choosing service names.

Comment on attachment 113221 [details] [review]
fixup [1/3]

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

::: configure.ac
@@ +1436,4 @@
>  AS_IF([test "$YELP_BUILD" = "no"],[have_yelp_build=no],[have_yelp_build=yes])
>  
>  AS_IF([test "$enable_ducktype_docs" = "auto"],[
> +    AS_IF([test "$have_ducktype" = "no" || "$have_yelp_build" = "no"],[

you'll need a "test" after the ||

Comment on attachment 113222 [details] [review]
fixup [2/3]

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

This seems good.

::: doc/dbus-api-design.duck
@@ +60,5 @@
>  specification), and is supported by tools such as $cmd(gdbus-codegen).
>  
> +Interface files for public API should be installed to
> +$code($var($$(datadir$))/dbus-1/interfaces) so that other services can load
> +them. Private APIs should not be installed There should be one file installed

Add full stop: "... installed. There ..."

@@ +228,5 @@
> +avoided, the messages are more compact, and typos can be more easily avoided by
> +developers (if, for example, C enums are used in the implementation).
> +
> +Transmissing values as strings means additional values can be defined by third
> +parties without fear of conflicting over integer values.

"... values, for instance by using the same reverse-domain-name namespacing as D-Bus interfaces."? (or something)

@@ +233,5 @@
> +
> +In both cases, the interface documentation should describe the meaning of each
> +value. It should state whether the type can be extended in future and, if so,
> +how the service and client should handle unrecognized values â typically by
> +considering them equal to a âunknownâ or âfailureâ value.

an unknown

Maybe worth mentioning that using 0 in an enum for unknown is a good convention?

@@ +513,5 @@
>  
> +If clients are not expected to be interested in most of the exposed objects,
> +$code(ObjectManager) should $em(not) be used, as it will send all of the objects
> +to each client anyway, wasting bus bandwidth. A file manager, therefore, should
> +not expose the entire file system hierarchy using $code(ObjectManager).

Nice example!

Comment on attachment 113223 [details] [review]
fixup [3/3]

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

looks good

Created attachment 113285 [details] [review]
fixup [4/5]

Add a missing ‘test’

Created attachment 113286 [details] [review]
fixup [5/5]

Various minor typo fixes

Created attachment 113287 [details] [review]
doc: Add a guide to designing D-Bus APIs (updated and squashed)

Here’s a version with all the fixups squashed in (obsoletes attachment #113207 [details] [review], but I’ll leave that alone since the fixups still refer to it).

Created attachment 113528 [details] [review]
Fix out-of-tree ducktype build

ducktype defaults to converting foo/bar.duck to foo/bar.page, but
in an out-of-tree build, it should convert $srcdir/foo/bar.duck
to ${builddir}/foo/bar.page instead.

---

I'll probably squash this into Attachment #113287 [details].

This looks good to me, other than the out-of-tree build fix.

Created attachment 113532 [details] [review]
doc: Add a guide to designing D-Bus APIs

This guide gives some pointers on how to write D-Bus APIs which are nice
to use.

It adds an optional dependency on Ducktype and yelp-build from
yelp-tools. These are used when available, but are not required unless
--enable-ducktype-docs is passed to configure. They are required for
uploading the docs, however.

Bug:

(In reply to Philip Withnall from comment #17)
> Created attachment 113532 [details] [review] [review]
> doc: Add a guide to designing D-Bus APIs
> 
> This guide gives some pointers on how to write D-Bus APIs which are nice
> to use.
> 
> It adds an optional dependency on Ducktype and yelp-build from
> yelp-tools. These are used when available, but are not required unless
> --enable-ducktype-docs is passed to configure. They are required for
> uploading the docs, however.

Here’s the squashed patch, rebased against master as of now. It seems I don’t have permission to push to the D-Bus repository, so either I could get permission or you could push it. :-)

Fixed in git for 1.9.12, thanks!