Bug 90314

I worked a few in that direction, not sure if it matches your needs, see 
 http://cgit.freedesktop.org/~rhabacker/dbus/log/?h=89444-comment-84-dbus-socket-set-winsock. It is based on the patches of bug 89444

That looks like a good start, thanks! I'll try to check the design soon.

> +/* dbus-socket-set-winsock.c - a socket set implemented via winsock

Did this start life as a copy of dbus-socket-set-poll.c? A lot of it looks extremely familiar. It might have a clearer git history if that was represented in git: one commit that just copies dbus-socket-set-poll.c, and a subsequent commit that renames things and hooks it up to the build system.

I was hoping you wouldn't have to rebuild the fd_set every time, but I was misremembering how select() works - it is indeed destructive (edits the fd_set), so what you're doing is the best we can do, unless it's considered valid in Winsock to say

    typedef struct {
      int n;
      fd_set read;
      fd_set write;
      fd_set exc;
    } DBusSocketSetWinsock;

    ... initialize those ...

    while (1)
      {
        fd_set r = set->read;             /* <- is this considered valid? */
        fd_set w = set->write;
        fd_set e = set->exc;

        ... = select (n, &r, &w, &e, timeout);
        ...
      }

It has copyright holders other than you, for the stuff copied from the poll implementation (which I wrote for Nokia, as you can see from its copyright boilerplate).

We could have a lot less diffstat if we left _dbus_poll() in dbus-sysdeps-win.c, and later in the patch series (when -poll becomes Unix-only), moved its declaration (along with that of DBusPollFD) to dbus-sysdeps-{win,unix}.h?

> +int
> +_dbus_socket_poll (DBusSocket s, int timeout)
> +{
> + DBusSocketEvent ready_fds;
> + DBusSocketSet *set = _dbus_socket_set_new (0);
> + ready_fds.fd = s;
> + int n_ready = _dbus_socket_set_poll (set, &ready_fds, 1, timeout);
> + _dbus_socket_set_free (set);
> + return n_ready;
> +}

This is going to need a way to get the "events" in and the "revents" out. I originally said it could return the "revents", but thinking about it, I suspect that might not actually be OK, because we also need to be able to distinguish between "event", "timed out", and "some error".

Creating a new DBusSocketSet is pretty inefficient for a one-shot operation: it does a significant amount of malloc()ing. My intention had been to just implement this in terms of native poll() (Unix) or select() (Windows) directly - the trivial case for a single socket is presumably much, much simpler than the "n sockets" case.

I'd like to use DBusWatchFlags for the events/revents, either immediately or after some refactoring. _DBUS_POLLWHATEVER is part of the same leaky abstraction as _dbus_poll(). We should never have to write code like this:

#ifdef _AIX
...
#define _DBUS_POLLIN      0x0001
...
#elif defined(__QNX__)
...
#define _DBUS_POLLIN      0x0005
...

and if we do, we certainly shouldn't try to claim that it's portable or an abstraction :-)

> -_dbus_socket_poll (DBusSocket s, int timeout)
> +_dbus_pollable_poll (DBusPollable p, int timeout)

