Re: [PATCH spice-server 05/33] sys-socket: Introduce some utility to make sockets more portable

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

On Fri, Dec 21, 2018 at 4:03 PM Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>
> Between Unix and Windows socket are quite different:
> - on Windows sockets have a different namespace from C file
>   descriptors so you can't use read/write/close or similar functions;
> - errors are not stored in errno but you must be read/write the
>   errors with specific function;
> - sometimes sockets are put in non-blocking mode automatically
>   calling some functions;
> - SOCKET type is 64 bit on Windows 64 which does not fit technically
>   in an int (although many programs assume it and currently is true).
>

While Windows could use values that don't fit in int, they would break
so many programs that we can safely assume this will never happen:
https://stackoverflow.com/questions/1953639/is-it-safe-to-cast-socket-to-int-under-win64

> So encapsulate the socket APIs in some definition to make easier
> and more safe to deal with them.
> Where the portability to Windows would make to code more offuscated a Unix

obfuscated

> style was preferred. For instance if errors are detected errno is set from
> Windows socket error instead of changing all code handling.
> Fortunately on Windows Qemu core interface accepts socket (but not
> other types like C file descriptors!).
>
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
>  server/Makefile.am  |   2 +
>  server/sys-socket.c | 212 ++++++++++++++++++++++++++++++++++++++++++++
>  server/sys-socket.h | 188 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 402 insertions(+)
>  create mode 100644 server/sys-socket.c
>  create mode 100644 server/sys-socket.h
>
> diff --git a/server/Makefile.am b/server/Makefile.am
> index 5009d197..3c0ec0c0 100644
> --- a/server/Makefile.am
> +++ b/server/Makefile.am
> @@ -167,6 +167,8 @@ libserver_la_SOURCES =                              \
>         stat.h                                  \
>         stream-channel.c                        \
>         stream-channel.h                        \
> +       sys-socket.h                            \
> +       sys-socket.c                            \
>         red-stream-device.c                     \
>         red-stream-device.h                     \
>         sw-canvas.c                             \
> diff --git a/server/sys-socket.c b/server/sys-socket.c
> new file mode 100644
> index 00000000..7ce5dab1
> --- /dev/null
> +++ b/server/sys-socket.c
> @@ -0,0 +1,212 @@
> +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> +/*
> +   Copyright (C) 2018 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/>.
> +*/
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stdbool.h>
> +#include <string.h>
> +#include <sys/types.h>
> +#ifndef _WIN32
> +#include <arpa/inet.h>
> +#include <netinet/in.h>
> +#include <netinet/ip.h>
> +#include <netinet/tcp.h>
> +#include <sys/socket.h>
> +#endif
> +
> +#include <common/log.h>
> +
> +#include "sys-socket.h"
> +
> +#ifdef _WIN32
> +// Map Windows socket errors to standard C ones
> +// See https://msdn.microsoft.com/en-us/library/windows/desktop/ms740668(v=vs.85).aspx
> +void socket_win32_set_errno(void)
> +{
> +    int err = EPIPE; // default
> +    switch (WSAGetLastError()) {
> +    case WSAEWOULDBLOCK:
> +    case WSAEINPROGRESS:
> +        err = EAGAIN;
> +        break;
> +    case WSAEINTR:
> +        err = EINTR;
> +        break;
> +    case WSAEBADF:
> +        err = EBADF;
> +        break;
> +    case WSA_INVALID_HANDLE:
> +    case WSA_INVALID_PARAMETER:
> +    case WSAEINVAL:
> +        err = EINVAL;
> +        break;
> +    case WSAENOTSOCK:
> +        err = ENOTSOCK;
> +        break;
> +    case WSA_NOT_ENOUGH_MEMORY:
> +        err = ENOMEM;
> +        break;
> +    case WSAEPROTONOSUPPORT:
> +    case WSAESOCKTNOSUPPORT:
> +    case WSAEOPNOTSUPP:
> +    case WSAEPFNOSUPPORT:
> +    case WSAEAFNOSUPPORT:
> +    case WSAVERNOTSUPPORTED:
> +        err = ENOTSUP;
> +        break;
> +    case WSAEFAULT:
> +        err = EFAULT;
> +        break;
> +    case WSAEACCES:
> +        err = EACCES;
> +        break;
> +    case WSAEMFILE:
> +        err = EMFILE;
> +        break;
> +    case WSAENAMETOOLONG:
> +        err = ENAMETOOLONG;
> +        break;
> +    case WSAENOTEMPTY:
> +        err = ENOTEMPTY;
> +        break;
> +    case WSA_OPERATION_ABORTED:
> +    case WSAECANCELLED:
> +    case WSA_E_CANCELLED:
> +        err = ECANCELED;
> +        break;
> +    case WSAEADDRINUSE:
> +        err = EADDRINUSE;
> +        break;
> +    case WSAENETDOWN:
> +        err = ENETDOWN;
> +        break;
> +    case WSAENETUNREACH:
> +        err = ENETUNREACH;
> +        break;
> +    case WSAENETRESET:
> +        err = ENETRESET;
> +        break;
> +    case WSAECONNABORTED:
> +        err = ECONNABORTED;
> +        break;
> +    case WSAECONNRESET:
> +        err = ECONNRESET;
> +        break;
> +    case WSAEISCONN:
> +        err = EISCONN;
> +        break;
> +    case WSAENOTCONN:
> +        err = ENOTCONN;
> +        break;
> +    case WSAETIMEDOUT:
> +        err = ETIMEDOUT;
> +        break;
> +    case WSAECONNREFUSED:
> +        err = ECONNREFUSED;
> +        break;
> +    case WSAEHOSTUNREACH:
> +        err = EHOSTUNREACH;
> +        break;
> +    case WSAEDESTADDRREQ:
> +        err = EDESTADDRREQ;
> +        break;
> +    case WSAEMSGSIZE:
> +        err = EMSGSIZE;
> +        break;
> +    case WSAEPROTOTYPE:
> +        err = EPROTOTYPE;
> +        break;
> +    case WSAENOPROTOOPT:
> +        err = ENOPROTOOPT;
> +        break;
> +    case WSAEADDRNOTAVAIL:
> +        err = EADDRNOTAVAIL;
> +        break;
> +    case WSAENOBUFS:
> +        err = ENOBUFS;
> +        break;
> +    // TODO
> +    case WSAESTALE:
> +    case WSAEDISCON:
> +    case WSA_IO_INCOMPLETE:
> +    case WSA_IO_PENDING:
> +    case WSAEALREADY:
> +    case WSAESHUTDOWN:
> +    case WSAETOOMANYREFS:
> +    case WSAELOOP:
> +    case WSAEHOSTDOWN:
> +    case WSAEPROCLIM:
> +    case WSAEUSERS:
> +    case WSAEDQUOT:
> +    case WSAEREMOTE:
> +    case WSASYSNOTREADY:
> +    case WSANOTINITIALISED:
> +    case WSAENOMORE:
> +    case WSAEINVALIDPROCTABLE:
> +    case WSAEINVALIDPROVIDER:
> +    case WSAEPROVIDERFAILEDINIT:
> +    case WSASYSCALLFAILURE:
> +    case WSASERVICE_NOT_FOUND:
> +    case WSATYPE_NOT_FOUND:
> +    case WSA_E_NO_MORE:
> +    case WSAEREFUSED:
> +    case WSAHOST_NOT_FOUND:
> +    case WSATRY_AGAIN:
> +    case WSANO_RECOVERY:
> +    case WSANO_DATA:
> +    case WSA_QOS_RECEIVERS:
> +    case WSA_QOS_SENDERS:
> +    case WSA_QOS_NO_SENDERS:
> +    case WSA_QOS_NO_RECEIVERS:
> +    case WSA_QOS_REQUEST_CONFIRMED:
> +    case WSA_QOS_ADMISSION_FAILURE:
> +    case WSA_QOS_POLICY_FAILURE:
> +    case WSA_QOS_BAD_STYLE:
> +    case WSA_QOS_BAD_OBJECT:
> +    case WSA_QOS_TRAFFIC_CTRL_ERROR:
> +    case WSA_QOS_GENERIC_ERROR:
> +    case WSA_QOS_ESERVICETYPE:
> +    case WSA_QOS_EFLOWSPEC:
> +    case WSA_QOS_EPROVSPECBUF:
> +    case WSA_QOS_EFILTERSTYLE:
> +    case WSA_QOS_EFILTERTYPE:
> +    case WSA_QOS_EFILTERCOUNT:
> +    case WSA_QOS_EOBJLENGTH:
> +    case WSA_QOS_EFLOWCOUNT:
> +    case WSA_QOS_EUNKOWNPSOBJ:
> +    case WSA_QOS_EPOLICYOBJ:
> +    case WSA_QOS_EFLOWDESC:
> +    case WSA_QOS_EPSFLOWSPEC:
> +    case WSA_QOS_EPSFILTERSPEC:
> +    case WSA_QOS_ESDMODEOBJ:
> +    case WSA_QOS_ESHAPERATEOBJ:
> +    case WSA_QOS_RESERVED_PETYPE:
> +        break;
> +    }
> +    errno = err;
> +}
> +
> +SPICE_CONSTRUCTOR_FUNC(socket_win32_init)
> +{
> +    WSADATA wsaData;
> +    WSAStartup(MAKEWORD(2, 2), &wsaData);
> +}
> +#endif
> diff --git a/server/sys-socket.h b/server/sys-socket.h
> new file mode 100644
> index 00000000..c21e9a69
> --- /dev/null
> +++ b/server/sys-socket.h
> @@ -0,0 +1,188 @@
> +/*
> +   Copyright (C) 2018 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/>.
> +*/
> +
> +/* Small compatibility layer for sockets, mostly to make easier portability
> + * for Windows but without loosing performances under Unix, the most supported
> + * system */
> +#ifndef RED_SYS_SOCKET_H_
> +#define RED_SYS_SOCKET_H_
> +
> +#include <stdbool.h>
> +
> +#ifndef _WIN32
> +#  include <sys/socket.h>
> +
> +#if 0

