On Mon, Feb 04, 2013 at 03:37:39PM +0100, Laszlo Ersek wrote: > I assume this is complex because the pre-patch situation is overly > complex as well: > > slirp -> fd_set > iohandler -> fd_set > unix windows > --------------------------------- > -------------------------------------------- > before after before after > -------------- ----------------- --------------------- > --------------------- > glib -> fd_set glib -> fd_set glib -> GPollFD glib -> GPollFD > fd_set -> GPollFD WaitObj -> GPollFD WaitObj -> GPollFD > > SELECT G_POLL G_POLL (glib/WaitObj) G_POLL > (glib/WaitObj) > > fd_set -> GPollFD > GPollFD -> fd_set > > SELECT (slirp/ioh.) SELECT (slirp/ioh.) > > (I'm ignoring the after-select / after-poll operations; they (should) > mirror the pre-select / pre-poll ones.) > > No idea why the windows version has been mixing g_poll() and select(). > I'd hope that this series kills select() for uniformity's sake, but the > 00/10 subject and the commit msg for this patch indicate otherwise.
This is why I CCed Fabien. I left the g_poll-followed-by-select behavior. It may be to do with Windows treating sockets different from other objects. Paolo may know the answer, too. > "main-loop.c" is full of global variables which makes it a *pain* to read. Yes. > > + if (FD_ISSET(fd, &wfds)) { > > + events |= G_IO_OUT | G_IO_ERR; > > + } > > + if (FD_ISSET(fd, &xfds)) { > > + events |= G_IO_PRI; > > + } > > + if (events) { > > + GPollFD pfd = { > > + .fd = fd, > > + .events = events, > > + }; > > + g_array_append_val(gpollfds, pfd); > > + } > > + } > > +} > > (I don't like "gpollfds" being global, but) this seems OK. It matches > the glib docs and also How the Mapping Should Be Done (TM). > > This function corresponds to the "unix / after / fd_set -> GPollFD" > entry of the diagram, and as such it is composed with "glib -> fd_set" > (glib_select_fill()). I'll note that glib_select_fill() sets "xfds" for > G_IO_ERR but not G_IO_PRI. (According to SUSv4, in our case xfds is the > logical-or of "error pending" and "OOB/urgent pending".) > > We can assume that each entry stored by g_main_context_query() will have > at least one of IN and OUT set in "events". Further assuming that glib > follows its own documentation, that implies that G_IO_ERR will be set > unconditionally (because it always accompanies each of IN and OUT). > glib_select_fill() will map that to xfds, which we then map here to > G_IO_PRI. The total effect is that G_IO_PRI is set on all file > descriptors, always, even if we only try to write. > > I think ultimately support for OOB data should be killed throughout, as > a policy, including xfds & G_IO_PRI. Pending errors are returned by > read()/write()/etc just fine; see eg. libvirt commit d19149dd. slirp uses OOB because it implements TCP. I don't think we can drop it. However, later in the series we drop the glib_select_fill() rfds/wfds/xfds conversion and use GPollFD directly. That solves these conversion issues. > > + > > + gpollfds_to_select(ret); > > > > if (timeout > 0) { > > qemu_mutex_lock_iothread(); > > @@ -327,6 +386,55 @@ void qemu_fd_register(int fd) > > FD_CONNECT | FD_WRITE | FD_OOB); > > } > > > > +static int pollfds_fill(GArray *pollfds, fd_set *rfds, fd_set *wfds, > > + fd_set *xfds) > > +{ > > + int nfds = -1; > > "nfds" shadows the global "nfds". The return value of pollfds_fill() is > then assigned to the global "nfds". Not very elegant (but it's all > rooted in the existense of the global nfds). Global nfds is dropped later in the series. I had real trouble picking good names due to the globals and ended up with temporary shadowing, which is fixed later. > > + if (FD_ISSET(fd, wfds)) { > > + revents |= G_IO_OUT | G_IO_ERR; > > + } > > + if (FD_ISSET(fd, xfds)) { > > + revents |= G_IO_PRI; > > + } > > + pfd->revents |= revents & pfd->events; > > I find this |= vs. = more confusing than the other two. "pfd->revents" > is zero here. Yes, it's an interesting feature of pollfds_poll(). It does not clobber revents. This means you can apply additional *_poll()-style functions before calling pollfds_poll(). I thought this was neat but I guess we can drop it. > > > + } > > +} > > + > > static int os_host_main_loop_wait(uint32_t timeout) > > { > > GMainContext *context = g_main_context_default(); > > @@ -382,12 +490,22 @@ static int os_host_main_loop_wait(uint32_t timeout) > > * improve socket latency. > > */ > > > > + /* This back-and-forth between GPollFDs and select(2) is temporary. > > We'll > > + * drop it in a couple of patches, I promise :). > > + */ > > + gpollfds_from_select(); > > + FD_ZERO(&rfds); > > + FD_ZERO(&wfds); > > + FD_ZERO(&xfds); > > + nfds = pollfds_fill(gpollfds, &rfds, &wfds, &xfds); > > if (nfds >= 0) { > > select_ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv0); > > if (select_ret != 0) { > > timeout = 0; > > + pollfds_poll(gpollfds, nfds, &rfds, &wfds, &xfds); > > This function shouldn't be invoked for "select_ret == -1" (in case > that's possible here); "revents" will end up a copy of "events": > > "On failure, the objects pointed to by the readfds, writefds, and > errorfds arguments shall not be modified." Thanks, will fix. > > > } > > } > > + gpollfds_to_select(select_ret || g_poll_ret); > > I can see this logical-or mimics the return value below; however I think > it's not robust. If one of the operands is -1, and the other is > non-positive, then gpollfds_to_select() should not traverse the > GPollFDs, but it will. > > ... Actually, "g_poll_ret" should have absolutely no effect on > gpollfds_to_select(): "g_poll_ret" corresponds to the "poll_fds" array, > which covers the glib-internal file descriptors and the WaitObjects. > Here (= in the windows case), "gpollfds" covers only slirp & iohandler, > thus only "select_ret" should be passed in. Will take a look at this for the next version of this patch. > I can't decide (yet) if any of the above is important; probably not. > From a quick look at the tree at the series' end, most of it seems to be > gone by then. I'll let you decide. Thanks for the review. If I need to respin I'll address comments. Stefan