(In reply to Simon McVittie from comment #0) > #ifdef DBUS_WIN > typedef int DBusSocket; > #else > typedef SOCKET DBusSocket; > #endif This one is less invasive. > > typedef struct { > #ifdef DBUS_WIN > SOCKET windows_socket; > #else > int unix_fd; > #endif > } DBusSocket; > this requires to refactor any access to socket fd's and is much more work. I would prefer the first one. > Unix-specific functions can continue to take and return int: the fact that > fds are ints, and the fact that sockets are just fds, is quite a fundamental > Unix ABI thing. Similarly, Windows-specific functions can use SOCKET or > HANDLE directly if desired. Sure, I'm not sure, when to use the first and when the other. With the appended patch I tried to change this. Created attachment 114033 [details] [review] DBusSocket refactoring - unix part Created attachment 114034 [details] [review] DBusSocket refactoring - windows part Created attachment 114035 [details] [review] DBusSocket refactoring - common part Created attachment 114036 [details] [review] DBusSocket refactoring - windows part (update) remved some unrelated changes Comment on attachment 114034 [details] [review] DBusSocket refactoring - windows part Review of attachment 114034 [details] [review]: ----------------------------------------------------------------- This can't be applied until after the common part is, afaics. I'd prefer all the s/int/DBusSocket/ to be one commit really, without any of the unrelated changes that seem to have crept in - I don't think there's any need to split it into Windows/Unix/common. ::: dbus/dbus-spawn-win.c @@ +77,5 @@ > char **envp; > > HANDLE child_handle; > + DBusSocket socket_to_babysitter; /* Connection to the babysitter thread */ > + DBusSocket socket_to_main; These are really sockets? ::: dbus/dbus-sysdeps-util-win.c @@ -380,4 @@ > const char *filename_c; > WIN32_FILE_ATTRIBUTE_DATA wfad; > char *lastdot; > - DWORD rc; Not actually part of this change afaics? ::: dbus/dbus-sysdeps-win.c @@ -52,4 @@ > #include "dbus-credentials.h" > > #include <windows.h> > -#include <ws2tcpip.h> Why? @@ +645,2 @@ > dbus_bool_t > _dbus_socket_is_invalid (int fd) This function should take a DBusSocket too, then you wouldn't need... @@ +645,4 @@ > dbus_bool_t > _dbus_socket_is_invalid (int fd) > { > + return fd == (int)INVALID_SOCKET ? TRUE : FALSE; ... this cast. @@ +1054,5 @@ > * @returns #FALSE on failure (if error is set) > */ > dbus_bool_t > +_dbus_full_duplex_pipe (DBusSocket *fd1, > + DBusSocket *fd2, Not your fault, and not a merge blocker for this change, but the name of this function is silly (a pipe is a jargon word, and this is not a pipe), and making this change to it makes that even more obvious. Perhaps we should call it _dbus_socketpair() since it has exactly the semantics "do the same as socketpair() on Unix, or as close to that as you can achieve on Windows"... @@ +1706,5 @@ > tmp = ai; > while (tmp) > { > + DBusSocket fd = INVALID_SOCKET, *newlisten_fd; > + if ((fd = (DBusSocket)socket (tmp->ai_family, SOCK_STREAM, 0)) == INVALID_SOCKET) Why the cast? MSDN says socket() returns a SOCKET already, and you're changing DBusSocket to be exactly a SOCKET? @@ +2271,4 @@ > if (!_dbus_string_lengthen (str, n_bytes)) > return FALSE; > > + p = (BYTE*)_dbus_string_get_data_len (str, old_len, n_bytes); This seems to be nothing to do with sockets? It does make sense, assuming the Windows crypto API you're using is done in terms of BYTE*, but it's really part of Bug #15522. I still think the right answer to that bug is to add _dbus_string_get_udata[_len] static inline functions that encapsulate the cast. @@ +2307,4 @@ > { > char *last_slash; > > + if (!GetTempPathA (sizeof (buf), (LPSTR)buf)) Why the cast? Is this really a Bug #15522 change? @@ +2313,5 @@ > _dbus_abort (); > } > > /* Drop terminating backslash or slash */ > + last_slash = strrchr (buf, '\\'); This change is nothing to do with sockets. @@ +2318,3 @@ > if (last_slash > buf && last_slash[1] == '\0') > last_slash[0] = '\0'; > + last_slash = strrchr (buf, '/'); Neither's this. @@ +3292,4 @@ > DWORD pathLength; > char *lastSlash; > SetLastError( 0 ); > + pathLength = GetModuleFileNameA(_dbus_win_get_dll_hmodule(), (LPSTR)prefix, len); Neither's this. @@ +3296,5 @@ > if ( pathLength == 0 || GetLastError() != 0 ) { > *prefix = '\0'; > return FALSE; > } > + lastSlash = strrchr(prefix, '\\'); Or this. Sorry, I see you've already fixed some of my review comments; please disregard where appropriate. Created attachment 114037 [details] [review] Refactor references to socket fd's to use DBusSocket to avoid conversion warnings Comment on attachment 114035 [details] [review] DBusSocket refactoring - common part Review of attachment 114035 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-server-socket.c @@ +51,4 @@ > { > DBusServer base; /**< Parent class members. */ > int n_fds; /**< Number of active file handles */ > + DBusSocket *fds; /**< File descriptor or -1 if disconnected. */ This suggests that -1 is used elsewhere in this file, and, sure enough, it is. The use of -1 should be some platform-dependent invalid value. I think that means we need #ifdef DBUS_WIN typedef SOCKET DBusSocket; # define DBUS_SOCKET_INVALID INVALID_SOCKET #else typedef int DBusSocket; # define DBUS_SOCKET_INVALID -1 #endif or something similar. @@ +187,3 @@ > int saved_errno; > > listen_fd = dbus_watch_get_socket (watch); Don't we now need to introduce DBusSocket _dbus_watch_get_socket (DBusWatch *) so that it can return a SOCKET on Windows, without truncation on 64-bit? (I want to keep DBusSocket out of the public API at least for now - it's hard to get right without breaking our policy of "public headers don't include system headers". I'll have to think about the best way to implement a SOCKET-based public API without mentioning SOCKET. For now, just keep it private.) @@ +295,4 @@ > > socket_server->noncefile = noncefile; > > + socket_server->fds = (DBusSocket *)dbus_new (int, n_fds); The need for a cast should be a clue that this is wrong. On Win64 this is a buffer overflow, because int is 4 bytes and SOCKET = uintptr_t is 8 bytes, so you're giving fds less space than it needs. dbus_new (DBusSocket, n_fds) would be fine. ::: dbus/dbus-sysdeps.h @@ +132,5 @@ > +typedef int DBusSocket; > +#else > +#include <ws2tcpip.h> > +typedef SOCKET DBusSocket; > +#endif Hmm, the need to add that #include isn't ideal - I thought we had it already - but this is a private header so I suppose it's OK. I'd prefer the #include done further up the file with the others. @@ +159,3 @@ > DBusString *buffer, > int count, > + DBusSocket *fds, This array should still be an array of ints, not DBusSockets. Unix fd-passing can pass any sort of fd - file, pipe, socket, whatever - so these are not conceptually sockets. @@ +384,4 @@ > DBusError *error); > > DBUS_PRIVATE_EXPORT > +const char *_dbus_get_tmpdir(void); Why this change? Our coding style in headers is to either line up (arguments all the way over here) so that (multiple functions) line up (I personally dislike that convention, but it's the prevailing style in dbus) or to have one space before the opening parenthesis. ::: dbus/dbus-transport-socket.c @@ +604,4 @@ > if (socket_transport->message_bytes_written <= 0 && DBUS_TRANSPORT_CAN_SEND_UNIX_FD(transport)) > { > /* Send the fds along with the first byte of the message */ > + const DBusSocket *unix_fds; As before, these are not conceptually sockets (even if some of the fds happen to point to a socket really). @@ +821,5 @@ > #ifdef HAVE_UNIX_FD_PASSING > if (DBUS_TRANSPORT_CAN_SEND_UNIX_FD(transport)) > { > + DBusSocket *fds; > + int n_fds; And here. (In reply to Ralf Habacker from comment #2) > DBusSocket refactoring - unix part No comments that I haven't already flagged in the common or Windows part. (In reply to Ralf Habacker from comment #1) > this requires to refactor any access to socket fd's and is much more work. I > would prefer the first one. Yeah, that's a valid concern. Doing the first one now does not rule out doing the second later, and is in fact a step towards it anyway. (In reply to Simon McVittie from comment #6) > Comment on attachment 114034 [details] [review] [review] > DBusSocket refactoring - windows part > > Review of attachment 114034 [details] [review] [review]: > ----------------------------------------------------------------- > > This can't be applied until after the common part is, afaics. > > I'd prefer all the s/int/DBusSocket/ to be one commit really, without any of > the unrelated changes that seem to have crept in - I don't think there's any > need to split it into Windows/Unix/common. > > ::: dbus/dbus-spawn-win.c > @@ +77,5 @@ > > char **envp; > > > > HANDLE child_handle; > > + DBusSocket socket_to_babysitter; /* Connection to the babysitter thread */ > > + DBusSocket socket_to_main; > > These are really sockets? they are used in a call to _dbus_close_socket() > ::: dbus/dbus-sysdeps-win.c > @@ -52,4 @@ > > #include "dbus-credentials.h" > > > > #include <windows.h> > > -#include <ws2tcpip.h> > > Why? the definitions has been moved to dbus-sysdeps.h to define SOCKET > > @@ +645,2 @@ > > dbus_bool_t > > _dbus_socket_is_invalid (int fd) > > This function should take a DBusSocket too, then you wouldn't need... sure, I will add a related patch > > @@ +1054,5 @@ > > * @returns #FALSE on failure (if error is set) > > */ > > dbus_bool_t > > +_dbus_full_duplex_pipe (DBusSocket *fd1, > > + DBusSocket *fd2, > > Not your fault, and not a merge blocker for this change, but the name of > this function is silly (a pipe is a jargon word, and this is not a pipe), > and making this change to it makes that even more obvious. > > Perhaps we should call it _dbus_socketpair() since it has exactly the > semantics "do the same as socketpair() on Unix, or as close to that as you > can achieve on Windows"... I will take a look > > @@ +1706,5 @@ > > tmp = ai; > > while (tmp) > > { > > + DBusSocket fd = INVALID_SOCKET, *newlisten_fd; > > + if ((fd = (DBusSocket)socket (tmp->ai_family, SOCK_STREAM, 0)) == INVALID_SOCKET) > > Why the cast? MSDN says socket() returns a SOCKET already, and you're > changing DBusSocket to be exactly a SOCKET? I will take a look > @@ +2271,4 @@ > > if (!_dbus_string_lengthen (str, n_bytes)) > > return FALSE; > > > > + p = (BYTE*)_dbus_string_get_data_len (str, old_len, n_bytes); > > This seems to be nothing to do with sockets? yes, sorry Got a warning:
/home/ralf/src/dbus-2/dbus/dbus-sysdeps-win.c: In function '_dbus_write_socket_two':
/home/ralf/src/dbus-2/dbus/dbus-sysdeps-win.c:639:21: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
if (bytes_written < 0 && errno == EINTR)
^
(In reply to Simon McVittie from comment #9) > @@ +187,3 @@ > > int saved_errno; > > > > listen_fd = dbus_watch_get_socket (watch); > > Don't we now need to introduce DBusSocket _dbus_watch_get_socket (DBusWatch > *) so that it can return a SOCKET on Windows, without truncation on 64-bit? > > (I want to keep DBusSocket out of the public API at least for now - it's > hard to get right without breaking our policy of "public headers don't > include system headers". I'll have to think about the best way to implement > a SOCKET-based public API without mentioning SOCKET. For now, just keep it > private.) > Adding a private function ? DBUS_PRIVATE_EXPORT DBusSocket _dbus_watch_get_socket (DBusWatch *watch); Created attachment 114041 [details] [review] Use typedef DBusSocket for sockets fd's to avoid conversion warnings (In reply to Simon McVittie from comment #9) > Comment on attachment 114035 [details] [review] [review] > DBusSocket refactoring - common part > > Review of attachment 114035 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: dbus/dbus-server-socket.c > @@ +51,4 @@ > > { > > DBusServer base; /**< Parent class members. */ > > int n_fds; /**< Number of active file handles */ > > + DBusSocket *fds; /**< File descriptor or -1 if disconnected. */ > > This suggests that -1 is used elsewhere in this file, and, sure enough, it > is. The use of -1 should be some platform-dependent invalid value. > > I think that means we need > > #ifdef DBUS_WIN > typedef SOCKET DBusSocket; > # define DBUS_SOCKET_INVALID INVALID_SOCKET > #else > typedef int DBusSocket; > # define DBUS_SOCKET_INVALID -1 > #endif > > or something similar. fixed with attachment 114041 [details] [review] > @@ +187,3 @@ > > int saved_errno; > > > > listen_fd = dbus_watch_get_socket (watch); > > Don't we now need to introduce DBusSocket _dbus_watch_get_socket (DBusWatch > *) so that it can return a SOCKET on Windows, without truncation on 64-bit? > > (I want to keep DBusSocket out of the public API at least for now - it's > hard to get right without breaking our policy of "public headers don't > include system headers". I'll have to think about the best way to implement > a SOCKET-based public API without mentioning SOCKET. For now, just keep it > private.) fixed with attachment 114041 [details] [review] > @@ +295,4 @@ > > > > socket_server->noncefile = noncefile; > > > > + socket_server->fds = (DBusSocket *)dbus_new (int, n_fds); > > The need for a cast should be a clue that this is wrong. > > On Win64 this is a buffer overflow, because int is 4 bytes and SOCKET = > uintptr_t is 8 bytes, so you're giving fds less space than it needs. > > dbus_new (DBusSocket, n_fds) would be fine. fixed with attachment 114041 [details] [review] > ::: dbus/dbus-sysdeps.h > @@ +132,5 @@ > > +typedef int DBusSocket; > > +#else > > +#include <ws2tcpip.h> > > +typedef SOCKET DBusSocket; > > +#endif > > Hmm, the need to add that #include isn't ideal - I thought we had it already > - but this is a private header so I suppose it's OK. > > I'd prefer the #include done further up the file with the others. fixed with attachment 114041 [details] [review] > @@ +159,3 @@ > > DBusString *buffer, > > int count, > > + DBusSocket *fds, > > This array should still be an array of ints, not DBusSockets. Unix > fd-passing can pass any sort of fd - file, pipe, socket, whatever - so these > are not conceptually sockets. fixed with attachment 114041 [details] [review] > ::: dbus/dbus-transport-socket.c > @@ +604,4 @@ > > if (socket_transport->message_bytes_written <= 0 && DBUS_TRANSPORT_CAN_SEND_UNIX_FD(transport)) > > { > > /* Send the fds along with the first byte of the message */ > > + const DBusSocket *unix_fds; > > As before, these are not conceptually sockets (even if some of the fds > happen to point to a socket really). > still open > @@ +821,5 @@ > > #ifdef HAVE_UNIX_FD_PASSING > > if (DBUS_TRANSPORT_CAN_SEND_UNIX_FD(transport)) > > { > > + DBusSocket *fds; > > + int n_fds; > > And here. still open Created attachment 114042 [details] [review] Revert partially DBusSocket refactoring, it is not appliable (to be merged into the main patch) Created attachment 114069 [details] [review] Rename _dbus_full_duplex_pipe() to more descriptive name _dbus_socketpair() Created attachment 114070 [details] [review] Complete rename to _dbus_socketpair() (this patch is going to be merged into the related base patch) I'll review these soon, but it'll take a little while to check that all the uses of -1 vs. INVALID_SOCKET are appropriate, so it'll have to wait til I have time to go through it all. If you want to squash the patches into fewer before I review, so that the patches are in the intended order and each one is individually correct, that would be great; or if you don't, I'll do that myself before reviewing, to avoid complaining about things that you actually fixed in a later patch :-) Created attachment 114092 [details] [review] Rename _dbus_full_duplex_pipe() to more descriptive name _dbus_socketpair(). Created attachment 114093 [details] [review] Use typedef DBusSocket for sockets fd's to avoid conversion warnings I splitted the reviewed stuff into 1. simple rename of _dbus_full_duplex_pipe() to _dbus_socketpair () 2. Introduce typedef DBusSocket. Thanks, I'll come back to this next week. Created attachment 114132 [details] [review] Use typedef DBusSocket for sockets fd's to avoid conversion warnings (update) - removed unrelated rename to DBusSocket (has been already rejected) Comment on attachment 114132 [details] [review] Use typedef DBusSocket for sockets fd's to avoid conversion warnings (update) Review of attachment 114132 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-connection.h @@ +31,4 @@ > #include <dbus/dbus-memory.h> > #include <dbus/dbus-message.h> > #include <dbus/dbus-shared.h> > +#include <dbus/dbus-sysdeps.h> This is not valid. Public, installed headers like dbus-connection.h cannot include internal, non-installed headers like dbus-sysdeps.h. Removing this line seems to be OK as a fix - nothing seems to be relying on adding this. Perhaps it was relied on in an earlier version of your patches. ::: dbus/dbus-mainloop.c @@ +333,2 @@ > > watches = _dbus_hash_table_lookup_int (loop->watches, fd); This is implicitly converting from SOCKET to (signed) int. There's lots of other code in this file doing the same, unfortunately... this patch is a step in the right direction, but we would still corrupt any SOCKET whose value is not a fairly small positive integer. ::: dbus/dbus-sysdeps-win.c @@ +1508,4 @@ > const char *noncefile, > DBusError *error) > { > + SOCKET fd = DBUS_SOCKET_INVALID, res; res isn't a SOCKET - getattr still returns int. ::: dbus/dbus-watch.c @@ +592,5 @@ > > +DBusSocket > +_dbus_watch_get_socket (DBusWatch *watch) > +{ > + _dbus_return_val_if_fail (watch != NULL, -1); Calling _dbus_return_val_if_fail() from a function whose name starts with "_" triggers an assertion failure; for internal functions you're meant to use _dbus_assert() instead. (Did you test this? Running the tests finds this immediately.) (In reply to Ralf Habacker from comment #24) > Use typedef DBusSocket for sockets fd's to avoid conversion warnings (update) This also forgets to change one of the versions of _dbus_socketpair() to take DBusSocket * parameters. (In reply to Simon McVittie from comment #20) > I'll review these soon, but it'll take a little while to check that all the > uses of -1 vs. INVALID_SOCKET are appropriate, so it'll have to wait til I > have time to go through it all. The short version is, they're not. I don't think we're going to get dbus to have actually correct socket handling unless/until we do this: (In reply to Simon McVittie from comment #0) > typedef struct { > #ifdef DBUS_WIN > SOCKET windows_socket; > #else > int unix_fd; > #endif > } DBusSocket; > > and pass/return it by value My work-in-progress patch for that is +480 -344 lines, which is not actually as bad as I had feared. (In reply to Simon McVittie from comment #25) > Comment on attachment 114132 [details] [review] [review] > Use typedef DBusSocket for sockets fd's to avoid conversion warnings (update) > > Review of attachment 114132 [details] [review] [review]: > ----------------------------------------------------------------- > > > +#include <dbus/dbus-sysdeps.h> > Removing this line seems to be OK as a fix - nothing seems to be relying on > adding this. Perhaps it was relied on in an earlier version of your patches. yes I can confirmn > res isn't a SOCKET - getattr still returns int. sure > > ::: dbus/dbus-watch.c > @@ +592,5 @@ > > > > +DBusSocket > > +_dbus_watch_get_socket (DBusWatch *watch) > > +{ > > + _dbus_return_val_if_fail (watch != NULL, -1); > > Calling _dbus_return_val_if_fail() from a function whose name starts with > "_" triggers an assertion failure; for internal functions you're meant to > use _dbus_assert() instead. > > (Did you test this? Running the tests finds this immediately.) I compiled dbus with the patches from this bug with autotools and did run make check make[3]: Entering directory `/home/ralf.habacker/src/dbus-master-autotools-build/test' make[4]: Entering directory `/home/ralf.habacker/src/dbus-master-autotools-build/test' PASS: test-shell PASS: test-printf PASS: test-corrupt PASS: test-dbus-daemon PASS: test-dbus-daemon-eavesdrop PASS: test-fdpass PASS: test-monitor PASS: test-loopback PASS: test-marshal PASS: test-refs PASS: test-relay PASS: test-sd-activation PASS: test-syntax PASS: test-syslog PASS: test-uid-permissions ============================================================================ Testsuite summary for dbus 1.9.15 ============================================================================ # TOTAL: 15 # PASS: 15 (In reply to Ralf Habacker from comment #28) > I compiled dbus with the patches from this bug with autotools and did run > make check Ah, I see what has happened. You didn't configure with --enable-embedded-tests (or --enable-developer), so you only got a subset of the tests, which happens to not have any calls to that function. (In reply to Simon McVittie from comment #27) > I don't think we're going to get dbus to have actually correct socket > handling unless/until we do this: > > (In reply to Simon McVittie from comment #0) > > typedef struct { > > #ifdef DBUS_WIN > > SOCKET windows_socket; > > #else > > int unix_fd; > > #endif > > } DBusSocket; > > > > and pass/return it by value > > My work-in-progress patch for that is +480 -344 lines, which is not actually > as bad as I had feared. It's a bit more complicated than that because we need a representation for "thing we can poll", which on Unix means any fd, but on Windows specifically means a SOCKET; and on Unix, for tedious reasons involving abuse of struct pollfd, we need the representation of "thing we can poll" to be exactly "int". I have a mostly working branch with three layers of type: DBusSocket: struct containing either SOCKET or int (a struct so that the compiler won't let us assign to/from int) - used in non-OS-specific socket APIs and high(ish)-level OS-specifics DBusPollable: on Windows, DBusSocket (not SOCKET so that the compiler will stop us doing anything incorrect); on Unix, int - used in DBusWatch, DBusSocketSet, DBusLoop Native type: SOCKET or int as appropriate - used in low-level OS-specific socket APIs Work in progress, might not work perfectly yet: http://cgit.freedesktop.org/~smcv/dbus/log/?h=wip-dbussocket (In reply to Simon McVittie from comment #31) > Work in progress, might not work perfectly yet: > > http://cgit.freedesktop.org/~smcv/dbus/log/?h=wip-dbussocket I did run cmake on linux cmake -DDBUS_ENABLE_EMBEDDED_TESTS=1 ../dbus-smcv/cmake/ and got x@y:~/src/dbus-smcv-build> make [ 0%] Building C object dbus/CMakeFiles/dbus-1.dir/home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.o /home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c: In function ‘_dbus_write_socket_two’: /home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c:605:27: error: incompatible type for argument 1 of ‘_dbus_write_two’ buffer2, start2, len2); ^ In file included from /home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c:29:0: /home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.h:61:1: note: expected ‘int’ but argument is of type ‘DBusSocket’ _dbus_write_two (int fd, ^ /home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c: In function ‘add_linux_security_label_to_credentials’: /home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c:1693:30: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] if (e != ERANGE || len <= _dbus_string_get_length_uint (&buf)) ^ /home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c:1718:11: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] if (len > _dbus_string_get_length_uint (&buf)) ^ /home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c:1740:51: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] if (strlen (_dbus_string_get_const_data (&buf)) != len) ^ make[2]: *** [dbus/CMakeFiles/dbus-1.dir/home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.o] Fehler 1 make[1]: *** [dbus/CMakeFiles/dbus-1.dir/all] Fehler 2 (In reply to Ralf Habacker from comment #32) > x@y:~/src/dbus-smcv-build> make > [ 0%] Building C object > dbus/CMakeFiles/dbus-1.dir/home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.o > /home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c: In function > ‘_dbus_write_socket_two’: > /home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c:605:27: error: incompatible > type for argument 1 of ‘_dbus_write_two’ > buffer2, start2, len2); Hmm. Did you apply the whole branch? I could build it using cmake 3.0 on Debian, targeting either Linux (native gcc) or Windows (mingw), without these errors. I got some compiler warnings on mingw at intermediate points along the branch, because the fundamental issue here is that we're doing various implied conversions between SOCKET and int, and gcc will warn if they are like this: if (my_SOCKET == my_int) but not if they're like this: my_function_that_takes_an_int (my_SOCKET) and in any case, those warnings go away after the whole branch has been applied. > /home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c: In function > ‘add_linux_security_label_to_credentials’: > /home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c:1693:30: warning: > comparison between signed and unsigned integer expressions [-Wsign-compare] > if (e != ERANGE || len <= _dbus_string_get_length_uint (&buf)) > ^ I think this indicates that your build thinks socklen_t is signed? (It isn't - or it's uint32 on x86-64 Linux, at least. See <bits/types.h>.) (In reply to Simon McVittie from comment #31) > Work in progress, might not work perfectly yet: > > http://cgit.freedesktop.org/~smcv/dbus/log/?h=wip-dbussocket I mentioned above and on Bug #89450 that this is kinda unfinished. I'm quite busy with non-D-Bus right now, but will try to come back to it (and in particular, work out why you get those compiler errors but I don't). "Rename _dbus_full_duplex_pipe() to more descriptive name _dbus_socketpair()" is good to apply to master whenever, as far as I'm concerned. "Fix assorted compiler warnings on Windows" and "cmake: only set CMP0053, CMP0054 on CMake >= 3.1" are not really anything to do with this bug, but the procedure I was using to test this branch was to build all of it in all my standard test configurations (I'm up to 12 now, including Autotools/native gcc with various configure options, Autotools/mingw gcc, CMake/native gcc and CMake/mingw gcc) and the warnings were getting in the way of seeing whether there was anything bad from this branch. "Fix assorted compiler warnings on Windows" should ideally be several patches but I haven't got round to splitting it yet. The rest of the branch shouldn't really be applied until all of it is, I don't think. At the beginning of the branch we're at some sort of stable state: we consistently (albeit incorrectly) treat sockets as if they were int. We know that this is wrong, but we also know that, in practice, it works OK. At the end of the branch, we've reached another stable state: we consistently treat sockets as SOCKET, and the compiler enforces this for us by not allowing naive assignment between DBusSocket and int. At all intermediate points (including after my modified version of your compiler-warning-squashing patch), there are still incorrect implicit conversions between int and SOCKET, and they're migrating around the source tree as I work through the changes. Sometimes they happen to be something that gcc warns about, sometimes they are something that it silently accepts - but they are there (and you can make that obvious by changing DBusSocket to not be assignment-compatible with int, as I did). I do think this is a net improvement; you'll notice the last commit removes this comment * @todo Use for the file descriptors a struct * - struct DBusSocket{ int d; }; - * instead of int to get type-safety which * will be checked by the compiler. which has been there since 2006 (even though back then, Havoc probably didn't know that the underlying type for SOCKET was uintptr_t). (In reply to Simon McVittie from comment #33) > Hmm. Did you apply the whole branch? To be clear, commit 84cb0ab7 is the one that I think should work. (In reply to Ralf Habacker from comment #32) > x@y:~/src/dbus-smcv-build> make > [ 0%] Building C object > dbus/CMakeFiles/dbus-1.dir/home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.o > /home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c: In function > ‘_dbus_write_socket_two’: > /home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c:605:27: error: incompatible > type for argument 1 of ‘_dbus_write_two’ > buffer2, start2, len2); > ^ OK, found it. The bug is that the backwards-compatible code path for when you don't have MSG_NOSIGNAL was wrong, and the cmake build doesn't detect that yet. Commit 3690335 (which is meant to be squashed into the previous one) fixes that. We might get a few more compilation failures similar to that on *BSD or old Linux, which I haven't tested; but there shouldn't be too many, and they should all be in platform-specific code where it's trivial to fix (fd -> fd.fd or similar). > /home/xxx/src/dbus-smcv/dbus/dbus-sysdeps-unix.c:1740:51: warning: > comparison between signed and unsigned integer expressions [-Wsign-compare] > if (strlen (_dbus_string_get_const_data (&buf)) != len) > ^ I'm not sure why you get this: you added the SOCKLEN_T check recently, so cmake should now know that socklen_t exists? Comment on attachment 114092 [details] [review] Rename _dbus_full_duplex_pipe() to more descriptive name _dbus_socketpair(). applied to master from the smcv/wip-dbussocket branch Created attachment 114755 [details] [review] [01/11] Use typedef DBusSocket for sockets fd's to avoid conversion warnings From: Ralf Habacker <ralf.habacker@freenet.de> [smcv: remove unneeded and invalid dbus-sysdeps.h from public header; make prototype of _dbus_socketpair() consistent; undo conversion of getaddrinfo result from int to SOCKET; don't call _dbus_return_val_if_fail() from internal function] Created attachment 114756 [details] [review] [02/11] bus_unix_fds_passing_test: the results of _dbus_socketpair are sockets Created attachment 114757 [details] [review] [03/11] dbus-sysdeps: add more infrastructure around DBusSocket This is all trivial right now, but will become significant when we change DBusSocket into a type-safe struct. --- See patch 11 for what this is going to turn into. Created attachment 114758 [details] [review] [04/11] Mostly remove the remnants of an older socket abstraction layer This is only used on Windows, and wasn't even a particularly abstract abstraction. I've removed DBUS_SOCKET_IS_INVALID in favour of DBUS_SOCKET_IS_VALID because I prefer to avoid double-negatives. Created attachment 114759 [details] [review] [05/11] DBusMainLoop, DBusSocketSet: work in terms of DBusPollable This requires generic support for keying hash tables by DBusPollable: there are already implementations for int and uintptr_t keys, but not for "int or uintptr_t depending on platform", which is what DBusPollable now means. Created attachment 114760 [details] [review] [06/11] main: reload_pipe is (despite its name) a socket pair --- One day I might rename it to reload_socket, or make it actually a pipe(), since it's entirely Unix-specific... but for now, RESOLVED LATER. Created attachment 114761 [details] [review] [07/11] Split _dbus_set_fd_nonblocking vs. _dbus_set_socket_nonblocking The former is Unix-specific, the latter is also portable to Windows. On Unix, they're really the same thing. Created attachment 114762 [details] [review] [08/11] generic socket transport code: work in terms of DBusSocket Created attachment 114763 [details] [review] [09/11] Convert miscellaneous socket APIs to DBusSocket Created attachment 114764 [details] [review] [10/11] Remove _dbus_socket_is_invalid, no longer used It didn't have many users anyway, and I've replaced them with the DBUS_SOCKET_IS_VALID macro. --- Again, I reversed the sense because I don't not dislike double-negatives. :-) Created attachment 114765 [details] [review] [11/11] Turn DBusSocket into a type-safe struct, preventing inappropriate conversion Fix the remaining platform-specific code to look at the struct's appropriate platform-specific member. --- This might cause minor and easily-fixed compilation failures on non-Linux, non-Windows platforms, if I've missed any uses of the raw fd... but I think on balance it's worth it anyway. If nothing else, that'll tell us who is testing D-Bus on their Unix platforms :-) On OS X, it compiles and links without errors if --disable-developer. With --enable-developer, the -Werror warnings make the build totally impossible, though they don't look specific to this branch. Three tests failed with the error: Failed to start message bus: launchd's environment variable DBUS_LAUNCHD_SESSION_BUS_SOCKET is empty, but should contain a socket path. I don't know if it's caused by this. On FreeBSD, I get the same warnings (clang with -Wcast-align). The three tests that failed with OS X do pass on FreeBSD. But test-fdpass (which was skipped on OS X) fails on FreeBSD: /relay: ..**********..OK /limit: ..**********..OK /too-many/plus1: ..**********..OK /too-many/plus2: ..**********..OK /too-many/plus17: ..**********..OK /too-many/split: ..**********..OK /flood/1: ..**********...OK /flood/half-limit: ..**********....OK /flood/over-half-limit: ..**********.....OK /flood/limit: ..**********......OK /odd-limit/minus1: ..**********..OK /odd-limit/at: ..**********...** ERROR:fdpass.c:742:test_odd_limit: assertion failed: (dbus_message_contains_unix_fds (incoming)) FAIL test-fdpass (exit status: 134) Testing master branch: exact same results, so none of the failures appear to be introduced by this branch. (In reply to Thiago Macieira from comment #50) > Testing master branch: exact same results, so none of the failures appear to > be introduced by this branch. Please open separate bugs for those. Thanks for testing! I would expect failures caused by this branch to be build failures, either from -Werror or actual hard failures. Created attachment 114788 [details] [review] [01/11] Use typedef DBusSocket for sockets fd's to avoid conversion warnings (rebased) Comment on attachment 114756 [details] [review] [02/11] bus_unix_fds_passing_test: the results of _dbus_socketpair are sockets Review of attachment 114756 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 114757 [details] [review] [03/11] dbus-sysdeps: add more infrastructure around DBusSocket Review of attachment 114757 [details] [review]: ----------------------------------------------------------------- looks good (In reply to Simon McVittie from comment #51) > (In reply to Thiago Macieira from comment #50) > > Testing master branch: exact same results, so none of the failures appear to > > be introduced by this branch. > > Please open separate bugs for those. Thanks for testing! > > I would expect failures caused by this branch to be build failures, either > from -Werror or actual hard failures. I had to compile without -Werror otherwise it wouldn't get past dbus-marshal.c, as clang is apparently stricter when casting from char* to higher-ranked pointer types. So it's possible that this patchset is introducing new warnings, but I doubt it. Almost everything I saw was either -Wcast-align or one unused function warning. But no hard compilation failures and no new test errors. I will report the OS X and FreeBSD errors. Comment on attachment 114758 [details] [review] [04/11] Mostly remove the remnants of an older socket abstraction layer Review of attachment 114758 [details] [review]: ----------------------------------------------------------------- looks good (In reply to Ralf Habacker from comment #52) > Created attachment 114788 [details] [review] > [01/11] Use typedef DBusSocket for sockets fd's to avoid conversion warnings > (rebased) What did you change? (In reply to Simon McVittie from comment #57) > (In reply to Ralf Habacker from comment #52) > > Created attachment 114788 [details] [review] [review] > > [01/11] Use typedef DBusSocket for sockets fd's to avoid conversion warnings > > (rebased) > > What did you change? The previous commit contained unrelated doc change revert from the following commit. commit 73af0d5d5c953d157e89f6c02d61872a0c490edf Date: Fri Mar 6 08:09:57 2015 +0100 Rename _dbus_full_duplex_pipe() to more descriptive name _dbus_socketpair(). (In reply to Ralf Habacker from comment #58) > The previous commit contained unrelated doc change revert from the following > commit. > > commit 73af0d5d5c953d157e89f6c02d61872a0c490edf > Date: Fri Mar 6 08:09:57 2015 +0100 > > Rename _dbus_full_duplex_pipe() to more descriptive name > _dbus_socketpair(). Good point. Your version looks better, and is ready to apply when we are happy with the rest of my branch (but we should still not apply it until the rest of the branch is ready, otherwise we're just moving the problem around). Any further thoughts on this? AFAICS, you've replaced patch 1, approved 2-4, and 5+ still need review. Your replacement version of patch 1 looks good, but please don't merge it until we are ready to merge the whole series: until we get to patch 11, we aren't really enforcing the distinction between int and DBusSocket, so the problem is just moving around the codebase. We know that the current state works in practice (even though it's wrong in theory), and I'm pretty confident that after patch 11 we're in good shape (because the compiler stops us from getting it wrong), but the intermediate states are less good. Comment on attachment 114760 [details] [review] [06/11] main: reload_pipe is (despite its name) a socket pair Review of attachment 114760 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 114761 [details] [review] [07/11] Split _dbus_set_fd_nonblocking vs. _dbus_set_socket_nonblocking Review of attachment 114761 [details] [review]: ----------------------------------------------------------------- looks good otherwise ::: dbus/dbus-sysdeps.h @@ +228,5 @@ > DBusError *error); > DBusSocket _dbus_accept (DBusSocket listen_fd); > > +dbus_bool_t _dbus_set_socket_nonblocking (DBusSocket fd, > + DBusError *error); this is a duplicate of the definition in line 167 Comment on attachment 114762 [details] [review] [08/11] generic socket transport code: work in terms of DBusSocket Review of attachment 114762 [details] [review]: ----------------------------------------------------------------- looks good ::: dbus/dbus-connection.c @@ +5149,1 @@ > int *fd) I assume 'int' is used because of locked public api. Comment on attachment 114763 [details] [review] [09/11] Convert miscellaneous socket APIs to DBusSocket Review of attachment 114763 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 114764 [details] [review] [10/11] Remove _dbus_socket_is_invalid, no longer used Review of attachment 114764 [details] [review]: ----------------------------------------------------------------- looks good Created attachment 115135 [details] [review] Convert mostly DBUS_SOCKET_... and DBUS_POLLABLE_.. macros to inline functions for more type safety. Comment on attachment 114759 [details] [review] [05/11] DBusMainLoop, DBusSocketSet: work in terms of DBusPollable Review of attachment 114759 [details] [review]: ----------------------------------------------------------------- a few api usage issues, otherwise looks good. ::: dbus/dbus-watch.c @@ +317,2 @@ > watch_flags_to_string (dbus_watch_get_flags (link->data)), > + DBUS_POLLABLE_PRINTABLE (watch->fd)); not better DBUS_POLLABLE_PRINTABLE (dbus_watch_get_pollable (link->data)) ? @@ +333,3 @@ > > + _dbus_verbose ("Removing watch on fd %" DBUS_POLLABLE_FORMAT " using newly-set remove function because initial add failed\n", > + DBUS_POLLABLE_PRINTABLE (watch2->fd)); not better DBUS_POLLABLE_PRINTABLE (dbus_watch_get_pollable (link2->data)) ? @@ +389,5 @@ > > if (watch_list->add_watch_function != NULL) > { > + _dbus_verbose ("Adding watch on fd %" DBUS_POLLABLE_FORMAT "\n", > + DBUS_POLLABLE_PRINTABLE (watch->fd)); dito @@ +420,5 @@ > > if (watch_list->remove_watch_function != NULL) > { > + _dbus_verbose ("Removing watch on fd %" DBUS_POLLABLE_FORMAT "\n", > + DBUS_POLLABLE_PRINTABLE (watch->fd)); dito @@ +453,5 @@ > if (watch_list->watch_toggled_function != NULL) > { > + _dbus_verbose ("Toggling watch %p on fd %" DBUS_POLLABLE_FORMAT " to %d\n", > + watch, > + DBUS_POLLABLE_PRINTABLE (watch->fd), dito @@ +669,5 @@ > { > _dbus_return_if_fail (watch != NULL); > > + _dbus_verbose ("Setting watch fd %" DBUS_POLLABLE_FORMAT " data to data = %p function = %p from data = %p function = %p\n", > + DBUS_POLLABLE_PRINTABLE (watch->fd), dito @@ +738,5 @@ > > if (flags == 0) > { > + _dbus_verbose ("After sanitization, watch flags on fd %" DBUS_POLLABLE_FORMAT " were 0\n", > + DBUS_POLLABLE_PRINTABLE (watch->fd)); dito Comment on attachment 114757 [details] [review] [03/11] dbus-sysdeps: add more infrastructure around DBusSocket Review of attachment 114757 [details] [review]: ----------------------------------------------------------------- found platform related code ordering issue afterwards ::: dbus/dbus-sysdeps.h @@ +349,4 @@ > #define _DBUS_POLLNVAL 0x0020 > #endif > > +#ifdef DBUS_WIN should be #ifndef DBUS_WIN first similiar to line 135 Comment on attachment 115135 [details] [review] Convert mostly DBUS_SOCKET_... and DBUS_POLLABLE_.. macros to inline functions for more type safety. Review of attachment 115135 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps.h @@ +370,4 @@ > # define DBUS_POLLABLE_FORMAT "Iu" > + > +static inline DBusPollable > +_dbus_socket_get_pollable(DBusSocket s) { return s; } self review: space after function name missing (In reply to Ralf Habacker from comment #68) > should be #ifndef DBUS_WIN first similiar to line 135 I think I see your point - are you saying that functionally, the code is fine, but stylistically, each of several conditional blocks should be in a consistent order? - but I disagree about the correct resolution. For the existing #ifdef, I kept the "#ifndef DBUS_WIN" first because it was already like that, but as a general principle I prefer to reduce the number of negatives - I think #ifdef...#else is much less confusing than #ifndef...#else. In all the places where it matters in dbus, the progression is fairly clearly "Windows is the special case, Unix is normal" - DBUS_UNIX covers Linux, Mac, *BSD, etc., and we do not currently support any non-Windows non-Unix platforms. So I prefer to have #ifdef DBUS_WIN printf ("Windows\n"); #else printf ("not Windows\n"); #endif rather than #ifndef. If we do any re-ordering here, I would prefer to turn > #ifndef DBUS_WIN > typedef int DBusSocket; > /* etc. */ > #else /* DBUS_WIN */ > typedef SOCKET DBusSocket; > /* etc. */ > #endif into > #ifdef DBUS_WIN > typedef SOCKET DBusSocket; > /* etc. */ > #else /* not DBUS_WIN */ > typedef int DBusSocket; > /* etc. */ > #endif Comment on attachment 115135 [details] [review] Convert mostly DBUS_SOCKET_... and DBUS_POLLABLE_.. macros to inline functions for more type safety. Review of attachment 115135 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps.h @@ +132,4 @@ > # define DBUS_SOCKET_INIT { -1 } > + > +static inline int > +_dbus_socket_printable (DBusSocket s) { return s.fd; } This is a pretty small improvement, which is why I didn't do it like this myself: if you mistakenly pass an int to _dbus_socket_printable(), or whatever, the compiler is already going to stop you. The only extra type-safety this gives you is to stop you from using some other struct that happens to have a member named fd. Having said that, sure, it's still an improvement. We can add this to the branch. (I'll test it on my collection of build configurations before merging.) @@ +134,5 @@ > +static inline int > +_dbus_socket_printable (DBusSocket s) { return s.fd; } > + > +static inline dbus_bool_t > +_dbus_socket_is_valid (DBusSocket s) { return s.fd >= 0; } I prefer to put logical conditions in parentheses when they're used in an expression, to make the interpretation obvious: return (s.fd >= 0) but for inline functions this small, it isn't important. Comment on attachment 114759 [details] [review] [05/11] DBusMainLoop, DBusSocketSet: work in terms of DBusPollable Review of attachment 114759 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-watch.c @@ +317,2 @@ > watch_flags_to_string (dbus_watch_get_flags (link->data)), > + DBUS_POLLABLE_PRINTABLE (watch->fd)); This is within watch.c, at the same level of abstraction as _dbus_watch_get_pollable() itself, so I don't think there's any value in that indirection. At worst, it costs us an external function call; at best, it costs nothing (if the compiler works out that it can be inlined[1]), but is longer and doesn't provide any benefit. I suspect that the major reason for using dbus_watch_get_flags() here was that whoever added it didn't want to declare "DBusWatch *watch = link->data" just so that they could say watch->flags. dbus_watch_get_flags() also has an extra assertion, so I suppose checking that assertion might have some small amount of value. [1] on Linux and other ELF platforms, calls to extern functions can never be inlined unless you use nonstandard linker options (-Bsymbolic), because of support for the rarely-used "function interposition" feature. I've occasionally wondered whether to set -Bsymbolic for dbus. (In reply to Simon McVittie from comment #70) > (In reply to Ralf Habacker from comment #68) > > should be #ifndef DBUS_WIN first similiar to line 135 > > I think I see your point - are you saying that functionally, the code is > fine, but stylistically, each of several conditional blocks should be in a > consistent order? you got it (In reply to Ralf Habacker from comment #62) > this is a duplicate of the definition in line 167 Good point. Testing a fixed version (In reply to Ralf Habacker from comment #63) > ::: dbus/dbus-connection.c > @@ +5149,1 @@ > > int *fd) > > I assume 'int' is used because of locked public api. Yes. (In reply to Simon McVittie from comment #70) > If we do any re-ordering here, I would prefer to turn > > > #ifndef DBUS_WIN > > typedef int DBusSocket; > > /* etc. */ > > into > > > #ifdef DBUS_WIN > > typedef SOCKET DBusSocket; > > /* etc. */ Added a patch that does this; testing now. Created attachment 115157 [details] [review] [fixup for 07/11] remove duplicate declaration of _dbus_set_socket_nonblocking Created attachment 115158 [details] [review] [fixup for Attachment #115135 [details]] style: space before parenthesis in declarations and definitions --- I've assumed Attachment #115135 [details] is meant to apply at the end of my patch series. Created attachment 115159 [details] [review] [13] DBusSocket: put the #ifdef case before the !defined case This avoids the confusing #ifndef...#else anti-pattern. --- To be applied after Attachment #115135 [details] and Attachment #115158 [details] (if applied out-of-order, they would conflict). Probably obvious, but the ones labelled "fixup" are intended to be squashed into the previous one in the patch series. This one is not; it is its own commit. (In reply to Ralf Habacker from comment #67) > not better DBUS_POLLABLE_PRINTABLE (dbus_watch_get_pollable (link->data)) ? I have not done anything about this: I hope my response above, explaining why not, is enough to address it. If you think I'm wrong, we can discuss further. http://cgit.freedesktop.org/~smcv/dbus/log/?h=dbussocket if you'd prefer to review like that. Comment on attachment 114765 [details] [review] [11/11] Turn DBusSocket into a type-safe struct, preventing inappropriate conversion Review of attachment 114765 [details] [review]: ----------------------------------------------------------------- Reviewing in platform related files is easy, in shared code I had to inspect the source code from the dbussocket branch to see for which platform the related changes is targeted. looks good. ::: bus/dispatch.c @@ +5144,2 @@ > _dbus_assert_not_reached("Failed to read value from pipe."); > this is in bus_unix_fds_passing_test() and therefore unix only: okay ::: bus/main.c @@ +70,4 @@ > char action[2] = { ACTION_RELOAD, '\0' }; > > _dbus_string_init_const (&str, action); > + if ((reload_pipe[RELOAD_WRITE_END].fd > 0) && this is in signal_handler, which is unix only: okay @@ +103,4 @@ > DBusString str; > char action[2] = { ACTION_QUIT, '\0' }; > _dbus_string_init_const (&str, action); > + if ((reload_pipe[RELOAD_WRITE_END].fd < 0) || dito @@ +248,4 @@ > while (!_dbus_string_init (&str)) > _dbus_wait_for_memory (); > > + if ((reload_pipe[RELOAD_READ_END].fd > 0) && unix only: okay ::: dbus/dbus-watch.c @@ +613,5 @@ > + s.fd = watch->fd; > +#else > + s = watch->fd; > +#endif > + looks ugly, but is caused by different implementations of DBusPollable (unix:int, windows:DBusSocket) Having unix variant of DBusPollable also derived from DBusSocket and adding some _dbus_pollable_... functions would have the benefit of a common api and would also reduce the required number of #ifdef DBUS_UNIX ... #else ... #endif. ::: test/fdpass.c @@ +540,4 @@ > /* This is blatant cheating, and the API documentation specifically > * tells you not use this function in this way. Never do this > * in application code. */ > + if (!dbus_connection_get_socket (f->left_client_conn, unix only: looks good Comment on attachment 115159 [details] [review] [13] DBusSocket: put the #ifdef case before the !defined case Review of attachment 115159 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 115157 [details] [review] [fixup for 07/11] remove duplicate declaration of _dbus_set_socket_nonblocking Review of attachment 115157 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 115158 [details] [review] [fixup for Attachment #115135 [details]] style: space before parenthesis in declarations and definitions Review of attachment 115158 [details] [review]: ----------------------------------------------------------------- looks good (In reply to Ralf Habacker from comment #80) > ::: dbus/dbus-watch.c > @@ +613,5 @@ > > + s.fd = watch->fd; > > +#else > > + s = watch->fd; > > +#endif > > + > > looks ugly, but is caused by different implementations of DBusPollable > (unix:int, windows:DBusSocket) > Having unix variant of DBusPollable also derived from DBusSocket and adding > some _dbus_pollable_... functions would have the benefit of a common api and > would also reduce the required number of #ifdef DBUS_UNIX ... #else ... > #endif. It would be incorrect for the Unix DBusPollable to be derived from DBusSocket: on Windows, select() (and hence _dbus_poll()) only works on SOCKETs, but on Unix, any fd is (at least potentially) pollable. In particular, on Unix you can select() on a pipe, but on Windows you can't. We rely on this in the Unix-specific files dbus-spawn.c, where we poll a pipe, and bus/dir-watch-{inotify,kqueue}.c, where we poll the appropriate OS-specific fd. Also, as I said in the commit message, DBusPollable needs to be just int, not a struct, because the way the Unix _dbus_poll() implementation works is that we need to make sure that DBusPollFD has the same in-memory layout as native struct pollfd. On Windows, there's no poll() to be compatible with, so we're free to put whatever we want in the DBusPollFD struct; I chose to use a DBusSocket. So these particular #ifdefs are not harmful, IMO: they are there because Unix and Windows really do work differently in this respect. Similarly, the stuff in dbus-hash.h really does need to distinguish between the two platforms, because the thing we're using as a hash key has a different size (on 64-bit). As a future change, we could consider refactoring polling so the Windows code can stop pretending to have a poll() implementation: * add a Windows-specific dbus-socket-set-winsock.c which implements the DBusSocketSet interface using select() directly, just like -epoll.c uses Linux epoll directly; stop building any other DBusSocketSet backends on Windows, which lets us treat -poll.c as Unix-specific * add _dbus_socket_poll() which polls exactly one socket (using _dbus_poll() on Unix and select() on Windows), and use that in dbus-transport-socket.c, something like this: DBusWatchFlags _dbus_socket_poll (DBusSocket sock, int timeout_milliseconds, DBusWatchFlags events); (or alternatively move the DBusSocketSet stuff into the shared library and use that, but I suspect that _dbus_socket_poll() would be really simple on both Windows and Unix) * make _dbus_poll() and DBusPollFD Unix-specific, or just use poll() and struct pollfd directly (we do not currently support any non-Windows platforms without poll() and struct pollfd), in dbus-socket-set-poll.c, _dbus_socket_poll_one(), and the other remaining user of _dbus_poll(), which is the Unix-specific dbus-spawn.c * ... that means we can change DBusPollable from int to struct { int } on Unix, if desired But I think that's too large a change to block this branch while waiting for it, and if we want to do this it deserves its own bug#. Is there anything here that you strongly object to? There are a few points where I have responded, but not changed the code. Do you agree with my reasoning on these or do we need to discuss them further / work out a concrete change that would be mergeable? (In reply to Simon McVittie from comment #78) > (In reply to Ralf Habacker from comment #67) > > not better DBUS_POLLABLE_PRINTABLE (dbus_watch_get_pollable (link->data)) ? > > I have not done anything about this: I hope my response above, explaining > why not, is enough to address it. (In reply to Simon McVittie from comment #84) > It would be incorrect for the Unix DBusPollable to be derived from > DBusSocket ... > So these particular #ifdefs are not harmful, IMO: they are there because > Unix and Windows really do work differently in this respect. > As a future change, we could consider refactoring polling so the Windows > code can stop pretending to have a poll() implementation: ... > But I think that's too large a change to block this branch while waiting for > it, and if we want to do this it deserves its own bug#. (In reply to Simon McVittie from comment #85) > > As a future change, we could consider refactoring polling so the Windows > > code can stop pretending to have a poll() implementation: > ... > > But I think that's too large a change to block this branch while waiting for > > it, and if we want to do this it deserves its own bug#. I've opened Bug #90314 for this part. (In reply to Simon McVittie from comment #85) > Is there anything here that you strongly object to? > > There are a few points where I have responded, but not changed the code. Do > you agree with my reasoning on these or do we need to discuss them further / > work out a concrete change that would be mergeable? I've pushed this as-is (with the fixups applied, obviously). I think we both agree that this branch is better than the previous state, even if there's more that could be done. If there's stuff that you object to, please reopen this (or open a new bug, either works) and suggest how I can fix it. Otherwise: fixed in git for 1.9.16. |
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.
On Unix, sockets are just file descriptors, which are of type int. -1 is the invalid non-socket value used to represent errors. On Windows, sockets are correctly of type SOCKET, which is a UINT_PTR (an unsigned type of size equal to pointer, like ISO C uintptr_t), and INVALID_SOCKET is the invalid non-socket value used to represent errors. At the moment, libdbus "sysdeps" methods that take a socket fd take an int. This is clearly wrong, and leads to warnings like this: /home/ralf.habacker/src/dbus-master/dbus/dbus-sysdeps-win.c: In function '_dbus_socket_is_invalid': /home/ralf.habacker/src/dbus-master/dbus/dbus-sysdeps-win.c:649:15: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] return fd == INVALID_SOCKET ? TRUE : FALSE; We should either do this in dbus-sysdeps.h instead: #ifdef DBUS_WIN typedef int DBusSocket; #else typedef SOCKET DBusSocket; #endif and make _dbus_close_socket() and similar functions take a DBusSocket instead of an int; either that, or use a type-safe struct typedef struct { #ifdef DBUS_WIN SOCKET windows_socket; #else int unix_fd; #endif } DBusSocket; and pass/return it by value, if that's something we can do portably. We cannot do this for public APIs like dbus_connection_get_socket(), which must continue to have their current types (with a cast from the internal representation if necessary). Unix-specific functions can continue to take and return int: the fact that fds are ints, and the fact that sockets are just fds, is quite a fundamental Unix ABI thing. Similarly, Windows-specific functions can use SOCKET or HANDLE directly if desired.