Code archeology

[Nicolas Alvarez]

Gee, this was a fun one...

I was staring at this nice method:
GUI_RPC_CONN_SET::get_fdset(FDSET_GROUP& fg, FDSET_GROUP& all)

Which, as far as I could see, added the exact same file descriptors to both
fg and all. I thought this was silly, the function could take a single
argument and be called twice with both FDSET_GROUPs.

So I went to CLIENT_STATE::do_io_or_sleep where it was called. The code was
somewhat strange... it called get_fdset(gui_rpc_fds, all_fds) but
gui_rpc_fds wasn't used anywhere else. It was only passed to get_fdset.

I found the real issue when I looked at BOINC commits to find where this
code was introduced (3 years ago).

CLIENT_STATE::do_io_or_sleep used to call:

net_xfers->get_fdset(curl_fds);
gui_rpcs.get_fdset(gui_rpc_fds);
all_fds.fdset_union(curl_fds, gui_rpc_fds);

The latter line merged both fdsets into one. This is BOINC revision 7360.
Not long after (revision 7367), it was modified. "You can't easily take the
union of two fd_sets on Windows, so do it a different way." New code:

net_xfers->get_fdset(curl_fds);
all_fds = curl_fds;
gui_rpcs.get_fdset(gui_rpc_fds, all_fds);

and GUI_RPC_CONN_SET::get_fdset was modified to take two FDSET_GROUPs, and
add the same file descriptors to both. In fact, it does exactly the same to
both arguments.

This is stupid in two ways. First, why take two arguments?? It could be just
called like this with a single argument:
gui_rpcs.get_fdset(gui_rpc_fds);
gui_rpcs.get_fdset(all_fds);

Second, I found it wasn't only gui_rpc_fds being unused after all the
changes. *Both* curl_fds and gui_rpc_fds were unused since the code was
introduced!! The only thing done with them after filling them with the
get_fdset methods was merging them into an all_fdset. All the useful code
was done with all_fds.

My new version:

net_xfers->get_fdset(all_fds);
gui_rpcs.get_fdset(all_fds);

That's it. That's what it should have been since the beginning. I should
clarify: get_fdset adds file descriptors to the passed set, doesn't remove
anything from them. So the above is a perfectly good replacement.

As good as this looks at half past midnight, I might be wrong. I'd
appreciate some review:
http://code.google.com/p/synecdoche/source/detail?r=269

Also note in r270 I fixed a bug in my change: all_fds wasn't being zeroed
out. The original code zeroed curl_fds and gui_rpc_fds, but didn't clear
all_fds since it was completely overwritten with the union anyway.

[Der Meister]

Nicolas Alvarez wrote:
> As good as this looks at half past midnight, I might be wrong. I'd
> appreciate some review:
> http://code.google.com/p/synecdoche/source/detail?r=269

Looks OK to me.