Re: [libvirt PATCH v2 06/13] util: introduce compat wrappers for Winsock2

On Fri, Jan 17, 2020 at 10:27:05AM +0100, Pavel Hrdina wrote: > On Thu, Jan 16, 2020 at 03:24:41PM +0000, Daniel P. Berrangé wrote: > > Windows sockets take a SOCKET HANDLE object instead of a > > file descriptor. Wrap them in the same way that gnulib > > does so that they use C runtime file descriptors. > > > > While we could in theory use GSocket, it is hard to get > > the exact same semantics libvirt has for its current > > socket usage. Wrapping the Winsock2 APIs is thus the > > easiest approach in the short term. > > > > Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com&gt; > > Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; > > --- > > src/util/Makefile.inc.am | 2 + > > src/util/virsocket.c | 369 +++++++++++++++++++++++++++++++++++++++ > > src/util/virsocket.h | 92 ++++++++++ > > src/util/virutil.c | 29 ++- > > 4 files changed, 488 insertions(+), 4 deletions(-) > > create mode 100644 src/util/virsocket.c > > create mode 100644 src/util/virsocket.h > > > > diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am > > index 23de4a6375..528c9f6cfe 100644 > > --- a/src/util/Makefile.inc.am > > +++ b/src/util/Makefile.inc.am > > @@ -188,6 +188,8 @@ UTIL_SOURCES = \ > > util/virseclabel.h \ > > util/virsecret.c \ > > util/virsecret.h \ > > + util/virsocket.c \ > > + util/virsocket.h \ > > util/virsocketaddr.c \ > > util/virsocketaddr.h \ > > util/virstorageencryption.c \ > > diff --git a/src/util/virsocket.c b/src/util/virsocket.c > > new file mode 100644 > > index 0000000000..2d770b931b > > --- /dev/null > > +++ b/src/util/virsocket.c > > @@ -0,0 +1,369 @@ > > +/* > > + * Copyright (C) 2020 Red Hat, Inc. > > + * > > + * This library is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU Lesser General Public > > + * License as published by the Free Software Foundation; either > > + * version 2.1 of the License, or (at your option) any later version. > > + * > > + * This library is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + * Lesser General Public License for more details. > > + * > > + * You should have received a copy of the GNU Lesser General Public > > + * License along with this library. If not, see > > + * <http://www.gnu.org/licenses/>;. > > + */ > > + > > +#include <config.h> > > + > > +#include "virsocket.h" > > + > > +#ifdef WIN32 > > + > > +# include <fcntl.h> > > + > > +# define FD2SK(fd) _get_osfhandle(fd) > > +# define SK2FD(sk) (_open_osfhandle((intptr_t) (sk), O_RDWR | O_BINARY)) > > + > > +# define GET_HANDLE(fd) \ > > + > > +# define RETURN_ERROR(call) \ > > + if ((call) < 0) { \ > > + set_errno(); \ > > + return -1; \ > > + } > > + > > +# undef accept > > +# undef bind > > +# undef closesocket > > +# undef connect > > +# undef dup > > +# undef dup2 > > We are not calling these two here so probably no need to undef here. > > > +# undef getpeername > > +# undef getsockname > > +# undef getsockopt > > +# undef ioctlsocket > > +# undef listen > > +# undef setsockopt > > +# undef socket > > [...] > > > diff --git a/src/util/virsocket.h b/src/util/virsocket.h > > new file mode 100644 > > index 0000000000..e9ef5380a3 > > --- /dev/null > > +++ b/src/util/virsocket.h > > @@ -0,0 +1,92 @@ > > +/* > > + * Copyright (C) 2020 Red Hat, Inc. > > + * > > + * This library is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU Lesser General Public > > + * License as published by the Free Software Foundation; either > > + * version 2.1 of the License, or (at your option) any later version. > > + * > > + * This library is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + * Lesser General Public License for more details. > > + * > > + * You should have received a copy of the GNU Lesser General Public > > + * License along with this library. If not, see > > + * <http://www.gnu.org/licenses/>;. > > + */ > > + > > +#pragma once > > + > > +#include "internal.h" > > + > > +#ifdef WIN32 > > + > > +# define WIN32_LEAN_AND_MEAN > > +# include <errno.h> > > +# include <winsock2.h> > > +# include <ws2tcpip.h> > > +# include <io.h> > > + > > +int vir_accept(int fd, struct sockaddr *addr, socklen_t *addrlen); > > +int vir_bind(int fd, const struct sockaddr *addr, socklen_t addrlen); > > +int vir_closesocket(int fd); > > +int vir_connect(int fd, const struct sockaddr *addr, socklen_t addrlen); > > +int vir_dup(int oldfd); > > +int vir_dup2(int oldfd, int newfd); > > These two are not implemented anywhere as you are using the _dup and > _dup2 from io.h. Opps, left over from earlier version when I did indeed replace dup/dup2. > > > +int vir_getpeername(int fd, struct sockaddr *addr, socklen_t *addrlen); > > +int vir_getsockname(int fd, struct sockaddr *addr, socklen_t *addrlen); > > +int vir_listen(int fd, int backlog); > > +int vir_ioctlsocket(int fd, int cmd, void *arg); > > +int vir_getsockopt(int fd, int level, int optname, > > + void *optval, socklen_t *optlen); > > +int vir_setsockopt(int fd, int level, int optname, > > + const void *optval, socklen_t optlen); > > +int vir_socket(int domain, int type, int protocol); > > One nit that I missed in v1, the function declaration doesn't follow > syntax style where each argument should be on separate line, but I don't > care that much for these wrappers. We've generally ignored that in the .h files in practice. > > > + > > + > > +/* Get rid of GNULIB's replacements */ > > +# undef accept > > +# undef bind > > +# undef closesocket > > +# undef connect > > +# undef dup > > +# undef dup2 > > +# undef getpeername > > +# undef getsockname > > +# undef getsockopt > > +# undef ioctlsocket > > +# undef listen > > +# undef setsockopt > > +# undef socket > > + > > +/* Provide our own replacements */ > > +# define accept vir_accept > > +# define bind vir_bind > > +# define closesocket vir_closesocket > > +# define connect vir_connect > > +# define dup _dup > > +# define dup2 _dup2 > > +# define ioctlsocket vir_ioctlsocket > > +# define getpeername vir_getpeername > > +# define getsockname vir_getsockname > > +# define getsockopt vir_getsockopt > > +# define listen vir_listen > > +# define setsockopt vir_setsockopt > > +# define socket vir_socket > > + > > +#else > > + > > +# include <sys/socket.h> > > +# include <unistd.h> > > +# include <sys/ioctl.h> > > +# include <arpa/inet.h> > > +# include <netinet/in.h> > > +# include <netinet/udp.h> > > +# include <netinet/tcp.h> > > +# include <sys/un.h> > > + > > +# define closesocket close > > +# define ioctlsocket ioctl > > + > > +#endif > > diff --git a/src/util/virutil.c b/src/util/virutil.c > > index a0fd7618ee..9ea9e2946d 100644 > > --- a/src/util/virutil.c > > +++ b/src/util/virutil.c > > Changes to this should be in a separate patch but otherwise it looks > good. I did have it separately before, but it breaks bisectability, because the GNULIB socket wrappers & non-blocking impl are mutually dependant.