Bug 45896 - remove dead code to deal with nonce-tcp as a Windows-specific thing
Summary: remove dead code to deal with nonce-tcp as a Windows-specific thing
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: Other All
: medium enhancement
Assignee: Ralf Habacker
QA Contact: John (J5) Palmieri
URL:
Whiteboard:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2012-02-10 07:16 UTC by Simon McVittie
Modified: 2012-06-05 04:24 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Remove duplicate nonce-tcp (client side) transport on Windows (2.07 KB, patch)
2012-02-10 07:16 UTC, Simon McVittie
Details | Splinter Review
Remove duplicate nonce-tcp (service-side) transport on Windows (1.84 KB, patch)
2012-02-10 07:23 UTC, Simon McVittie
Details | Splinter Review
_dbus_transport_new_for_tcp_socket: add missing commas to address (1.21 KB, patch)
2012-02-24 03:25 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 2012-02-10 07:16:21 UTC
Created attachment 56874 [details] [review]
Remove duplicate nonce-tcp (client side) transport on  Windows

_dbus_transport_open_socket is called before
_dbus_transport_open_platform_specific, and now handles nonce-tcp, so
this version is no longer useful.

---

I noticed this while looking at another bug. I don't run Windows, so this will need testing, but I can confirm that nonce-tcp does work on Unix...
Comment 1 Simon McVittie 2012-02-10 07:23:22 UTC
Created attachment 56875 [details] [review]
Remove duplicate nonce-tcp (service-side) transport on  Windows

