Bug 68308 - [PATCH] fix a typo: DBUS_ACTIVATION_ --> DBUS_STARTER_
Summary: [PATCH] fix a typo: DBUS_ACTIVATION_ --> DBUS_STARTER_
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: Other All
: medium normal
Assignee: Havoc Pennington
QA Contact:
URL:
Whiteboard: vaguely review+, giving a chance to veto
Keywords: patch
Depends on:
Blocks:
 
Reported: 2013-08-20 07:52 UTC by Chengwei Yang
Modified: 2013-11-01 12:13 UTC (History)
5 users (show)

See Also:
i915 platform:
i915 features:


Attachments
[PATCH] Fix a typo: DBUS_ACTIVATION_ --> DBUS_STARTER_ (1.09 KB, patch)
2013-08-20 07:57 UTC, Chengwei Yang
Details | Splinter Review
[PATCH] Remove unnecessary dbus_setenv() (1.21 KB, patch)
2013-10-09 01:56 UTC, Chengwei Yang
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chengwei Yang 2013-08-20 07:52:44 UTC
DBUS_ACTIVATION_ADDRESS and DBUS_ACTIVATION_BUS_TYPE has been changed to DBUS_STARTER_ADDRESS and DBUS_STARTER_BUS_TYPE since 2005.

As below commit

