Re: [Qemu-devel] [PATCH v2 0/5] Do not set SO_REUSEADDR on Windows

[Stefan Hajnoczi]

On Mon, Sep 09, 2013 at 02:15:20PM +0200, Sebastian Ottlik wrote:
> On 09.09.2013 14:05, Stefan Hajnoczi wrote:
> >On Thu, Sep 05, 2013 at 03:48:16PM +0200, Sebastian Ottlik wrote:
> >>On 04.09.2013 19:08, Sebastian Ottlik wrote:
> >>>This patchset disabels all use of SO_REUSEADDR on Windows. On Windows 
> >>>systems
> >>>the default behaviour is equivalent to SO_REUSEADDR on other operating
> >>>systems. SO_REUSEADDR can still be set but results in undesired behaviour
> >>>instead. It may even lead to situations were system behaviour is
> >>>unspecified. More information on this can be found at:
> >>>http://msdn.microsoft.com/en-us/library/windows/desktop/ms740621.aspx
> >>>
> >>>I originally encountered this issue when accidentally launching two QEMU
> >>>instances with identical GDB ports at the same time. In which case QEMU 
> >>>won't
> >>>fail as one might expect.
> >>>
> >>>v2 Changes:
> >>>
> >>>- Introduce a function with os specific implementation instead of using 
> >>>#ifdef
> >>>   I named it socket_set_fast_reuse instead of the suggested 
> >>> qemu_set_reuseaddr
> >>>   so the name better reflects what the function actually does.
> >>>
> >>>  gdbstub.c              |    6 ++----
> >>>  include/qemu/sockets.h |    1 +
> >>>  net/socket.c           |   19 +++++++------------
> >>>  slirp/misc.c           |    3 +--
> >>>  slirp/socket.c         |    4 +---
> >>>  slirp/tcp_subr.c       |    6 ++----
> >>>  slirp/udp.c            |    4 ++--
> >>>  util/oslib-posix.c     |   14 ++++++++++++++
> >>>  util/oslib-win32.c     |   10 ++++++++++
> >>>  util/qemu-sockets.c    |    6 +++---
> >>>  10 files changed, 43 insertions(+), 30 deletions(-)
> >>>
> >>>
> >>>util: add socket_set_fast_reuse function
> >>>gdbstub: call socket_set_fast_reuse instead of setting SO_REUSEADDR
> >>>net: call socket_set_fast_reuse instead of setting SO_REUSEADDR
> >>>slirp: call socket_set_fast_reuse instead of setting SO_REUSEADDR
> >>>util: call socket_set_fast_reuse instead of setting SO_REUSEADDR
> >>>
> >>Pinging this patch, as I think it is still an appropriate approach
> >>to the issue:
> >>
> >>I did some research and apparently there is a valid use case for
> >>SO_REUSEADDR
> >>on windows when multiple clients need to listen to the same port for
> >>the same
> >>multicast group. IMHO making qemu_setsockopt ignore SO_REUSEADDR on windows
> >>might be confusing for some use cases. Actually net_socket_mcast_create in
> >>net/socket.c should probably set SO_REUSEADDR on windows. This is
> >>also an issue
> >>with patch 3 I supplied that I will address in a new version of this
> >>patch set if there is
> >>an agreement on a general approach.
> >Sounds like a good idea.  The patch series overall looks good.
> >
> >Stefan
> Thanks for the feedback. I will resubmit the patch series including
> the change for net_socket_mcast_create and fixes for the style
> issues you pointed out soon.
> 
> When I submitted this new version of the patch set I think I was a
> little early as there was still some discussion in the thread of the
> original version. In general, what is a good period to wait before
> submitting a new version?

Sending an extra revision is not a problem.  In fact, I think that's
better than waiting too long and forgetting about the series.  If you
want to wait for discussion to end, two business days seems like a safe
period of time.

By being responsive you will also spur reviewers to be responsive :).

Stefan