Re: [PATCH v3 14/16] win32: avoid mixing SOCKET and file descriptor space

[Marc-André Lureau]

Hi

On Fri, Mar 3, 2023 at 12:54 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 2/21/23 07:47, marcandre.lureau@redhat.com wrote:
> > 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. When necessary,
> > QEMU eventually queries whether it's a socket with the help of
> > fd_is_socket(). However, there is no guarantee of conflict between the
> > fd and SOCKET space. Such conflict would have surprising consequences,
> > we shouldn't mix them.
> >
> > Also, it is often forgotten that SOCKET must be closed with
> > closesocket(), and not close().
> >
> > Instead, let's make the win32 socket wrapper functions return and take a
> > file descriptor, and let util/ wrappers do the fd/SOCKET conversion as
> > necessary. A bit of adaptation is necessary in io/ as well.
> >
> > Unfortunately, we can't drop closesocket() usage, despite
> > _open_osfhandle() documentation claiming transfer of ownership, testing
> > shows bad behaviour if you forget to call closesocket().
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   include/sysemu/os-win32.h |   4 +-
> >   io/channel-watch.c        |   6 +-
> >   util/aio-win32.c          |   9 +-
> >   util/oslib-win32.c        | 219 ++++++++++++++++++++++++++++++++------
> >   4 files changed, 197 insertions(+), 41 deletions(-)
>
> >   #undef connect
> > @@ -308,7 +315,13 @@ int qemu_connect_wrap(int sockfd, const struct 
> > sockaddr *addr,
> >                         socklen_t addrlen)
> >   {
> >       int ret;
> > -    ret = connect(sockfd, addr, addrlen);
> > +    SOCKET s = _get_osfhandle(sockfd);
> > +
> > +    if (s == INVALID_SOCKET) {
> > +        return -1;
> > +    }
> > +
> > +    ret = connect(s, addr, addrlen);
>
>
> Previously you passed int sockfd and now you convert this sockfd to a SOCKET 
> s and you can pass this as an alternative? Or was sockfd before this patch a 
> SOCKET??

yes, sockfd was in fact a SOCKET.

Previous to this patch, a SOCKET is cast to int, as a fake fd, and
back to SOCKET as necessary. The whole point of this patch is to avoid
mixing SOCKET & fd space, instead a SOCKET is associated with a real
CRT fd.

thanks

-- 
Marc-André Lureau