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

[Bin Meng]

Hi Paolo,

On Wed, Sep 21, 2022 at 9:02 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> 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.
>

Ping?

Regards,
Bin