Re: [PATCH 3/4] win32: stop mixing SOCKET and file descriptor space

[Daniel P . Berrangé]

On Mon, Feb 20, 2023 at 07:29:11PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Feb 20, 2023 at 4:38 PM Markus Armbruster <armbru@redhat.com> wrote:
> 
> > marcandre.lureau@redhat.com writes:
> >
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > > Until now, a win32 SOCKET handle is often cast to an int file
> > > descriptor, as this is what other OS use for sockets.


> > > @@ -275,15 +276,21 @@ GSource *qio_channel_create_fd_watch(QIOChannel
> > *ioc,
> > >
> > >  #ifdef CONFIG_WIN32
> > >  GSource *qio_channel_create_socket_watch(QIOChannel *ioc,
> > > -                                         int socket,
> > > +                                         int sockfd,
> > >                                           GIOCondition condition)
> > >  {
> > > +    SOCKET s = _get_osfhandle(sockfd);
> >
> > _get_osfhandle() returns a HANDLE as intptr_t.  Is a HANDLE that refers
> > to a socket also a SOCKET?  The docs I found so far are confusing...
> >
> 
> yes
> 
> 
> >
> > >      GSource *source;
> > >      QIOChannelSocketSource *ssource;
> > >
> > > -    WSAEventSelect(socket, ioc->event,
> > > -                   FD_READ | FD_ACCEPT | FD_CLOSE |
> > > -                   FD_CONNECT | FD_WRITE | FD_OOB);
> > > +    if (s == -1 ||
> > > +        WSAEventSelect(s, ioc->event,
> > > +                       FD_READ | FD_ACCEPT | FD_CLOSE |
> > > +                       FD_CONNECT | FD_WRITE | FD_OOB) == SOCKET_ERROR)
> > {
> > > +        g_autofree gchar *emsg = g_win32_error_message(GetLastError());
> > > +        error_printf("error creating socket watch: %s", emsg);
> >
> > Uh, why is printing an error appropriate here?  Shouldn't we leave error
> > handling to callers?
> >
> 
> We could, but we would have to modify callers as well, which can go deep. I
> am considering a &error_warn as a first approach (I am working on something
> to check other WSA API users). Does that sound reasonable?

The caller should also be handling 'NULL' as a return value, as none
of them expect that. They just carry on calling g_source APIs. "Luckily"
glib turns them all into no-ops, so it won't crash, but it also means
the backend is likelyto be non-functional since events won't be
processed.

It isn't clear that there's much of value that a caller can do when it
gets a NULL source either. The context in wich we call this API does
not have error propagation either and its non-trival to add in many
of the callers.

Feels like the realistic choice is between a error_report or an
assert/abort, whether in this method or the caller doesn't make
all that much difference.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|