Bug 100795

Created attachment 131048 [details] [review]
document error codes specific to each call

Created attachment 131049 [details] [review]
document generic error codes

Comment on attachment 131047 [details] [review]
document all driver calls in the same section

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

> This patch simply moves RequestName, ReleaseName and ListNameOwners
> to the "Message Bus Messages" section.

Do you mean s/ListNameOwners/ListQueuedOwners/?

Looks reasonable to me other than that. smcv?

Comment on attachment 131048 [details] [review]
document error codes specific to each call

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

I think it might make sense to mention somewhere that these lists of error codes are potentially not exhaustive, and that clients should gracefully handle unrecognised errors (probably by retrying later). D-Bus implementations may always return their own error codes for other failure modes.

We should be careful about the extent to which the specification prescribes error codes (which must then be used by all implementations of D-Bus, not just dbus-daemon), and the extent to which it’s left implementation-defined. This patch seems reasonable to me, in that all these error codes are unlikely to vary between implementations of D-Bus, so it’s useful to mandate them all in the specification. However, we need to be careful to keep the phrasing implementation-independent.

Would it make sense to document somewhere else a standard set of error codes which are returned for invalid input? BadAddress, InvalidArgs, UnknownMethod, etc.

Comment on attachment 131049 [details] [review]
document generic error codes

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

This looks like what I just asked for in the review of the previous patch. :-)

