+more people
On Tue, Oct 11, 2022 at 6:42 PM Bin Meng <bmeng...@gmail.com> wrote:
>
> Hi Paolo,
>
> On Thu, Oct 6, 2022 at 11:03 AM Bin Meng <bmeng...@gmail.com> wrote:
> >
> > Hi Paolo,
> >
> > On Wed, Sep 28, 2022 at 2:10 PM Bin Meng <bmeng...@gmail.com> wrote:
> > >
> > > Hi Paolo,
> > >
> > > On Wed, Sep 21, 2022 at 9:02 AM Bin Meng <bmeng...@gmail.com> wrote:
> > > >
> > > > On Wed, Sep 14, 2022 at 4:08 PM Bin Meng <bmeng...@gmail.com> wrote:
> > > > >
> > > > > On Wed, Sep 7, 2022 at 1:07 PM Bin Meng <bmeng...@gmail.com> wrote:
> > > > > >
> > > > > > Hi Clément,
> > > > > >
> > > > > > On Tue, Sep 6, 2022 at 8:06 PM Clément Chigot <chi...@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?
> > >
> >
> > Ping? Can you please comment??
> >
>
> Ping?
>
Hi,
Paolo remains silent. Please let me know who else could approve this
change. Thanks.
Regards,
Bin
