Bug 39610

Created attachment 49640 [details] [review]
fix a typo

Created attachment 49641 [details] [review]
fix some redudant assignments

Review of attachment 49640 [details] [review]:

review- to this one.

::: dbus/dbus-sysdeps-unix.c
@@ +2801,3 @@
  */
 void
+_dbus_fd_set_close_on_exec (int fd)

No, it's deliberately intptr_t - on Windows, it's actually a HANDLE (a pointer), not a numeric file descriptor, so it needs to be pointer-sized (which D-Bus assumes to be at least int-sized).

(Yeah, we should be using a struct or union really, or at least document why this is intptr_t.)

Review of attachment 49641 [details] [review]:

Yeah, these look OK. The patch that added these went through the whole source tree making sure that fds were set to -1 after they were closed, but that provably doesn't matter here.

(In reply to comment #4)
> Review of attachment 49641 [details] [review]:
> 
> Yeah, these look OK. The patch that added these went through the whole source
> tree making sure that fds were set to -1 after they were closed, but that
> provably doesn't matter here.

OK, commited this one to dbus-1.4 and master.

Will leave the bug open, as someone should really document the other issue in a comment (and honestly, I think the code is very much broken like this. dbus_close should not be shared with Windows at all I'd claim. The intptr_t "solution" is an abomination).

(In reply to comment #5)
> Will leave the bug open, as someone should really document the other issue in a
> comment (and honestly, I think the code is very much broken like this.
> dbus_close should not be shared with Windows at all I'd claim. The intptr_t
> "solution" is an abomination).

Yeah, that, pretty much. A better solution would be to distinguish between file-handles (HANDLE on Windows, fd on Unix) and sockets (small integer on either platform), systematically, and make it obvious which one each function expects/returns.

(In reply to comment #5)
> (and honestly, I think the code is very much broken like this.
> dbus_close should not be shared with Windows at all I'd claim. The intptr_t
> "solution" is an abomination).

_dbus_close() no longer exists on Windows.

_dbus_close_socket() takes an int argument, which is specifically a socket (in the sense of BSD sockets, i.e. a fd on Unix and a SOCKET on Windows).

So that part is done.

(In reply to comment #6)
> A better solution would be to distinguish between
> file-handles (HANDLE on Windows, fd on Unix) and sockets (small integer on
> either platform)

It's actually a little more complicated than that.

A Windows HANDLE is pointer-sized, but not necessarily a pointer.

A Windows SOCKET is int-sized (it's actually an unsigned int, which we use as if it was a signed int, but apparently that has always worked in practice...) that has the property that you can pass ((HANDLE) a_socket) to APIs that expect a HANDLE and it'll work.

Other native Windows file-handles (e.g. from CreateFile()) do not necessarily fit in an int-sized variable.

File descriptors as returned by _pipe() etc. are not a Windows kernel feature; they are an invention of the C library (msvcrt*.dll) in the same way as FILE *. They cannot be passed between C libraries, in the same way that on Linux, you can't take a FILE * from glibc and expect uclibc, dietlibc or Bionic to understand it, even if you somehow manage to get more than one libc in your process-space without crashing.

So it was in fact valid to change _dbus_fd_set_close_on_exec to take an int... as long as on Windows, you only ever pass a SOCKET to it, never some other sort of HANDLE.

Since all the calls to _dbus_fd_set_close_on_exec() are in platform-specific code anyway, I think it's simpler to make it a Unix-specific function, and rename Windows' implementation to _dbus_win_handle_set_close_on_exec() to indicate that it cannot be called on arbitrary file descriptors, only on SOCKETs or other HANDLEs.

Created attachment 106125 [details] [review]
Split _dbus_fd_set_close_on_exec into Unix and Windows versions

On Unix, the thing that can be made close-on-exec is a file descriptor,
which is an int.

On Windows, the thing that can be made close-on-exec is a HANDLE,
which is pointer-sized (but not necessarily a pointer!). In practice,
on Windows we only called _dbus_fd_set_close_on_exec() on socket
pseudo-file-descriptors (SOCKET, which is an unsigned int);
every SOCKET can validly be cast to HANDLE, but not every HANDLE
is a SOCKET.

Before this commit we used an intptr_t as a sort of fake
union { int; HANDLE; }, which just obscures what's going on.

In practice, everything that called _dbus_fd_set_close_on_exec()
is really platform-specific anyway, so let's just have two separate
functions and call this solved.

---

The Windows version, _dbus_win_handle_set_close_on_exec(), is now static. It can be made extern if another module needs it, but at the moment nothing does.

Adding Ralf to Cc because this involves Windows'isms

Comment on attachment 49640 [details] [review]
fix a typo

superseded

Comment on attachment 49641 [details] [review]
fix some redudant assignments

already committed

(In reply to comment #9)
> Created attachment 106125 [details] [review] [review]
> Split _dbus_fd_set_close_on_exec into Unix and Windows versions
> 
... 
> The Windows version, _dbus_win_handle_set_close_on_exec(), is now static. It
> can be made extern if another module needs it, but at the moment nothing
> does.
windows part looks good

Ralf committed this, so I assume he approves of the Unix part too. Fixed in git for 1.9.0.