(In reply to Philip Withnall from comment #4)
> Comment on attachment 131048 [details] [review] [review]
> document error codes specific to each call
> 
> Review of attachment 131048 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Would it make sense to document somewhere else a standard set of error codes
> which are returned for invalid input? BadAddress, InvalidArgs,
> UnknownMethod, etc.

⇒ attachment 131049 [details] [review]

Thanks for the patches; they all look good to me. I think we might want to make it a bit clearer how clients are expected to handle errors (especially errors which are not documented here).

smcv should take a look at these before they can be pushed though.

(In reply to Philip Withnall from comment #3)
> Comment on attachment 131047 [details] [review] [review]
> document all driver calls in the same section
> 
> Review of attachment 131047 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> > This patch simply moves RequestName, ReleaseName and ListNameOwners
> > to the "Message Bus Messages" section.
> 
> Do you mean s/ListNameOwners/ListQueuedOwners/?

Yes, thanks!

(In reply to Philip Withnall from comment #4)
> Comment on attachment 131048 [details] [review] [review]
> document error codes specific to each call
> 
> Review of attachment 131048 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> I think it might make sense to mention somewhere that these lists of error
> codes are potentially not exhaustive, and that clients should gracefully
> handle unrecognised errors (probably by retrying later). D-Bus
> implementations may always return their own error codes for other failure
> modes.
> 
> We should be careful about the extent to which the specification prescribes
> error codes (which must then be used by all implementations of D-Bus, not
> just dbus-daemon), and the extent to which it’s left implementation-defined.
> This patch seems reasonable to me, in that all these error codes are
> unlikely to vary between implementations of D-Bus, so it’s useful to mandate
> them all in the specification. However, we need to be careful to keep the
> phrasing implementation-independent.

Indeed, my intention was to document things that are de facto standard, as clients are likely to depend on the behavior of the dbus daemon for these errors. I'll do another patch to clarify a bit more that this is not exhaustive.

Created attachment 131166 [details] [review]
document all driver calls in the same section

Fix the commit message.

Created attachment 131167 [details] [review]
document generic error codes

Make it clear that the list of errors is not exhaustive.

Comment on attachment 131167 [details] [review]
document generic error codes

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

Seems reasonable to me, thanks for the clarification.

smcv?

(In reply to Tom Gundersen from comment #10)
> Created attachment 131166 [details] [review]
> document all driver calls in the same section

Sorry, I can't review this. A diff > 100K is not a particularly useful thing to receive.

I'll try to break up similar changes into something more comprehensible...

Comment on attachment 131166 [details] [review]
document all driver calls in the same section

(In reply to Simon McVittie from comment #13)
> Sorry, I can't review this. A diff > 100K is not a particularly useful thing
> to receive.
> 
> I'll try to break up similar changes into something more comprehensible...

I've pushed a version that differs only in whitespace from yours (I added blank lines between methods to make it easier to navigate through the document), but is broken up so that git diff produces reviewable results.

Comment on attachment 131048 [details] [review]
document error codes specific to each call

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

::: doc/dbus-specification.xml
@@ +5262,5 @@
> +                  <entry>The maximum number of active connections have been exceeded</entry>
> +                </row>
> +                <row>
> +                  <entry>org.freedesktop.DBus.Error.Failed</entry>
> +                  <entry>The client is already registered on the bus</entry>

I'm not so sure about documenting Failed anywhere. The intention is that Error.Failed is like GLib's WHATEVER_ERROR_FAILED - it indicates "something went wrong, but there is no more specific error code".

In particular, if a hypothetical non-reference implementation of D-Bus raised o.fd.D.E.InvalidArgs (or something) to report an excessive number of connections, it might be reasonable to open a bug asking the implementer to change that to o.fd.D.E.LimitsExceeded; but if it raised some other error on attempts to call Hello() twice (perhaps some sort of SequenceError or ProtocolError), then I don't think it would be appropriate to open a bug asking it to use o.fd.D.E.Failed instead.

I think it would be more appropriate to document Failed as a generic error alongside InvalidInterface.

(I'm surprised the reference implementation doesn't kick the caller off the bus for trying to call Hello() more than once, actually. That's unusually generous of us...)

@@ +5349,5 @@
> +                  <entry>The requested name is reserved or not a valid well-known name</entry>
> +                </row>
> +                <row>
> +                  <entry>org.freedesktop.DBus.Error.AccessDenied</entry>
> +                  <entry>Application is not allowed to own name</entry>

The caller is not allowed to own the requested name

or maybe

The calling connection is not ...

@@ +5715,5 @@
> +              </thead>
> +              <tbody>
> +                <row>
> +                  <entry>org.freedesktop.DBus.Error.NameHasNoOwner</entry>
> +                  <entry>The name does not exist</entry>

The name does not exist (in other words, there are no connections in the queue to own it)

@@ +6014,5 @@
> +                  <entry>The maximum number of pending activations has been exceeded</entry>
> +                </row>
> +                <row>
> +                  <entry>org.freedesktop.DBus.Error.ServiceUnknown</entry>
> +                  <entry>The name is not activatable</entry>

The name is not activatable and is not currently owned by a connection


(because if it was owned already, the method would return ALREADY_RUNNING instead of raising an error)

@@ +6112,5 @@
> +              </thead>
> +              <tbody>
> +                <row>
> +                  <entry>org.freedesktop.DBus.Error.AccessDenied</entry>
> +                  <entry>Invalid object path, or the caller does not have permission to alter the activation environemnt</entry>

Spelling: environment

The fact that an invalid object path provokes this error is really an obscure implementation detail rather than part of the specification - it is security hardening against people installing faulty system bus policies that allow arbitrary messages to be sent to a particular object path, because the reference implementation has the weird historical quirk that it accepts most method calls on arbitrary object paths.

I think I'd prefer the paragraph about generic errors to say something like this:

      <para>
        The special message bus name <literal>org.freedesktop.DBus</literal>
        responds to a number of additional method call messages, all of which
        should be sent to the object path <literal>/org/freedesktop/DBus</literal>.
        That object path is also used when emitting the
        <xref linkend='bus-messages-name-owner-changed'/> signal.
      <para>

      <para>
        The message bus may return a number of different error messages on failure
        etc.
      </para>

      <para>
        For historical reasons, some of these methods are available on multiple object
        paths. Message bus implementations should accept method calls that were
        added before specification version 0.26 on any object path. Message bus
        implementations should not accept newer method calls on unexpected
        object paths, and as a security hardening measure, older
        method calls that are security-sensitive may be rejected with the error
        <literal>org.freedesktop.DBus.Error.AccessDenied</literal> when called on an
        unexpected object path. Client software should send all method calls to
        <literal>/org/freedesktop/DBus</literal> instead of relying on this.
      </para>

Maybe AccessDenied and perhaps LimitsExceeded should be listed as generic error replies - a message bus implementation is free to make any method call of its choice fail, if it decides that the method call is problematic.

@@ +6722,5 @@
> +                  <entry>The match rule string is invalid</entry>
> +                </row>
> +                <row>
> +                  <entry>org.freedesktop.DBus.Error.AccessDenied</entry>
> +                  <entry>The caller does not have permission to eavesdrop</entry>

The caller does not have permission to add this match rule (typically returned if a caller attempts to eavesdrop and does not have permission)


(because a message bus would be free to disallow any other class of match rule that it wanted to, for example forbidding overly broad match rules, or match rules that can only match messages that the caller is not allowed to receive - and if it did, AccessDenied would be the right error)

@@ +6862,5 @@
> +              </thead>
> +              <tbody>
> +                <row>
> +                  <entry>org.freedesktop.DBus.Error.AccessDenied</entry>
> +                  <entry>Invalid object path, or the caller does not have the permission to eavesdrop</entry>

Again, I'd prefer not to mention the weirdness with object paths, and I'd prefer to be a bit more vague about what permission is denied

(In reply to Philip Withnall from comment #4)
> We should be careful about the extent to which the specification prescribes
> error codes (which must then be used by all implementations of D-Bus, not
> just dbus-daemon), and the extent to which it’s left implementation-defined.

I would go further than this. I think we should make it clear that these error names are merely advisory/best-practice: any D-Bus method call can fail with any error code, and callers have to deal with that.

The times when it makes sense to mandate or strongly recommend particular error codes are when the correct response from a caller differs according to the error code provided.

It seems to me as though the error codes break down like this, where "fail" means report some sort of error to the user and cease to work as intended:

* NameHasNoOwner: This is genuinely useful information: it indicates
  that a process we were trying to communicate with has gone away.
  The correct response is application-specific.

* ServiceUnknown: Some component that we're trying to talk to is
  unavailable. If it is mandatory, fail. If it is optional, gracefully
  disable that part of the functionality.
  (But this is equally necessary handling for D-Bus calls to the service
  itself.)

* MatchRuleInvalid etc.: If the calling application is using a feature
  that its author knows is a recent addition, it could maybe fall back to
  doing something less efficient but equally valid. Otherwise, it will
  just have to fail.

* Unable to determine pid or whatever: This is semi-useful information:
  it indicates that we shouldn't give that process any special level
  of trust.

* InvalidArgs, MatchRuleNotFound etc.: There is no sensible runtime
  response to this programming error. The author of the calling application
  must fix their code. Until then, the application will just fail.

* Access denied: Fail.
  (But this is equally necessary handling for D-Bus calls to the service
  itself.)

* Limits exceeded: In theory it could try again later, but in practice
  it will just fail.

(In reply to Simon McVittie from comment #15)
> The fact that an invalid object path provokes this error is really an
> obscure implementation detail rather than part of the specification - it is
> security hardening against people installing faulty system bus policies that
> allow arbitrary messages to be sent to a particular object path, because the
> reference implementation has the weird historical quirk that it accepts most
> method calls on arbitrary object paths.
> 
> I think I'd prefer the paragraph about generic errors to say something like
> this:
...
>       <para>
>         For historical reasons, some of these methods are available on
> multiple object
>         paths.
...

I've done something similar on Bug #101256.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/172.