[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] linux-user: improve target_to_host_sock_type co
|
From: |
Aurelien Jarno |
|
Subject: |
Re: [Qemu-devel] [PATCH] linux-user: improve target_to_host_sock_type conversion for MIPS |
|
Date: |
Sun, 24 Mar 2013 11:27:03 +0100 |
|
User-agent: |
Mutt/1.5.21 (2010-09-15) |
I am reviewing that for the MIPS part, and I am not really used to the
linux-user code, so a review from someone familiar with it would be
appreciated.
I added Richard Henderson and Blue Swirl in Cc: as alpha and sparc are
also affected by this issue.
On Mon, Mar 18, 2013 at 11:47:05PM +0100, Petar Jovanovic wrote:
> From: Petar Jovanovic <address@hidden>
>
> Previous implementation has failed to take into account different value of
> SOCK_NONBLOCK on target (MIPS) and host, and existence of SOCK_CLOEXEC.
> The same conversion has to be applied both for do_socket and do_socketpair,
> so the code has been isolated in a static inline function.
> It is defined for MIPS target only.
>
> enum sock_type in linux-user/socket.h has been extended to include
> TARGET_SOCK_CLOEXEC and TARGET_SOCK_NONBLOCK, similar to definition in libc.
> The patch also includes necessary code style changes (tab to spaces) in the
> header file in the MIPS #ifdef block touched by the change.
>
> Signed-off-by: Petar Jovanovic <address@hidden>
> ---
> linux-user/socket.h | 172
> ++++++++++++++++++++++++++------------------------
> linux-user/syscall.c | 45 ++++++++-----
> 2 files changed, 119 insertions(+), 98 deletions(-)
>
> diff --git a/linux-user/socket.h b/linux-user/socket.h
> index 339cae5..e4dfe56 100644
> --- a/linux-user/socket.h
> +++ b/linux-user/socket.h
> @@ -1,91 +1,101 @@
>
> #if defined(TARGET_MIPS)
> - // MIPS special values for constants
> -
> - /*
> - * For setsockopt(2)
> - *
> - * This defines are ABI conformant as far as Linux supports these ...
> - */
> - #define TARGET_SOL_SOCKET 0xffff
> -
> - #define TARGET_SO_DEBUG 0x0001 /* Record debugging information.
> */
> - #define TARGET_SO_REUSEADDR 0x0004 /* Allow reuse of local
> addresses. */
> - #define TARGET_SO_KEEPALIVE 0x0008 /* Keep connections alive and
> send
> - SIGPIPE when they die. */
> - #define TARGET_SO_DONTROUTE 0x0010 /* Don't do local routing. */
> - #define TARGET_SO_BROADCAST 0x0020 /* Allow transmission of
> - broadcast messages. */
> - #define TARGET_SO_LINGER 0x0080 /* Block on close of a reliable
> - socket to transmit pending data. */
> - #define TARGET_SO_OOBINLINE 0x0100 /* Receive out-of-band data
> in-band. */
> - #if 0
> - To add: #define TARGET_SO_REUSEPORT 0x0200 /* Allow local address
> and port reuse. */
> - #endif
> -
> - #define TARGET_SO_TYPE 0x1008 /* Compatible name for SO_STYLE.
> */
> - #define TARGET_SO_STYLE SO_TYPE /* Synonym */
> - #define TARGET_SO_ERROR 0x1007 /* get error status and clear */
> - #define TARGET_SO_SNDBUF 0x1001 /* Send buffer size. */
> - #define TARGET_SO_RCVBUF 0x1002 /* Receive buffer. */
> - #define TARGET_SO_SNDLOWAT 0x1003 /* send low-water mark */
> - #define TARGET_SO_RCVLOWAT 0x1004 /* receive low-water mark */
> - #define TARGET_SO_SNDTIMEO 0x1005 /* send timeout */
> - #define TARGET_SO_RCVTIMEO 0x1006 /* receive timeout */
> - #define TARGET_SO_ACCEPTCONN 0x1009
> -
> - /* linux-specific, might as well be the same as on i386 */
> - #define TARGET_SO_NO_CHECK 11
> - #define TARGET_SO_PRIORITY 12
> - #define TARGET_SO_BSDCOMPAT 14
> + /* MIPS special values for constants */
> +
> + /*
> + * For setsockopt(2)
> + *
> + * This defines are ABI conformant as far as Linux supports these ...
> + */
> + #define TARGET_SOL_SOCKET 0xffff
> +
> + #define TARGET_SO_DEBUG 0x0001 /* Record debugging information.
> */
> + #define TARGET_SO_REUSEADDR 0x0004 /* Allow reuse of local
> addresses. */
> + #define TARGET_SO_KEEPALIVE 0x0008 /* Keep connections alive and send
> + SIGPIPE when they die. */
> + #define TARGET_SO_DONTROUTE 0x0010 /* Don't do local routing. */
> + #define TARGET_SO_BROADCAST 0x0020 /* Allow transmission of
> + broadcast messages. */
> + #define TARGET_SO_LINGER 0x0080 /* Block on close of a reliable
> + * socket to transmit pending
> data.
> + */
> + #define TARGET_SO_OOBINLINE 0x0100 /* Receive out-of-band data
> in-band.
> + */
> + #if 0
> + /* To add: Allow local address and port reuse. */
> + #define TARGET_SO_REUSEPORT 0x0200
> + #endif
> +
> + #define TARGET_SO_TYPE 0x1008 /* Compatible name for SO_STYLE.
> */
> + #define TARGET_SO_STYLE SO_TYPE /* Synonym */
> + #define TARGET_SO_ERROR 0x1007 /* get error status and clear */
> + #define TARGET_SO_SNDBUF 0x1001 /* Send buffer size. */
> + #define TARGET_SO_RCVBUF 0x1002 /* Receive buffer. */
> + #define TARGET_SO_SNDLOWAT 0x1003 /* send low-water mark */
> + #define TARGET_SO_RCVLOWAT 0x1004 /* receive low-water mark */
> + #define TARGET_SO_SNDTIMEO 0x1005 /* send timeout */
> + #define TARGET_SO_RCVTIMEO 0x1006 /* receive timeout */
> + #define TARGET_SO_ACCEPTCONN 0x1009
>
> - #define TARGET_SO_PASSCRED 17
> - #define TARGET_SO_PEERCRED 18
> + /* linux-specific, might as well be the same as on i386 */
> + #define TARGET_SO_NO_CHECK 11
> + #define TARGET_SO_PRIORITY 12
> + #define TARGET_SO_BSDCOMPAT 14
>
> - /* Security levels - as per NRL IPv6 - don't actually do anything */
> - #define TARGET_SO_SECURITY_AUTHENTICATION 22
> - #define TARGET_SO_SECURITY_ENCRYPTION_TRANSPORT 23
> - #define TARGET_SO_SECURITY_ENCRYPTION_NETWORK 24
> + #define TARGET_SO_PASSCRED 17
> + #define TARGET_SO_PEERCRED 18
> +
> + /* Security levels - as per NRL IPv6 - don't actually do anything */
> + #define TARGET_SO_SECURITY_AUTHENTICATION 22
> + #define TARGET_SO_SECURITY_ENCRYPTION_TRANSPORT 23
> + #define TARGET_SO_SECURITY_ENCRYPTION_NETWORK 24
>
> - #define TARGET_SO_BINDTODEVICE 25
> + #define TARGET_SO_BINDTODEVICE 25
>
> - /* Socket filtering */
> - #define TARGET_SO_ATTACH_FILTER 26
> - #define TARGET_SO_DETACH_FILTER 27
> + /* Socket filtering */
> + #define TARGET_SO_ATTACH_FILTER 26
> + #define TARGET_SO_DETACH_FILTER 27
>
> - #define TARGET_SO_PEERNAME 28
> - #define TARGET_SO_TIMESTAMP 29
> - #define SCM_TIMESTAMP SO_TIMESTAMP
> -
> - #define TARGET_SO_PEERSEC 30
> - #define TARGET_SO_SNDBUFFORCE 31
> - #define TARGET_SO_RCVBUFFORCE 33
> -
> - /** sock_type - Socket types
> - *
> - * Please notice that for binary compat reasons MIPS has to
> - * override the enum sock_type in include/linux/net.h, so
> - * we define ARCH_HAS_SOCKET_TYPES here.
> - *
> - * @SOCK_DGRAM - datagram (conn.less) socket
> - * @SOCK_STREAM - stream (connection) socket
> - * @SOCK_RAW - raw socket
> - * @SOCK_RDM - reliably-delivered message
> - * @SOCK_SEQPACKET - sequential packet socket
> - * @SOCK_PACKET - linux specific way of getting packets at the dev
> level.
> - * For writing rarp and other similar things on the user
> level.
> - */
> - enum sock_type {
> - TARGET_SOCK_DGRAM = 1,
> - TARGET_SOCK_STREAM = 2,
> - TARGET_SOCK_RAW = 3,
> - TARGET_SOCK_RDM = 4,
> - TARGET_SOCK_SEQPACKET = 5,
> - TARGET_SOCK_DCCP = 6,
> - TARGET_SOCK_PACKET = 10,
> - };
> -
> - #define TARGET_SOCK_MAX (SOCK_PACKET + 1)
> + #define TARGET_SO_PEERNAME 28
> + #define TARGET_SO_TIMESTAMP 29
> + #define SCM_TIMESTAMP SO_TIMESTAMP
> +
> + #define TARGET_SO_PEERSEC 30
> + #define TARGET_SO_SNDBUFFORCE 31
> + #define TARGET_SO_RCVBUFFORCE 33
> +
> + /** sock_type - Socket types
> + *
> + * Please notice that for binary compat reasons MIPS has to
> + * override the enum sock_type in include/linux/net.h, so
> + * we define ARCH_HAS_SOCKET_TYPES here.
> + *
Not related to your changes, but note that ARCH_HAS_SOCKET_TYPES is not
defined anywhere contrary to what is said in the comment. It might be a
good idea to revive it, as at least alpha and sparc also have the issue.
> + * @SOCK_DGRAM - datagram (conn.less) socket
> + * @SOCK_STREAM - stream (connection) socket
> + * @SOCK_RAW - raw socket
> + * @SOCK_RDM - reliably-delivered message
> + * @SOCK_SEQPACKET - sequential packet socket
> + * @SOCK_DCCP - Datagram Congestion Control Protocol socket
> + * @SOCK_PACKET - linux specific way of getting packets at the dev level.
> + * For writing rarp and other similar things on the user
> + * level.
> + * @SOCK_CLOEXEC - sets the close-on-exec (FD_CLOEXEC) flag.
> + * @SOCK_NONBLOCK - sets the O_NONBLOCK file status flag.
> + */
> + enum sock_type {
> + TARGET_SOCK_DGRAM = 1,
> + TARGET_SOCK_STREAM = 2,
> + TARGET_SOCK_RAW = 3,
> + TARGET_SOCK_RDM = 4,
> + TARGET_SOCK_SEQPACKET = 5,
> + TARGET_SOCK_DCCP = 6,
> + TARGET_SOCK_PACKET = 10,
> + TARGET_SOCK_CLOEXEC = 02000000,
> + TARGET_SOCK_NONBLOCK = 0200,
> + };
> +
> + #define TARGET_SOCK_MAX (TARGET_SOCK_PACKET + 1)
> + #define TARGET_SOCK_TYPE_MASK 0xf /* Covers up to TARGET_SOCK_MAX-1.
> */
>
> #elif defined(TARGET_ALPHA)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index ee82a2d..0a9e6db 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1859,30 +1859,38 @@ static void unlock_iovec(struct iovec *vec, abi_ulong
> target_addr,
> free(vec);
> }
>
> -/* do_socket() Must return target values and target errnos. */
> -static abi_long do_socket(int domain, int type, int protocol)
> -{
> #if defined(TARGET_MIPS)
> - switch(type) {
> +static inline void target_to_host_sock_type(int *type)
> +{
> + int host_type = 0;
> + int target_type = *type;
> +
> + switch (target_type & TARGET_SOCK_TYPE_MASK) {
> case TARGET_SOCK_DGRAM:
> - type = SOCK_DGRAM;
> + host_type = SOCK_DGRAM;
> break;
> case TARGET_SOCK_STREAM:
> - type = SOCK_STREAM;
> + host_type = SOCK_STREAM;
> break;
> - case TARGET_SOCK_RAW:
> - type = SOCK_RAW;
> - break;
> - case TARGET_SOCK_RDM:
> - type = SOCK_RDM;
> - break;
> - case TARGET_SOCK_SEQPACKET:
> - type = SOCK_SEQPACKET;
> - break;
> - case TARGET_SOCK_PACKET:
> - type = SOCK_PACKET;
> + default:
> + host_type = target_type & TARGET_SOCK_TYPE_MASK;
> break;
> }
I am not sure we want to really copy the type without more checking than
the mask: a new value still within the mask limit could be added
differently depending on the architecture.
> + if (target_type & TARGET_SOCK_CLOEXEC) {
> + host_type |= SOCK_CLOEXEC;
> + }
> + if (target_type & TARGET_SOCK_NONBLOCK) {
> + host_type |= SOCK_NONBLOCK;
> + }
> + *type = host_type;
Also I think the values should be checked in all cases returning -EINVAL
if not supported. On other architecture where no translation is done,
this is done by the host kernel. With the code above, unsupported
arguments are simply ignored.
> +}
> +#endif
> +
> +/* do_socket() Must return target values and target errnos. */
> +static abi_long do_socket(int domain, int type, int protocol)
> +{
> +#if defined(TARGET_MIPS)
> + target_to_host_sock_type(&type);
> #endif
As said above this could probably be conditional on
ARCH_HAS_SOCKET_TYPES to make it more generic.
> if (domain == PF_NETLINK)
> return -EAFNOSUPPORT; /* do not NETLINK socket connections possible
> */
> @@ -2116,6 +2124,9 @@ static abi_long do_socketpair(int domain, int type, int
> protocol,
> int tab[2];
> abi_long ret;
>
> +#if defined(TARGET_MIPS)
> + target_to_host_sock_type(&type);
> +#endif
> ret = get_errno(socketpair(domain, type, protocol, tab));
> if (!is_error(ret)) {
> if (put_user_s32(tab[0], target_tab_addr)
> --
> 1.7.9.5
>
>
--
Aurelien Jarno GPG: 1024D/F1BCDB73
address@hidden http://www.aurel32.net