Re: [PATCH 49/51] io/channel-watch: Fix socket watch on Windows

[Bin Meng]

On Wed, Sep 14, 2022 at 4:08 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Wed, Sep 7, 2022 at 1:07 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Clément,
> >
> > On Tue, Sep 6, 2022 at 8:06 PM Clément Chigot <chigot@adacore.com> wrote:
> > >
> > > > > > I checked your patch, what you did seems to be something one would
> > > > > > naturally write, but what is currently in the QEMU sources seems to 
> > > > > > be
> > > > > > written intentionally.
> > > > > >
> > > > > > +Paolo Bonzini , you are the one who implemented the socket watch on
> > > > > > Windows. Could you please help analyze this issue?
> > > > > >
> > > > > > > to avoid WSAEnumNetworkEvents for the master GSource which only 
> > > > > > > has
> > > > > > > G_IO_HUP (or for any GSource having only that).
> > > > > > > As I said above, the current code doesn't do anything with it 
> > > > > > > anyway.
> > > > > > > So, IMO, it's safe to do so.
> > > > > > >
> > > > > > > I'll send you my patch attached. I was planning to send it in the 
> > > > > > > following
> > > > > > > weeks anyway. I was just waiting to be sure everything looks fine 
> > > > > > > on our
> > > > > > > CI. Feel free to test and modify it if needed.
> > > > > >
> > > > > > I tested your patch. Unfortunately there is still one test case
> > > > > > (migration-test.exe) throwing up the "Broken pipe" message.
> > > > >
> > > > > I must say I didn't fully test it against qemu testsuite yet. Maybe 
> > > > > there are
> > > > > some refinements to be done. "Broken pipe" might be linked to the 
> > > > > missing
> > > > > G_IO_HUP support.
> > > > >
> > > > > > Can you test my patch instead to see if your gdb issue can be fixed?
> > > > >
> > > > > Yeah sure. I'll try to do it this afternoon.
> > >
> > > I can't explain how mad at me I am... I'm pretty sure your patch was the 
> > > first
> > > thing I've tried when I encountered this issue. But it wasn't working
> > > or IIRC the
> > > issue went away but that was because the polling was actually disabled 
> > > (looping
> > > indefinitely)...I'm suspecting that I already had changed the CreateEvent 
> > > for
> > > WSACreateEvent which forces you to handle the reset.
> > > Finally, I end up struggling reworking the whole check function...
> > > But yeah, your patch does work fine on my gdb issues too.
> >
> > Good to know this patch works for you too.
> >
> > > And I guess the events are reset when recv() is being called because of 
> > > the
> > > auto-reset feature set up by CreateEvent().
> > > IIUC, what Marc-André means by busy loop is the polling being looping
> > > indefinitely as I encountered. I can ensure that this patch doesn't do 
> > > that.
> > > It can be easily checked by setting the env variable G_MAIN_POLL_DEBUG.
> > > It'll show what g_poll is doing and it's normally always available on
> > > Windows.
> > >
> > > Anyway, we'll wait for Paolo to see if he remembers why he had to call
> > > WSAEnumNetworkEvents. Otherwize, let's go for your patch. Mine might
> > > be a good start to improve the whole polling on Windows but if it doesn't
> > > work in your case, it then needs some refinements.
> > >
> >
> > Yeah, this issue bugged me quite a lot. If we want to reset the event
> > in qio_channel_socket_source_check(), we will have to do the following
> > to make sure qtests are happy.
> >
> > diff --git a/io/channel-watch.c b/io/channel-watch.c
> > index 43d38494f7..f1e1650b81 100644
> > --- a/io/channel-watch.c
> > +++ b/io/channel-watch.c
> > @@ -124,8 +124,6 @@ qio_channel_socket_source_check(GSource *source)
> > return 0;
> > }
> > - WSAEnumNetworkEvents(ssource->socket, ssource->ioc->event, &ev);
> > -
> > FD_ZERO(&rfds);
> > FD_ZERO(&wfds);
> > FD_ZERO(&xfds);
> > @@ -153,6 +151,10 @@ qio_channel_socket_source_check(GSource *source)
> > ssource->revents |= G_IO_PRI;
> > }
> > + if (ssource->revents) {
> > + WSAEnumNetworkEvents(ssource->socket, ssource->ioc->event, &ev);
> > + }
> > +
> > return ssource->revents;
> > }
> >
> > Removing "if (ssource->revents)" won't work.
> >
> > It seems to me that resetting the event twice (one time with the
> > master Gsource, and the other time with the child GSource) causes some
> > bizarre behavior. But MSDN [1] says
> >
> >     "Resetting an event that is already reset has no effect."
> >
> > [1] 
> > https://docs.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-resetevent
> >
>
> Paolo, any comments about this issue?
>

v2 series has been sent out, and this patch remains unchanged.

Paolo, still would appreciate your comments.

Regards,
Bin