Bug 39610 - document why we use intptr_t for {fd or HANDLE} union, or replace it with something better
Summary: document why we use intptr_t for {fd or HANDLE} union, or replace it with som...
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: All All
: medium normal
Assignee: Simon McVittie
QA Contact: John (J5) Palmieri
URL:
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2011-07-27 20:54 UTC by Lennart Poettering
Modified: 2014-09-24 12:15 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
fix a typo (1.17 KB, patch)
2011-07-27 20:55 UTC, Lennart Poettering
Details | Splinter Review
fix some redudant assignments (876 bytes, patch)
2011-07-27 20:55 UTC, Lennart Poettering
Details | Splinter Review
Split _dbus_fd_set_close_on_exec into Unix and Windows versions (4.02 KB, patch)
2014-09-11 11:17 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lennart Poettering 2011-07-27 20:54:54 UTC
unrelated to each other, but i am too lazy to open two bugs for it. Both are trivial.
Comment 1 Lennart Poettering 2011-07-27 20:55:16 UTC
Created attachment 49640 [details] [review]
fix a typo
Comment 2 Lennart Poettering 2011-07-27 20:55:49 UTC
Created attachment 49641 [details] [review]
fix some redudant assignments
Comment 3 Simon McVittie 2011-07-28 05:38:06 UTC
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.)
Comment 4 Simon McVittie 2011-07-28 07:45:38 UTC
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.
Comment 5 Lennart Poettering 2011-07-28 12:45:54 UTC
(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).
Comment 6 Simon McVittie 2011-07-29 05:02:32 UTC
(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.
Comment 7 Simon McVittie 2014-09-11 11:07:01 UTC
(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.
Comment 8 Simon McVittie 2014-09-11 11:15:48 UTC
(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.
Comment 9 Simon McVittie 2014-09-11 11:17:20 UTC
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.
Comment 10 Simon McVittie 2014-09-11 11:17:53 UTC
Adding Ralf to Cc because this involves Windows'isms
Comment 11 Simon McVittie 2014-09-11 11:18:21 UTC
Comment on attachment 49640 [details] [review]
fix a typo

superseded
Comment 12 Simon McVittie 2014-09-11 11:18:42 UTC
Comment on attachment 49641 [details] [review]
fix some redudant assignments

already committed
Comment 13 Ralf Habacker 2014-09-11 11:24:47 UTC
(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
Comment 14 Simon McVittie 2014-09-24 12:15:55 UTC
Ralf committed this, so I assume he approves of the Unix part too. Fixed in git for 1.9.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.