Turns out this was duplicated too. We can just use the
platform-independent version, which uses the same code.
Comment 2 Ralf Habacker 2012-02-17 03:41:21 UTC
(In reply to comment #0)
> Created attachment 56874 [details] [review] [review]
> Remove duplicate nonce-tcp (client side) transport on  Windows
> 
> _dbus_transport_open_socket is called before
> _dbus_transport_open_platform_specific, and now handles nonce-tcp, so
> this version is no longer useful.
> 
> ---
> 
> I noticed this while looking at another bug. I don't run Windows, so this will need testing, but I can confirm that nonce-tcp does work on Unix...

Which kind of nonce-tcp address do you have used, so that this can be verified  on windows ?
Comment 3 Simon McVittie 2012-02-20 03:03:15 UTC
(In reply to comment #2)
> > I noticed this while looking at another bug. I don't run Windows,
> > so this will need testing, but I can confirm that nonce-tcp does work
> > on Unix...
> 
> Which kind of nonce-tcp address do you have used, so that this can be
> verified  on windows ?

The verification I'm asking for is more like "with these patches applied, normal usage on Windows still works". I believe it should, because there's now a second copy of nonce-tcp in dbus-transport-socket, and the one in dbus-transport-socket gets tried first, so as far as I can see, the Windows-specific copy is never actually run (even on Windows).

What I've tested on Unix is test/loopback.c, which puts a DBusServer on "nonce-tcp:host=127.0.0.1" then connects to whatever port the DBusServer ends up with.
Comment 4 Ralf Habacker 2012-02-20 05:48:23 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > > I noticed this while looking at another bug. I don't run Windows,
> > > so this will need testing, but I can confirm that nonce-tcp does work
> > > on Unix...
> > 
> > Which kind of nonce-tcp address do you have used, so that this can be
> > verified  on windows ?
> 
> The verification I'm asking for is more like "with these patches applied, normal usage on Windows still works". I believe it should, because there's now a
> second copy of nonce-tcp in dbus-transport-socket, and the one in dbus-transport-socket gets tried first, so as far as I can see, the Windows-specific copy is never actually run (even on Windows).
> 
> What I've tested on Unix is test/loopback.c, 

unfortunally this test depends on glib, which is not integrated into windows windows dbus, so I cannot run it. 

> which puts a DBusServer on "nonce-tcp:host=127.0.0.1" then connects to whatever port the DBusServer ends up with.

As a workaround I used "nonce-tcp:host=127.0.0.1" in session.conf and launched a dbus-daemon with that address. 

The log now reported me the real used session bus address 

nonce-tcp:host=127.0.0.1,port=39373,noncefile=C%3a\Users\admin\AppData\Local\Temp/dbus_nonce-FHI1Sq4n,guid=83380282fd7a433021c38fc14f424648

which I used to set the DBUS_SESSION_BUS_ADDRESS environment variable. 

Now I can connect to the dbus-daemon using qdbusviewer. 

Then i killed dbus-daemon and used the following address in session.conf 

nonce-tcp:host=127.0.0.1,port=39373,noncefile=C%3a\Users\admin\AppData\Local\Temp/dbus_nonce-FHI1Sq4n,guid=83380282fd7a433021c38fc14f424648

and set DBUS_SESSION_BUS_ADDRESS environment to 
nonce-tcp:host=127.0.0.1,port=39373,noncefile=C%3a\Users\admin\AppData\Local\Temp/dbus_nonce-FHI1Sq4n,guid=83380282fd7a433021c38fc14f424648

and run qdbusviewer 
to which i *cannot* connect to. 

I found the following message in the log 
[3928] qdbusviewer: [dbus\dbus-transport.c(205):_dbus_transport_init_base] Initialized transport on address nonce-tcp:host=127.0.0.1,port=39373noncefile=C:\Users\admin\AppData\Local\Temp/dbus_nonce-FHI1Sq4n

There is a ',' missing between the port value and the 'noncefile' string
port=39373noncefile

There seems to be something broken in the nonce-tcp stuff.
Comment 5 Simon McVittie 2012-02-21 06:48:24 UTC
(In reply to comment #4)
> There seems to be something broken in the nonce-tcp stuff.

Indeed. Do you get the same failure without the patches from this bug? (If so, it's not a regression, so perhaps we should merge them anyway, to have less duplicate code.)
Comment 6 Simon McVittie 2012-02-24 02:40:44 UTC
(In reply to comment #4)
> unfortunally this test depends on glib, which is not integrated into windows
> windows dbus, so I cannot run it. 

FYI, it's not integrated into cmake, but works fine in Autotools-targeting-Windows (I have a cross-compilation environment using mingw-w64).

> The log now reported me the real used session bus address 
> 
> nonce-tcp:host=127.0.0.1,port=39373,noncefile=C%3a\Users\admin\AppData\Local\Temp/dbus_nonce-FHI1Sq4n,guid=83380282fd7a433021c38fc14f424648
> 
> which I used to set the DBUS_SESSION_BUS_ADDRESS environment variable. 

This is almost what loopback.c does (but using dbus-daemon --print-address rather than a debug log). Bug #46049 prevents that from working on Windows.

> Then i killed dbus-daemon and used the following address in session.conf 
> 
> nonce-tcp:host=127.0.0.1,port=39373,noncefile=C%3a\Users\admin\AppData\Local\Temp/dbus_nonce-FHI1Sq4n,guid=83380282fd7a433021c38fc14f424648

It doesn't usually make sense to force a specific port/noncefile/guid in the listening address (what you did first was a more realistic use of nonce-tcp), but OK...

> [3928] qdbusviewer: [dbus\dbus-transport.c(205):_dbus_transport_init_base]
> Initialized transport on address
> nonce-tcp:host=127.0.0.1,port=39373noncefile=C:\Users\admin\AppData\Local\Temp/dbus_nonce-FHI1Sq4n
> 
> There is a ',' missing between the port value and the 'noncefile' string
> port=39373noncefile

This is a bug in platform-independent code (dbus-transport-socket.c); it's equally broken on Unix and Windows. Patch on the way, but it's pretty obvious (add some commas).
Comment 7 Simon McVittie 2012-02-24 03:24:47 UTC
(In reply to comment #4)
> Then i killed dbus-daemon and used the following address in session.conf 
> 
> nonce-tcp:host=127.0.0.1,port=39373,noncefile=C%3a\Users\admin\AppData\Local\Temp/dbus_nonce-FHI1Sq4n,guid=83380282fd7a433021c38fc14f424648

That's not meant to work: the server (dbus-daemon in this case) always ignores the 'noncefile' and 'guid' address entries, and generates new ones. The Windows-specific duplicate code that I deleted from dbus/dbus-server-win.c in Attachment #56875 [details] doesn't read those entries either.

(I'm not trying to change how anything works here, just remove duplication: there's nothing Windows-specific about nonce-tcp, and as far as I can see, the code structure is such that the generic copy is already the only one that gets used.)
Comment 8 Simon McVittie 2012-02-24 03:25:55 UTC
Created attachment 57582 [details] [review]
_dbus_transport_new_for_tcp_socket: add missing commas to  address

Ralf pointed out that the address doesn't round-trip correctly.

---

This is actually just cosmetic, as far as I can see: it's in the client side (transport, not server), so nothing except debug messages should be using this value. Still, it's good to fix it.
Comment 9 Ralf Habacker 2012-02-24 04:27:36 UTC
(In reply to comment #7)
> (In reply to comment #4)
> > Then i killed dbus-daemon and used the following address in session.conf 
> > 
> > nonce-tcp:host=127.0.0.1,port=39373,noncefile=C%3a\Users\admin\AppData\Local\Temp/dbus_nonce-FHI1Sq4n,guid=83380282fd7a433021c38fc14f424648
> 
> That's not meant to work: the server (dbus-daemon in this case) always ignores
> the 'noncefile' and 'guid' address entries, and generates new ones. The
> Windows-specific duplicate code that I deleted from dbus/dbus-server-win.c in
> Attachment #56875 [details] doesn't read those entries either.
> 
> (I'm not trying to change how anything works here, just remove duplication:
> there's nothing Windows-specific about nonce-tcp, and as far as I can see, the
> code structure is such that the generic copy is already the only one that gets
> used.)

sure, I tried to get the nonce-tcp stuff runnning on windows, but failed because I did not find the correct way how to use it. 

When I use 'nonce-tcp:host=127.0.0.1' as listening address, I wasn't able to figure out which related client address to use, especially how the client will get the name for the noncefile file. 

I tried some more combinations and had only success with the above mentioned address.
Comment 10 Simon McVittie 2012-02-24 08:10:02 UTC
(In reply to comment #9)
> When I use 'nonce-tcp:host=127.0.0.1' as listening address, I wasn't able to
> figure out which related client address to use, especially how the client will
> get the name for the noncefile file.

In C code, you use dbus_server_get_address() to get the client address out (see loopback.c).

When running dbus-daemon, you use dbus-daemon --print-address, except that doesn't work on Windows, because of Bug #46049. With that bug fixed (I attached a patch, but I've only tested it under emulation), you can get the address out that way, and use it as the client address.

Either of those should give you the same address that was in the debug log; you can put it in DBUS_SESSION_BUS_ADDRESS to test it, for instance. (That's all that's meant to work; there's nothing as elaborate as Windows autolaunch.)

In the normal Unix setup, dbus-launch puts the address from dbus-daemon --print-address in the DBUS_SESSION_BUS_ADDRESS environment variable and a hidden X11 window when you log in (or just in the hidden X11 window, when autolaunching occurs), and everything else gets it from there.
Comment 11 Ralf Habacker 2012-02-28 05:43:47 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > When I use 'nonce-tcp:host=127.0.0.1' as listening address, I wasn't able to
> > figure out which related client address to use, especially how the client will
> > get the name for the noncefile file.
> 
> In C code, you use dbus_server_get_address() to get the client address out (see
> loopback.c).
> 
> When running dbus-daemon, you use dbus-daemon --print-address, except that
> doesn't work on Windows, because of Bug #46049. With that bug fixed (I attached
> a patch, but I've only tested it under emulation), you can get the address out
> that way, and use it as the client address.
> 
> Either of those should give you the same address that was in the debug log; you
> can put it in DBUS_SESSION_BUS_ADDRESS to test it, for instance. (That's all
> that's meant to work; there's nothing as elaborate as Windows autolaunch.)
> 
> In the normal Unix setup, dbus-launch puts the address from dbus-daemon
> --print-address in the DBUS_SESSION_BUS_ADDRESS environment variable and a
> hidden X11 window when you log in (or just in the hidden X11 window, when
> autolaunching occurs), and everything else gets it from there.

After fixing #46049 nonce-tcp protocol will still not be usable on windows because 

1. dbus-launch on windows will not fetch the address 
2. dbus client library still uses CreateProcess to spawn dbus-daemon on autolaunching 
3. dbus-launch or dbus-client library do not store the session bus address into the parent environment

Setting parent environment on windows looks complicated http://www.perlmonks.org/?node_id=658278
and registry settings are unusable for portable installations, so here we are in trouble
Comment 12 Simon McVittie 2012-03-12 06:42:50 UTC
(In reply to comment #11)
> After fixing #46049 nonce-tcp protocol will still not be usable on windows

I don't mind that it doesn't work without configuration; neither do the tcp and unix transports, or the nonce-tcp transport on Unix. All I want is that if you have an out-of-band way to communicate the server address (i.e. dbus_server_get_address()) to the client library, nonce-tcp works. Your testing has already demonstrated that that's true (Comment #4), as far as I can see.

What I'm mainly trying to achieve with this bug is just to get rid of a duplicate secondary copy of part of the nonce-tcp implementation. In Unix builds, there is only one copy of the setup logic for nonce-tcp, in dbus-transport-socket.c (for clients) and dbus-server-socket.c (for servers). I know that it works on Unix, because loopback.c tests it; it didn't work on Unix before commit 54de9c060 due to Bug #34569.

On Windows, there is the same code as on Unix, and also a second copy in the Windows-specific files, which is deleted by Attachment #56874 [details] and Attachment #56875 [details]. As far as I can tell, the platform-independent version is the one that is actually used for nonce-tcp addresses, because it comes first in the search order, so the second, Windows-specific version is never reached.

I assume that the Windows-specific version was written first, and then someone noticed that there was no reason why it couldn't work on Unix too, and copied it into the generic transport code. I don't know why they copied it instead of moving it; it seems to have been duplicated ever since nonce-tcp was first added to the main D-Bus repository (commit 5e2a99c12c7e3).

If you've reviewed the patches attached here and they are OK, please say so and I'll merge them :-)

> Setting parent environment on windows looks complicated
> http://www.perlmonks.org/?node_id=658278

You can't do that on Unix either (the situation is the same, except that there is no Registry, so you can't use that method even if there weren't reasons not to do so). So, no problem there.
Comment 13 Ralf Habacker 2012-05-07 06:58:48 UTC
(In reply to comment #12)
> If you've reviewed the patches attached here and they are OK, please say so and
> I'll merge them :-)
> 
Found some time to review this. nonce-tcp works with this patches on windows.
Comment 14 Simon McVittie 2012-06-05 04:24:12 UTC
(In reply to comment #13)
> Found some time to review this. nonce-tcp works with this patches on windows.

Thanks, fixed in git for 1.6.0.


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.