Works fine with systemd:


Mar 26 13:39:01 pocket dbus: [system] Activating via systemd service name='org.freedesktop.NetworkManager' unit='dbus-org.freedesktop.NetworkManager.service'
Mar 26 13:39:04 pocket dbus: [system] Activating service 'org.freedesktop.ModemManager'

First is via systemd, second is normal.  And now to debug what I thought was happening and now confirmed:

Mar 26 13:39:31 pocket dbus: [system] Activation of service 'org.bluez' timed out

Created attachment 44942 [details] [review]
updated path

This adds a bit more logging, such as on successful activation completion.

Created attachment 45393 [details] [review]
don't log unnecessarily, fix servicehelper logging

I think it's a good idea to add more logging!

I would however prefer avoiding using the term Service - e.g. instead of

 Successfully activated service '%s'

it should be

 Launched owner for well-known name '%s'

or for extra credit

 Launched owner (/lib/udisks/udisksd) for well-known name org.freedesktop.UDisks2

or something. I think Havoc had a big mailing list post with more details on why we should avoid using the term 'service' but I can't find it right now.

Review of attachment 45393 [details] [review]:

Looks good to me with two small changes. Would prefer changing the terminology as per comment 4 but I don't think that needs to block inclusion of the patch. Thanks.

::: bus/activation.c
@@ +2124,3 @@
+        dbus_move_error (&tmp_error, error);
+        dbus_free_string_array (argv);
+    if (!_dbus_spawn_async_with_babysitter (&pending_activation->babysitter, argv,

the use of braces here is a bit confusing ... there are no braces for the if-else part and then unrelated braces for a compound statement (I guess because you want to declare tmp_error). Suggest to move tmp_error up in scope.

@@ +2214,3 @@
     dbus_set_error(&error, code, str);
 
+

whitespace

(In reply to comment #4)

> or something. I think Havoc had a big mailing list post with more details on
> why we should avoid using the term 'service' but I can't find it right now.

The process name would definitely be nice (and ideally the pid, but that turns out to be a pain in the ass to get because of the intermediate child process).

I'm not getting the anti-service thing though; I mean, the files components and apps install are called org.example.Foo.service.  Actually just look at the spec:

http://dbus.freedesktop.org/doc/dbus-specification.html#message-bus-starting-services

(In reply to comment #6)
> (In reply to comment #4)
> 
> > or something. I think Havoc had a big mailing list post with more details on
> > why we should avoid using the term 'service' but I can't find it right now.
> 
> The process name would definitely be nice (and ideally the pid, but that turns
> out to be a pain in the ass to get because of the intermediate child process).
> 
> I'm not getting the anti-service thing though; I mean, the files components and
> apps install are called org.example.Foo.service.  Actually just look at the
> spec:
> 
> http://dbus.freedesktop.org/doc/dbus-specification.html#message-bus-starting-services

Yeah, I know, but I remember Havoc saying "we can't change that, too bad" or something. Let me try to find the mail.

I think it was this mail

 http://lists.freedesktop.org/archives/dbus/2004-December/001851.html

Can't believe it's _that_ long ago though... I thought it was more recent. Anyway, I still think terms like "name owner" is more precise and most of GDBus already follows this trend. It's not a biggie though.

I basically agree with davidz's review, except that I don't think calling things services is actually a problem. My understanding of the terminology is:

- the strings that appear in sender and destination are "bus names"

  - :1.2 is a "unique name", or a "unique bus name" if I want to disambiguate

  - com.example.Notepad is a "well-known name", or a "well-known bus name"

- a service is something that can be launched by service-activation;
  it is identified by its well-known name (but things other than activatable
  services can own well-known names too)

Some nitpicking (which I don't think blocks merge - the perfect is the enemy of the good):

> "Activating via systemd service name='%s' unit='%s'",
> "Activating systemd to hand-off service name='%s' unit='%s'",
> "Failed to activate via systemd service name='%s' unit='%s'",

I'd prefer this with more punctuation:

Activating via systemd: service name='%s' unit='%s'
Activating systemd to hand-off: service name='%s' unit='%s'
Failed to activate via systemd: service name='%s' unit='%s'

I agree that tmp_error should be hoisted to the top of the function, unless you're really attached to it having a limited scope via an otherwise-useless block, in which case it deserves a comment just outside it, like /* block to give tmp_error a narrower scope */. I think adding braces to the if/else just above the block would be good for clarity, too.

Assigning to Colin since he seems to be the one actively working on this.

(In reply to comment #9)
> I basically agree with davidz's review, except that I don't think calling
> things services is actually a problem. My understanding of the terminology is:
> 
> - the strings that appear in sender and destination are "bus names"
> 
>   - :1.2 is a "unique name", or a "unique bus name" if I want to disambiguate
> 
>   - com.example.Notepad is a "well-known name", or a "well-known bus name"
> 
> - a service is something that can be launched by service-activation;
>   it is identified by its well-known name (but things other than activatable
>   services can own well-known names too)

Sounds good to me.

(In reply to comment #9)
>
> Some nitpicking (which I don't think blocks merge - the perfect is the enemy of
> the good):

Updated for comments and pushed to dbus-1.4.