Hi, On 27 May 2013 13:49, Petar Jovanovic <petar.jovano...@imgtec.com> 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: riku.voi...@linaro.org; qemu-devel@nongnu.org > Cc: Aurelien Jarno; Petar Jovanovic; blauwir...@gmail.com; r...@twiddle.net; > 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; blauwir...@gmail.com; > riku.voi...@linaro.org; qemu-devel@nongnu.org; r...@twiddle.net; 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 [afaer...@suse.de] > Sent: Monday, April 29, 2013 4:56 PM > To: Petar Jovanovic > Cc: Aurelien Jarno; Petar Jovanovic; blauwir...@gmail.com; > riku.voi...@linaro.org; qemu-devel@nongnu.org; r...@twiddle.net; 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 [aurel...@aurel32.net] >> Sent: Monday, April 15, 2013 3:47 PM >> To: Petar Jovanovic >> Cc: qemu-devel@nongnu.org; Petar Jovanovic; riku.voi...@linaro.org; >> r...@twiddle.net; blauwir...@gmail.com >> 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 <petar.jovano...@imgtec.com> >>> >>> 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 <petar.jovano...@imgtec.com> >>> --- >>> 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 >> aurel...@aurel32.net 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 >