[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] linux-user: improve target_to_host_sock_type
|
From: |
Riku Voipio |
|
Subject: |
Re: [Qemu-devel] [PATCH v2] linux-user: improve target_to_host_sock_type conversion |
|
Date: |
Thu, 30 May 2013 16:28:11 +0300 |
Hi,
On 27 May 2013 13:49, Petar Jovanovic <address@hidden> wrote:
> Can anyone take a look at this and commit it if there are no other change
> requests?
I thought Aurelian had an issue with this patch, but it seems you explained your
position well. I'll get this included in my next pull.
Riku
> ________________________________________
> From: Petar Jovanovic
> Sent: Wednesday, May 08, 2013 1:16 AM
> To: address@hidden; address@hidden
> Cc: Aurelien Jarno; Petar Jovanovic; address@hidden; address@hidden;
> Alexander Graf; Andreas Färber
> Subject: RE: [Qemu-devel] [PATCH v2] linux-user: improve
> target_to_host_sock_type conversion
>
> ping
>
> http://patchwork.ozlabs.org/patch/232770/
> ________________________________________
> From: Petar Jovanovic
> Sent: Tuesday, April 30, 2013 3:20 AM
> To: Andreas Färber
> Cc: Aurelien Jarno; Petar Jovanovic; address@hidden; address@hidden;
> address@hidden; address@hidden; Alexander Graf
> Subject: RE: [Qemu-devel] [PATCH v2] linux-user: improve
> target_to_host_sock_type conversion
>
> My assumption was that if new socket values are added in future, they will
> likely be the same for all platforms, so they can be supported without
> adding new lines of code. Here we convert the values known to be different,
> we leave the values known to be the same, and for the incorrect values - we
> pass them to (host) kernel as they are in belief that kernel would return
> error. We could handle that part and check for incorrect value before
> passing it to the kernel, but in that case we bring more kernel logic to
> QEMU for the cases that are not likely to happen.
>
> Petar
> ________________________________________
> From: Andreas Färber address@hidden
> Sent: Monday, April 29, 2013 4:56 PM
> To: Petar Jovanovic
> Cc: Aurelien Jarno; Petar Jovanovic; address@hidden; address@hidden;
> address@hidden; address@hidden; Alexander Graf
> Subject: Re: [Qemu-devel] [PATCH v2] linux-user: improve
> target_to_host_sock_type conversion
>
> Am 23.04.2013 02:15, schrieb Petar Jovanovic:
>> Thanks Aurelien for your comments.
>>
>> What others think? Can we submit this version of the patch? Riku? Richard,
>> Blue?
>
> No objection but isn't that a frequent issue that mappings may need to
> be extended from time to time? The way I've seen that handled is on a
> case by case basis mapping from one known value to another, with
> defaulting to whatever form of error reporting appropriate. Here it
> seems that some cases were dropped and we are now defaulting to taking
> the literal value where no difference is known. This may lead to silent
> errors, whereas an abort as other extreme may prohibit use cases with no
> value difference between host and target.
>
> Andreas
>
>> ________________________________________
>> From: Aurelien Jarno address@hidden
>> Sent: Monday, April 15, 2013 3:47 PM
>> To: Petar Jovanovic
>> Cc: address@hidden; Petar Jovanovic; address@hidden; address@hidden;
>> address@hidden
>> Subject: Re: [PATCH v2] linux-user: improve target_to_host_sock_type
>> conversion
>>
>> On Mon, Apr 01, 2013 at 05:49:39PM +0200, Petar Jovanovic wrote:
>>> From: Petar Jovanovic <address@hidden>
>>>
>>> Previous implementation has failed to take into account different value of
>>> SOCK_NONBLOCK on target 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.
>>> The change is valid for architectures that define ARCH_HAS_SOCKET_TYPES,
>>> these are MIPS, SPARC and ALPHA now.
>>>
>>> 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>
>>> ---
>>> v2:
>>>
>>> - the patch defines ARCH_HAS_SOCKET_TYPES for MIPS, SPARC and ALPHA
>>> - values for sock_type are defined for SPARC and ALPHA in socket.h
>>>
>>> linux-user/socket.h | 248
>>> +++++++++++++++++++++++++++++++++-----------------
>>> linux-user/syscall.c | 45 +++++----
>>> 2 files changed, 195 insertions(+), 98 deletions(-)
>>>
>>> diff --git a/linux-user/socket.h b/linux-user/socket.h
>>> index 339cae5..d2b05dc 100644
>>> --- a/linux-user/socket.h
>>> +++ b/linux-user/socket.h
>>> @@ -1,91 +1,104 @@
>>>
>>> #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
>>>
>>> - #define TARGET_SO_BINDTODEVICE 25
>>> + /* 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
>>>
>>> - /* Socket filtering */
>>> - #define TARGET_SO_ATTACH_FILTER 26
>>> - #define TARGET_SO_DETACH_FILTER 27
>>> + #define TARGET_SO_BINDTODEVICE 25
>>>
>>> - #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)
>>> + /* 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_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.
>>> + */
>>> +
>>> + #define ARCH_HAS_SOCKET_TYPES 1
>>> +
>>> + 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)
>>>
>>> @@ -156,8 +169,81 @@
>>> /* Instruct lower device to use last 4-bytes of skb data as FCS */
>>> #define TARGET_SO_NOFCS 43
>>>
>>> + /** sock_type - Socket types
>>> + *
>>> + * Please notice that for binary compat reasons ALPHA 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_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.
>>> + */
>>> +
>>> + #define ARCH_HAS_SOCKET_TYPES 1
>>> +
>>> + enum sock_type {
>>> + TARGET_SOCK_STREAM = 1,
>>> + TARGET_SOCK_DGRAM = 2,
>>> + TARGET_SOCK_RAW = 3,
>>> + TARGET_SOCK_RDM = 4,
>>> + TARGET_SOCK_SEQPACKET = 5,
>>> + TARGET_SOCK_DCCP = 6,
>>> + TARGET_SOCK_PACKET = 10,
>>> + TARGET_SOCK_CLOEXEC = 010000000,
>>> + TARGET_SOCK_NONBLOCK = 010000000000,
>>> + };
>>> +
>>> + #define TARGET_SOCK_MAX (TARGET_SOCK_PACKET + 1)
>>> + #define TARGET_SOCK_TYPE_MASK 0xf /* Covers up to
>>> TARGET_SOCK_MAX-1. */
>>> #else
>>>
>>> +#if defined(TARGET_SPARC)
>>> + /** sock_type - Socket types
>>> + *
>>> + * Please notice that for binary compat reasons SPARC 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_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.
>>> + */
>>> +
>>> + #define ARCH_HAS_SOCKET_TYPES 1
>>> +
>>> + enum sock_type {
>>> + TARGET_SOCK_STREAM = 1,
>>> + TARGET_SOCK_DGRAM = 2,
>>> + TARGET_SOCK_RAW = 3,
>>> + TARGET_SOCK_RDM = 4,
>>> + TARGET_SOCK_SEQPACKET = 5,
>>> + TARGET_SOCK_DCCP = 6,
>>> + TARGET_SOCK_PACKET = 10,
>>> + TARGET_SOCK_CLOEXEC = 020000000,
>>> + TARGET_SOCK_NONBLOCK = 040000,
>>> + };
>>> +
>>> + #define TARGET_SOCK_MAX (TARGET_SOCK_PACKET + 1)
>>> + #define TARGET_SOCK_TYPE_MASK 0xf /* Covers up to
>>> TARGET_SOCK_MAX-1. */
>>> +#endif
>>> +
>>> /* For setsockopt(2) */
>>> #define TARGET_SOL_SOCKET 1
>>>
>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>> index ee82a2d..5b87c9d 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(ARCH_HAS_SOCKET_TYPES)
>>> +static inline void target_to_host_sock_type(int *type)
>>> {
>>> -#if defined(TARGET_MIPS)
>>> - switch(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;
>>> - break;
>>> - case TARGET_SOCK_RAW:
>>> - type = SOCK_RAW;
>>> + host_type = SOCK_STREAM;
>>> 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;
>>> }
>>> + if (target_type & TARGET_SOCK_CLOEXEC) {
>>> + host_type |= SOCK_CLOEXEC;
>>> + }
>>> + if (target_type & TARGET_SOCK_NONBLOCK) {
>>> + host_type |= SOCK_NONBLOCK;
>>> + }
>>> + *type = host_type;
>>
>> The problem I see with this way of handling the flags is that if the
>> kernel later adds a new flag value, QEMU will silently drop it. It means
>> that if the glibc tries to see it will think the kernel (and here QEMU)
>> supports it (as no error is returned), and will not use to the fallback
>> code. For that each processed flag should probably be removed from
>> target_type and it should be 0 at the end of the function.
>>
>> Imagine for example SOCK_NONBLOCK is a new feature, it starts to be
>> supported by the glibc, but silently dropped by QEMU.
>>
>> That said I agree it's purely hypothetical, as no new flags has been
>> added recently or is planned to be added. We can also decide to fix that
>> when a new flag is added. What the other people (the Cc:ed one) think?
>>
>>> +}
>>> +#endif
>>> +
>>> +/* do_socket() Must return target values and target errnos. */
>>> +static abi_long do_socket(int domain, int type, int protocol)
>>> +{
>>> +#if defined(ARCH_HAS_SOCKET_TYPES)
>>> + target_to_host_sock_type(&type);
>>> #endif
>>> 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(ARCH_HAS_SOCKET_TYPES)
>>> + 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)
>>
>> Otherwise the patch looks fine.
>>
>>
>> --
>> Aurelien Jarno GPG: 1024D/F1BCDB73
>> address@hidden http://www.aurel32.net
>>
>>
>
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>