should be removed

> +typedef struct {
> +    int fd;
> +} socket_t;
> +
> +static inline int socket_get_raw(socket_t sock)
> +{
> +    return sock.fd;
> +}
> +#define SOCKET_INVALID ((socket_t){-1})
> +#define SOCKET_FROM_INT(fd) ((socket_t){fd})
> +#else
> +typedef int socket_t;

Imho, it's not a great idea to introduce socket_t.

Everybody uses int. It's a mistake from Microsoft (actually winsock)
to use a type that isn't compatible with int (berkley/posix api).
They will never break compatibility with int, because most programs
would start breaking. (there are various sources of that claim on the
internet).

Please avoid _t, it may conflict with system types, it is reserved:
(https://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html)

Please reconsider using plain int (btw, that's what qemu and glib use
two to store SOCKET, so Spice is doomed)

> +static inline int socket_get_raw(socket_t sock)
> +{
> +    return sock;
> +}
> +#define SOCKET_INVALID (-1)
> +#define SOCKET_FROM_INT(fd) (fd)
> +#endif
> +
> +static inline bool socket_is_valid(socket_t sock)
> +{
> +    return socket_get_raw(sock) >= 0;
> +}
> +
> +static inline void socket_win32_set_errno(void)
> +{
> +}
> +
> +#define socket_read(sock, buf, len) read(socket_get_raw(sock), buf, len)
> +#define socket_write(sock, buf, len) write(socket_get_raw(sock), buf, len)
> +#define socket_writev(sock, iov, n) writev(socket_get_raw(sock), iov, n)
> +#define socket_close(sock) close(socket_get_raw(sock))
> +#define socket_getopt(sock, lvl, type, value, len) \
> +    getsockopt(socket_get_raw(sock), lvl, type, value, len)
> +#define socket_setopt(sock, lvl, type, value, len) \
> +    setsockopt(socket_get_raw(sock), lvl, type, value, len)
> +#define socket_listen(sock, num) \
> +    listen(socket_get_raw(sock), num)
> +
> +#else
> +#  include <winsock2.h>
> +#  include <windows.h>
> +typedef int socklen_t;
> +
> +#if ENABLE_EXTRA_CHECKS

same

> +typedef struct {
> +    SOCKET fd;
> +} socket_t;
> +
> +static inline int socket_get_raw(socket_t sock)
> +{
> +    return sock.fd;
> +}
> +
> +#define SOCKET_INVALID ((socket_t){INVALID_SOCKET})
> +#define SOCKET_FROM_INT(fd) ((socket_t){fd})
> +#else
> +typedef SOCKET socket_t;
> +
> +static inline int socket_get_raw(socket_t sock)
> +{
> +    return sock;
> +}
> +
> +#define SOCKET_INVALID INVALID_SOCKET
> +#endif
> +
> +// this definition is ABI compatible with WSABUF
> +struct iovec {
> +    u_long iov_len;
> +    void FAR *iov_base;
> +};
> +
> +void socket_win32_set_errno(void);
> +
> +static inline ssize_t socket_read(socket_t sock, void *buf, size_t count)
> +{
> +    ssize_t res = recv(socket_get_raw(sock), buf, count, 0);
> +    if (res < 0) {
> +        socket_win32_set_errno();
> +    }
> +    return res;
> +}
> +
> +static inline ssize_t socket_write(socket_t sock, const void *buf, size_t count)
> +{
> +    ssize_t res = send(socket_get_raw(sock), buf, count, 0);
> +    if (res < 0) {
> +        socket_win32_set_errno();
> +    }
> +    return res;
> +}
> +
> +static inline ssize_t socket_writev(socket_t sock, const struct iovec *iov, int n_iov)
> +{
> +    DWORD sent;
> +    int res = WSASend(socket_get_raw(sock), (LPWSABUF) iov, n_iov, &sent, 0, NULL, NULL);
> +    if (res) {
> +        socket_win32_set_errno();
> +        return -1;
> +    }
> +    return sent;
> +}
> +
> +#define socket_close(sock) closesocket(socket_get_raw(sock))
> +
> +static inline bool socket_is_valid(socket_t sock)
> +{
> +    return socket_get_raw(sock) != INVALID_SOCKET;
> +}
> +
> +#define SHUT_RDWR SD_BOTH
> +
> +static inline int
> +socket_getopt(socket_t sock, int lvl, int type, void *value, socklen_t *len)
> +{
> +    int res = getsockopt(socket_get_raw(sock), lvl, type, value, len);
> +    if (res < 0) {
> +        socket_win32_set_errno();
> +    }
> +    return res;
> +}
> +
> +static inline int
> +socket_setopt(socket_t sock, int lvl, int type, const void *value, socklen_t len)
> +{
> +    int res = setsockopt(socket_get_raw(sock), lvl, type, value, len);
> +    if (res < 0) {
> +        socket_win32_set_errno();
> +    }
> +    return res;
> +}
> +
> +static inline int
> +socket_listen(socket_t sock, int backlog)
> +{
> +    int res = listen(socket_get_raw(sock), backlog);
> +    if (res < 0) {
> +        socket_win32_set_errno();
> +    }
> +    return res;
> +}
> +#endif
> +
> +// common part
> +//
> +
> +#define socket_getpeername(sock, name, len) \
> +    getpeername(socket_get_raw(sock), name, len)
> +#define socket_getsockname(sock, name, len) \
> +    getsockname(socket_get_raw(sock), name, len)
> +#define socket_new(af, type, flags) \
> +    SOCKET_FROM_INT(socket(af, type, flags))
> +// FIXME windows, set error!
> +#define socket_bind(sock, addr, len) \
> +    bind(socket_get_raw(sock), addr, len)
> +
> +#endif // RED_SYS_SOCKET_H_
> --
> 2.17.2
>

Looks ok overall.

It's a bit sad that Spice doesn't use GIO instead, GSocket would do
most of this, and more.
There is also GIOChannel, although it's a bit more archaic,
g_io_channel_win32_new_socket() etc..



--
Marc-André Lureau
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux Virtualization]     [Linux Virtualization]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]