* [Qemu-devel] [PATCH v3] linux-user: improve target_to_host_sock_type conversion
@ 2013-07-01 0:44 Petar Jovanovic
2013-07-02 13:44 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: Petar Jovanovic @ 2013-07-01 0:44 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, riku.voipio, petar.jovanovic, aurelien, rth
From: Petar Jovanovic <petar.jovanovic@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.
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 since most of the file has been touched by this change.
Signed-off-by: Petar Jovanovic <petar.jovanovic@imgtec.com>
---
v3:
- define TARGET_SOCK_* for the architectures that do not define
ARCH_HAS_SOCKET_TYPES
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 | 381 +++++++++++++++++++++++++++++++++-----------------
linux-user/syscall.c | 43 +++---
2 files changed, 275 insertions(+), 149 deletions(-)
diff --git a/linux-user/socket.h b/linux-user/socket.h
index 339cae5..ae17959 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
-
- #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
-
- /* 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)
+ /* 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
+
+ /* 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
+
+ #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
+
+ /* 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,61 +169,167 @@
/* 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
- /* For setsockopt(2) */
- #define TARGET_SOL_SOCKET 1
-
- #define TARGET_SO_DEBUG 1
- #define TARGET_SO_REUSEADDR 2
- #define TARGET_SO_TYPE 3
- #define TARGET_SO_ERROR 4
- #define TARGET_SO_DONTROUTE 5
- #define TARGET_SO_BROADCAST 6
- #define TARGET_SO_SNDBUF 7
- #define TARGET_SO_RCVBUF 8
- #define TARGET_SO_SNDBUFFORCE 32
- #define TARGET_SO_RCVBUFFORCE 33
- #define TARGET_SO_KEEPALIVE 9
- #define TARGET_SO_OOBINLINE 10
- #define TARGET_SO_NO_CHECK 11
- #define TARGET_SO_PRIORITY 12
- #define TARGET_SO_LINGER 13
- #define TARGET_SO_BSDCOMPAT 14
- /* To add :#define TARGET_SO_REUSEPORT 15 */
+#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
+
+ #define TARGET_SO_DEBUG 1
+ #define TARGET_SO_REUSEADDR 2
+ #define TARGET_SO_TYPE 3
+ #define TARGET_SO_ERROR 4
+ #define TARGET_SO_DONTROUTE 5
+ #define TARGET_SO_BROADCAST 6
+ #define TARGET_SO_SNDBUF 7
+ #define TARGET_SO_RCVBUF 8
+ #define TARGET_SO_SNDBUFFORCE 32
+ #define TARGET_SO_RCVBUFFORCE 33
+ #define TARGET_SO_KEEPALIVE 9
+ #define TARGET_SO_OOBINLINE 10
+ #define TARGET_SO_NO_CHECK 11
+ #define TARGET_SO_PRIORITY 12
+ #define TARGET_SO_LINGER 13
+ #define TARGET_SO_BSDCOMPAT 14
+ /* To add :#define TARGET_SO_REUSEPORT 15 */
#if defined(TARGET_PPC)
- #define TARGET_SO_RCVLOWAT 16
- #define TARGET_SO_SNDLOWAT 17
- #define TARGET_SO_RCVTIMEO 18
- #define TARGET_SO_SNDTIMEO 19
- #define TARGET_SO_PASSCRED 20
- #define TARGET_SO_PEERCRED 21
+ #define TARGET_SO_RCVLOWAT 16
+ #define TARGET_SO_SNDLOWAT 17
+ #define TARGET_SO_RCVTIMEO 18
+ #define TARGET_SO_SNDTIMEO 19
+ #define TARGET_SO_PASSCRED 20
+ #define TARGET_SO_PEERCRED 21
#else
- #define TARGET_SO_PASSCRED 16
- #define TARGET_SO_PEERCRED 17
- #define TARGET_SO_RCVLOWAT 18
- #define TARGET_SO_SNDLOWAT 19
- #define TARGET_SO_RCVTIMEO 20
- #define TARGET_SO_SNDTIMEO 21
+ #define TARGET_SO_PASSCRED 16
+ #define TARGET_SO_PEERCRED 17
+ #define TARGET_SO_RCVLOWAT 18
+ #define TARGET_SO_SNDLOWAT 19
+ #define TARGET_SO_RCVTIMEO 20
+ #define TARGET_SO_SNDTIMEO 21
#endif
- /* 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
+ /* 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 TARGET_SCM_TIMESTAMP TARGET_SO_TIMESTAMP
- #define TARGET_SO_PEERNAME 28
- #define TARGET_SO_TIMESTAMP 29
- #define TARGET_SCM_TIMESTAMP TARGET_SO_TIMESTAMP
+ #define TARGET_SO_ACCEPTCONN 30
- #define TARGET_SO_ACCEPTCONN 30
+ #define TARGET_SO_PEERSEC 31
+
+#endif
- #define TARGET_SO_PEERSEC 31
+#ifndef ARCH_HAS_SOCKET_TYPES
+ /** sock_type - Socket types - default values
+ *
+ *
+ * @SOCK_STREAM - stream (connection) socket
+ * @SOCK_DGRAM - datagram (conn.less) 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_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 = 02000000,
+ TARGET_SOCK_NONBLOCK = 04000,
+ };
+
+ #define TARGET_SOCK_MAX (TARGET_SOCK_PACKET + 1)
+ #define TARGET_SOCK_TYPE_MASK 0xf /* Covers up to TARGET_SOCK_MAX-1. */
#endif
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index cdd0c28..a7ad018 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1696,31 +1696,36 @@ 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)
+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;
}
-#endif
+ if (target_type & TARGET_SOCK_CLOEXEC) {
+ host_type |= SOCK_CLOEXEC;
+ }
+ if (target_type & TARGET_SOCK_NONBLOCK) {
+ host_type |= SOCK_NONBLOCK;
+ }
+ *type = host_type;
+}
+
+/* do_socket() Must return target values and target errnos. */
+static abi_long do_socket(int domain, int type, int protocol)
+{
+ target_to_host_sock_type(&type);
+
if (domain == PF_NETLINK)
return -EAFNOSUPPORT; /* do not NETLINK socket connections possible */
return get_errno(socket(domain, type, protocol));
@@ -1953,6 +1958,8 @@ static abi_long do_socketpair(int domain, int type, int protocol,
int tab[2];
abi_long ret;
+ target_to_host_sock_type(&type);
+
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] linux-user: improve target_to_host_sock_type conversion
2013-07-01 0:44 [Qemu-devel] [PATCH v3] linux-user: improve target_to_host_sock_type conversion Petar Jovanovic
@ 2013-07-02 13:44 ` Peter Maydell
2013-07-02 14:13 ` Petar Jovanovic
2013-07-08 9:12 ` Petar Jovanovic
0 siblings, 2 replies; 6+ messages in thread
From: Peter Maydell @ 2013-07-02 13:44 UTC (permalink / raw)
To: Petar Jovanovic; +Cc: riku.voipio, rth, qemu-devel, aurelien, petar.jovanovic
On 1 July 2013 01:44, Petar Jovanovic <petar.jovanovic@rt-rk.com> wrote:
> The patch also includes necessary code style changes (tab to spaces) in the
> header file since most of the file has been touched by this change.
This makes it pretty painful to review because of all the "not actually
changing anything" changes. If you want to fix up indent on an entire
file that would be better as a single patch on its own before this
one; then it's easy to check that that has no actual code change,
and the patch with the code changes is smaller and easier to read.
(The general rule of thumb is "if you're just changing a few lines
of indent/braces in lines that the patch touches anyway" that's fine;
larger scale changes go in their own patch if we do them at all.)
Not using the same base as the kernel (hex vs octal) made checking
the values a bit tedious too, but I think they're all correct.
That said,
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] linux-user: improve target_to_host_sock_type conversion
2013-07-02 13:44 ` Peter Maydell
@ 2013-07-02 14:13 ` Petar Jovanovic
2013-07-02 16:34 ` Peter Maydell
2013-07-08 9:12 ` Petar Jovanovic
1 sibling, 1 reply; 6+ messages in thread
From: Petar Jovanovic @ 2013-07-02 14:13 UTC (permalink / raw)
To: Peter Maydell, Petar Jovanovic; +Cc: riku.voipio, qemu-devel, aurelien, rth
________________________________________
From: Peter Maydell [peter.maydell@linaro.org]
Sent: Tuesday, July 02, 2013 3:44 PM
To: Petar Jovanovic
Cc: qemu-devel@nongnu.org; Petar Jovanovic; aurelien@aurel32.net; riku.voipio@linaro.org; rth@twiddle.net
Subject: Re: [PATCH v3] linux-user: improve target_to_host_sock_type conversion
On 1 July 2013 01:44, Petar Jovanovic <petar.jovanovic@rt-rk.com> wrote:
> The patch also includes necessary code style changes (tab to spaces) in the
> header file since most of the file has been touched by this change.
Thanks for the review, Peter.
> (The general rule of thumb is "if you're just changing a few lines
> of indent/braces in lines that the patch touches anyway" that's fine;
> larger scale changes go in their own patch if we do them at all.)
I am aware of this, and I originally tried to make these changes as small
as possible. Check for the previous two versions of the patch.
The commit message originally said:
"The patch also includes necessary code style changes (tab to spaces) in the
header file in the MIPS #ifdef block touched by the change."
Yet, I ended up changing 80% of the header file with this version, so I
was under impression it makes sense to change the remaining 20%.
> Not using the same base as the kernel (hex vs octal) made checking
> the values a bit tedious too, but I think they're all correct.
What values are you refering to?
I actually tried to copy the values from the kernel in the same format:
O_NONBLOCK 00004000
http://lxr.free-electrons.com/source/include/uapi/asm-generic/fcntl.h#L37
O_CLOEXEC 02000000
http://lxr.free-electrons.com/source/include/uapi/asm-generic/fcntl.h#L61
all socket type values:
http://lxr.free-electrons.com/source/include/linux/net.h#L58
enum sock_type {
SOCK_STREAM = 1,
SOCK_DGRAM = 2,
SOCK_RAW = 3,
SOCK_RDM = 4,
SOCK_SEQPACKET = 5,
SOCK_DCCP = 6,
SOCK_PACKET = 10,
};
#define SOCK_TYPE_MASK 0xf
Petar
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] linux-user: improve target_to_host_sock_type conversion
2013-07-02 14:13 ` Petar Jovanovic
@ 2013-07-02 16:34 ` Peter Maydell
2013-07-02 21:43 ` Petar Jovanovic
0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2013-07-02 16:34 UTC (permalink / raw)
To: Petar Jovanovic; +Cc: riku.voipio, rth, Petar Jovanovic, aurelien, qemu-devel
On 2 July 2013 15:13, Petar Jovanovic <Petar.Jovanovic@imgtec.com> wrote:
> Yet, I ended up changing 80% of the header file with this version, so I
> was under impression it makes sense to change the remaining 20%.
Yeah, that's fine -- but once you get to that scale of change it's
nicer to have patch 1/2 fix formatting, patch 2/2 actual change.
(Just a note for next time at this point, though.)
>> Not using the same base as the kernel (hex vs octal) made checking
>> the values a bit tedious too, but I think they're all correct.
>
> What values are you refering to?
arch/sparc/include/uapi/asm/fcntl.h:#define O_CLOEXEC 0x400000
arch/alpha/include/asm/socket.h:#define SOCK_NONBLOCK 0x40000000
arch/mips/include/uapi/asm/fcntl.h:#define O_NONBLOCK 0x0080
arch/sparc/include/uapi/asm/fcntl.h:#define O_NONBLOCK 0x4000
thanks
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] linux-user: improve target_to_host_sock_type conversion
2013-07-02 16:34 ` Peter Maydell
@ 2013-07-02 21:43 ` Petar Jovanovic
0 siblings, 0 replies; 6+ messages in thread
From: Petar Jovanovic @ 2013-07-02 21:43 UTC (permalink / raw)
To: Peter Maydell; +Cc: riku.voipio, rth, Petar Jovanovic, aurelien, qemu-devel
________________________________________
From: Peter Maydell [peter.maydell@linaro.org]
Sent: Tuesday, July 02, 2013 6:34 PM
To: Petar Jovanovic
Cc: Petar Jovanovic; qemu-devel@nongnu.org; aurelien@aurel32.net; riku.voipio@linaro.org; rth@twiddle.net
Subject: Re: [PATCH v3] linux-user: improve target_to_host_sock_type conversion
>> What values are you refering to?
> arch/sparc/include/uapi/asm/fcntl.h:#define O_CLOEXEC 0x400000
> arch/alpha/include/asm/socket.h:#define SOCK_NONBLOCK 0x40000000
> arch/mips/include/uapi/asm/fcntl.h:#define O_NONBLOCK 0x0080
> arch/sparc/include/uapi/asm/fcntl.h:#define O_NONBLOCK 0x4000
I see, there have been some exceptions.
I had to make some calls as the kernel itself is inconsistent, and I hoped
that consistency over socket.h would be helpful.
Regards,
Petar
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] linux-user: improve target_to_host_sock_type conversion
2013-07-02 13:44 ` Peter Maydell
2013-07-02 14:13 ` Petar Jovanovic
@ 2013-07-08 9:12 ` Petar Jovanovic
1 sibling, 0 replies; 6+ messages in thread
From: Petar Jovanovic @ 2013-07-08 9:12 UTC (permalink / raw)
To: Peter Maydell, Petar Jovanovic; +Cc: riku.voipio, qemu-devel, aurelien, rth
ping
http://patchwork.ozlabs.org/patch/255998/
Riku, can you integrate this change?
Thank you.
Petar
________________________________________
From: Peter Maydell [peter.maydell@linaro.org]
Sent: Tuesday, July 02, 2013 3:44 PM
To: Petar Jovanovic
Cc: qemu-devel@nongnu.org; Petar Jovanovic; aurelien@aurel32.net; riku.voipio@linaro.org; rth@twiddle.net
Subject: Re: [PATCH v3] linux-user: improve target_to_host_sock_type conversion
On 1 July 2013 01:44, Petar Jovanovic <petar.jovanovic@rt-rk.com> wrote:
> The patch also includes necessary code style changes (tab to spaces) in the
> header file since most of the file has been touched by this change.
This makes it pretty painful to review because of all the "not actually
changing anything" changes. If you want to fix up indent on an entire
file that would be better as a single patch on its own before this
one; then it's easy to check that that has no actual code change,
and the patch with the code changes is smaller and easier to read.
(The general rule of thumb is "if you're just changing a few lines
of indent/braces in lines that the patch touches anyway" that's fine;
larger scale changes go in their own patch if we do them at all.)
Not using the same base as the kernel (hex vs octal) made checking
the values a bit tedious too, but I think they're all correct.
That said,
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-07-08 9:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-01 0:44 [Qemu-devel] [PATCH v3] linux-user: improve target_to_host_sock_type conversion Petar Jovanovic
2013-07-02 13:44 ` Peter Maydell
2013-07-02 14:13 ` Petar Jovanovic
2013-07-02 16:34 ` Peter Maydell
2013-07-02 21:43 ` Petar Jovanovic
2013-07-08 9:12 ` Petar Jovanovic
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.