commit 8873c90f99303f9cc308f15f8d03e637911f5b9e
Author: Havoc Pennington <hp@redhat.com>
Date:   Tue Jan 18 20:42:15 2005 +0000
Comment 1 Chengwei Yang 2013-08-20 07:57:39 UTC
Created attachment 84313 [details] [review]
[PATCH] Fix a typo: DBUS_ACTIVATION_ --> DBUS_STARTER_
Comment 2 Simon McVittie 2013-08-22 17:30:19 UTC
What practical effect does this have? If it's been wrong since 2005, then perhaps it was never necessary (and I'm somewhat worried about regressions).
Comment 3 Chengwei Yang 2013-08-23 13:52:13 UTC
(In reply to comment #2)
> What practical effect does this have? If it's been wrong since 2005, then
> perhaps it was never necessary (and I'm somewhat worried about regressions).

Yeah, the support of DBUS_STARTER_X (called DBUS_ACTIVATION_X at that time) introduced in 2003, ten years ago. Not quite understand what's exactly the background, maybe it's useful to the client that which type of bus (system/session) activated it. Refer to DBus Spec: http://dbus.freedesktop.org/doc/dbus-specification.html#message-bus-starting-services

However, seems in the past 10 years, most of all are just go and connect the other two well-known types of bus (system and session), rather than connect to the starter bus.

Anyway, from my POV, this fix should not introduce any regression because once the client initalized it's three types of connections, it's safe to clear it's environment.
Comment 4 Simon McVittie 2013-09-05 13:31:32 UTC
It looks as though the intention of this code is that if you have this process tree:

dbus-daemon
  \- activated service com.example.MyService
     \- child process /usr/bin/myutil

then MyService gets the DBUS_STARTER_BUS_TYPE, DBUS_STARTER_ADDRESS from dbus-daemon, but myutil doesn't inherit them (which could conceivably make it behave incorrectly, e.g. if it wants to start a new session for regression tests or something like in Bug #63119).

However, if we haven't been doing this correctly for 8 years, and everyone has coped OK with that, then it can't have been all that important... and I think anyone who is starting a sub-session is responsible for sorting out their own environment, e.g. with the new dbus-run-session(1) (which, as of git master, does unset these variables).

Compounding this, setenv() is not thread-safe (e.g. Bug #65681): if you setenv() in one thread, a concurrent getenv() in other threads can return wrong pointers. So I'm very tempted to just remove these _dbus_setenv() calls, instead of making them (un)set the variable that actually has an effect.

Thiago, Colin, David, any thoughts?

GDBus doesn't seem to clear these environment variables, for what it's worth. It also mitigates the equivalent of Bug #63119 by ignoring DBUS_STARTER_ADDRESS, and just using DBUS_STARTER_BUS_TYPE to select whether it does the equivalent of dbus_bus_get (DBUS_BUS_SESSION, ...) or dbus_bus_get (DBUS_BUS_SYSTEM, ...). (Do you think we should do that too?)
Comment 5 Simon McVittie 2013-10-08 12:19:38 UTC
(In reply to comment #4)
> Compounding this, setenv() is not thread-safe (e.g. Bug #65681)

Also Bug #27585.
Comment 6 Chengwei Yang 2013-10-09 01:33:37 UTC
(In reply to comment #4)
> It looks as though the intention of this code is that if you have this
> process tree:
> 
> dbus-daemon
>   \- activated service com.example.MyService
>      \- child process /usr/bin/myutil
> 
> then MyService gets the DBUS_STARTER_BUS_TYPE, DBUS_STARTER_ADDRESS from
> dbus-daemon, but myutil doesn't inherit them (which could conceivably make
> it behave incorrectly, e.g. if it wants to start a new session for
> regression tests or something like in Bug #63119).
> 
> However, if we haven't been doing this correctly for 8 years, and everyone
> has coped OK with that, then it can't have been all that important... and I
> think anyone who is starting a sub-session is responsible for sorting out
> their own environment, e.g. with the new dbus-run-session(1) (which, as of
> git master, does unset these variables).
> 
> Compounding this, setenv() is not thread-safe (e.g. Bug #65681): if you
> setenv() in one thread, a concurrent getenv() in other threads can return
> wrong pointers. So I'm very tempted to just remove these _dbus_setenv()
> calls, instead of making them (un)set the variable that actually has an
> effect.

Seems it's a better idea giving that it unset two never setup envs (because the typo) in the past 8 years, this is just the same effect as just doesn't do the unset.

> 
> Thiago, Colin, David, any thoughts?
> 
> GDBus doesn't seem to clear these environment variables, for what it's
> worth. It also mitigates the equivalent of Bug #63119 by ignoring
> DBUS_STARTER_ADDRESS, and just using DBUS_STARTER_BUS_TYPE to select whether
> it does the equivalent of dbus_bus_get (DBUS_BUS_SESSION, ...) or
> dbus_bus_get (DBUS_BUS_SYSTEM, ...). (Do you think we should do that too?)
Comment 7 Chengwei Yang 2013-10-09 01:56:21 UTC
Created attachment 87312 [details] [review]
[PATCH] Remove unnecessary dbus_setenv()
Comment 8 Ralf Habacker 2013-10-09 05:12:35 UTC
Comment on attachment 87312 [details] [review]
[PATCH] Remove unnecessary dbus_setenv()

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

looks good
Comment 9 Simon McVittie 2013-10-09 09:30:17 UTC
(In reply to comment #8)
> Comment on attachment 87312 [details] [review]
> [PATCH] Remove unnecessary dbus_setenv()
...
> looks good

I'm going to give Thiago and Colin a chance to veto this, so it probably won't be in the 1.7.6 release I plan to do soon; but if they don't respond soon, I'll apply it for 1.7.8.
Comment 10 Colin Walters 2013-10-10 16:22:55 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > Comment on attachment 87312 [details] [review] [review]
> > [PATCH] Remove unnecessary dbus_setenv()
> ...
> > looks good
> 
> I'm going to give Thiago and Colin a chance to veto this, so it probably
> won't be in the 1.7.6 release I plan to do soon; but if they don't respond
> soon, I'll apply it for 1.7.8.

No objections here; I agree with Chengwei that basically the "starter bus" concept ended up not being useful.
Comment 11 Simon McVittie 2013-11-01 12:13:33 UTC
Fixed in git for 1.7.8, let's see whether it breaks anything...


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.