Interesting idea, but my plan had been to only use _dbus_socket_poll() for DBusTransportSocket, and just use poll() directly for its Unix-specific uses (which are the only places where it's valid to poll a non-socket).

>  set (DBUS_SHARED_SOURCES ${DBUS_SHARED_SOURCES}
> ${DBUS_DIR}/dbus-file-win.c
> ${DBUS_DIR}/dbus-init-win.cpp
> + ${DBUS_DIR}/dbus-socket-set-winsock.c

I'd rather keep DBusSocketSet out of libdbus-1 if we can help it - it is explicitly not thread-safe, for a start.

If _dbus_socket_poll() just uses poll() or select() directly, then that shouldn't be too hard.

(In reply to Simon McVittie from comment #0)
> * 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)

Oh, ugh, this is not actually true... _dbus_poll() falls back to select() :-(

OK, I have a simpler plan for a starting point. I'll attach a patch shortly.

(In reply to Simon McVittie from comment #5)
> OK, I have a simpler plan for a starting point. I'll attach a patch shortly.

Or not. Sorry, still thinking about the best way to attack this, given that there are two annoyingly different code paths on Unix, and we still need something essentially equivalent to an array of DBusPollFD on Windows.

11:27 < smcv> the point of DBusPollFD is to try to avoid including OS-dependent 
              bits in otherwise OS-independent APIs
11:27 < smcv> but now I think about it
11:27 < smcv> we already do
11:27 < smcv> #ifdef HAVE_POLL
11:27 < smcv> #include <sys/poll.h>
11:27 < smcv> #endif
11:27 < vapula> exactly
11:27 < smcv> so... we could maybe do
11:27 < smcv> #ifdef HAVE_POLL
11:28 < smcv> typedef struct pollfd DBusPollFD;
11:28 < vapula> yup
11:28 < smcv> #define _DBUS_POLLIN POLLIN /* etc. */
11:28 < smcv> #else
11:28 < smcv> ... what we do now
11:28 < smcv> that would be fractionally less awful
11:28 < smcv> it still isn't an abstraction
11:28 < vapula> benefit is that you can also remove the runtime check in 
                _dbus_poll
11:29 < smcv> right
11:29 < smcv> oh, it should be #if defined(HAVE_POLL) && !defined(BROKEN_POLL)
11:29 < smcv> because apparently we support platforms where poll() exists but 
              is wrong, or something
11:29 < smcv> portability. isn't it wonderful

(this was in the context of vapula wanting to port libdbus to a the Integrity RTOS, where struct pollfd has additional members beyond its traditional "shape")

Created attachment 116919 [details] [review]
when HAVE_POLL, use the system definition of struct pollfd

Patch to map system definitions of struct pollfd and POLL* macros to d-bus ones when available.
Of note, the current portable DBusPollFD is not fully portable, since http://pubs.opengroup.org/onlinepubs/009695399/basedefs/poll.h.html mentions that there are some members _at least_ there, but there may be more.

Forgot to mention that was created against 1.9.16

Comment on attachment 116919 [details] [review]
when HAVE_POLL, use the system definition of struct pollfd

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

::: dbus-1.9.16.linux/dbus/dbus-sysdeps.h
@@ +341,4 @@
>  #define _DBUS_POLLHUP     0x0040
>  /** Invalid request: fd not open */
>  #define _DBUS_POLLNVAL    0x1000
> +#elif HAVE_POLL

This should be #if defined(HAVE_POLL) && !defined(BROKEN_POLL), i.e. exactly the circumstances in which we call the real poll() at all.

The OS-specific bits above here (for QNX, Haiku, AIX) can go away, because we should now be using their genuine POLLIN, POLLPRI etc. constants in any case.

The #else should have a comment "we fake poll() in terms of select()" or something.

@@ +371,5 @@
>  
> +/**
> + * A portable struct pollfd wrapper. 
> + */
> +#if HAVE_POLL

#if defined(HAVE_POLL) && !defined(BROKEN_POLL)

(Be careful with the difference between #if X and #if defined(X) - they are not the same, although the latter is equivalent to #ifdef X.)

(In reply to Rolland Dudemaine from comment #9)
> Of note, the current portable DBusPollFD is not fully portable, since
> http://pubs.opengroup.org/onlinepubs/009695399/basedefs/poll.h.html mentions
> that there are some members _at least_ there, but there may be more.

If you delete the AIX, Haiku, QNX special cases as I suggested, then we will be defining our own structure if and only if we are not actually going to call poll() anyway.

See dbus/dbus-sysdeps-unix.c for the actual implementation; as you noted on IRC, the runtime checks can go away too.

In other words, the changes I'm asking you for are really just an exercise in deleting code :-)

It would be ideal if you could propose a patch in "git format-patch" format - that's the easiest way to maintain attribution information when I merge stuff.

Created attachment 116941 [details] [review]
pdated patch including _dbus_poll()

Here is a git patch against HEAD with both the header and _dbus_poll() implementation.
Better ?

Comment on attachment 116941 [details] [review]
pdated patch including _dbus_poll()

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

Yes, this looks good. I'll adjust the comments to make it a little clearer.

::: dbus/dbus-sysdeps-unix.c
@@ +2758,3 @@
>      }
> +
> +  return poll ((struct pollfd*) fds,

This cast isn't actually needed any more, which is a plus.

Fixed in git for 1.9.20, with some adjustments.

I think this is the best abstraction we're likely to get, so I'm closing this.