linux-user: syscall: Add SO_LINGER for setsockopt
PATCH v3 1/1 linux-user: syscall: Add SO_LINGER for setsockopt
Original implementation from Chen Gang; code moved around as per v2
--- linux-user/syscall.c | 16 ++++++++++++++++ linux-user/syscall_defs.h | 5 +++++ 2 files changed, 21 insertions(+)
linux-user/syscall.c +16 -0
@@ -2829,6 +2829,8 @@ static abi_long do_setsockopt(int sockfd, int level, int optname,int val;struct ip_mreqn *ip_mreq;struct ip_mreq_source *ip_mreq_source; +struct linger lg; +struct target_linger *tlg;switch (level) {case SOL_TCP :
@@ -3071,6 +3073,20 @@ set_timeout:unlock_user (dev_ifname, optval_addr,0 );return ret; } +case TARGET_SO_LINGER : + optname =SO_LINGER ; +if (optlen !=sizeof (struct target_linger )) { +return -TARGET_EINVAL ; + } +if (!lock_user_struct (VERIFY_READ , tlg, optval_addr,1 )) { +return -TARGET_EFAULT ; + } +__get_user (lg.l_onoff, &tlg->l_onoff); +__get_user (lg.l_linger, &tlg->l_linger); + ret =get_errno (setsockopt (sockfd,SOL_SOCKET , optname, + &lg,sizeof (lg))); +unlock_user_struct (tlg, optval_addr,0 ); +return ret;/* Options with 'int' argument. */ case TARGET_SO_DEBUG : optname =SO_DEBUG ;
linux-user/syscall_defs.h +5 -0
@@ -202,6 +202,11 @@ struct target_ip_mreq_source {uint32_t imr_sourceaddr; }; +struct target_linger { +abi_int l_onoff;/* Linger active */ +abi_int l_linger;/* How long to linger for */ +}; +struct target_timeval {abi_long tv_sec;abi_long tv_usec;
Signature · 1 line
2.11.0 (Apple Git-81)
Le 19/09/2017 à 10:15, Carlo Marcelo Arenas Belón a écrit :
Quoted 31 lines
Original implementation from Chen Gang; code moved around as per v2
Signed-off-by Chen Gang <[email protected]>Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>--- linux-user/syscall.c | 16 ++++++++++++++++ linux-user/syscall_defs.h | 5 +++++ 2 files changed, 21 insertions(+)linux-user/syscall.c +7
-0@@ -2829,6 +2829,8 @@ static abi_long do_setsockopt(int sockfd, int level, int optname,int val;struct ip_mreqn *ip_mreq;struct ip_mreq_source *ip_mreq_source; +struct linger lg; +struct target_linger *tlg;switch (level) {case SOL_TCP :@@ -3071,6 +3073,20 @@ set_timeout:unlock_user (dev_ifname, optval_addr,0 );return ret; } +case TARGET_SO_LINGER : + optname =SO_LINGER ; +if (optlen !=sizeof (struct target_linger )) { +return -TARGET_EINVAL ; + }
linux-user/syscall.c set_timeout: · line 3090
Quoted 10 lines
linux-user/syscall.c +9
-0+if (!lock_user_struct (VERIFY_READ , tlg, optval_addr,1 )) { +return -TARGET_EFAULT ; + } +__get_user (lg.l_onoff, &tlg->l_onoff); +__get_user (lg.l_linger, &tlg->l_linger); + ret =get_errno (setsockopt (sockfd,SOL_SOCKET , optname, + &lg,sizeof (lg))); +unlock_user_struct (tlg, optval_addr,0 ); +return ret;/* Options with 'int' argument. */
It looks good.
Could you also add it for getsockopt()?
Quoted 9 lines
linux-user/syscall.c +0
-0case TARGET_SO_DEBUG : optname =SO_DEBUG ;linux-user/syscall_defs.h +0
-0@@ -202,6 +202,11 @@ struct target_ip_mreq_source {uint32_t imr_sourceaddr;
linux-user/syscall_defs.h struct target_ip_mreq_source { · line 212
Quoted 10 lines
linux-user/syscall_defs.h +5
-0}; +struct target_linger { +abi_int l_onoff;/* Linger active */ +abi_int l_linger;/* How long to linger for */ +}; +struct target_timeval {abi_long tv_sec;abi_long tv_usec;
Could you update the definition of TARGET_SO_LINGER for sparc (kernel says 0x0080, and we use 13)
Thanks,
Laurent
PATCH 1/3 linux-user: fix TARGET_SO_LINGER for sparc
--- linux-user/socket.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
linux-user/socket.h +5 -1
@@ -246,8 +246,12 @@#define TARGET_SOCK_MAX (TARGET_SOCK_PACKET + 1)#define TARGET_SOCK_TYPE_MASK 0xf/* Covers up to TARGET_SOCK_MAX-1. */ +#define TARGET_SO_LINGER 0x0080 +#define TARGET_SO_PASSSEC 31#else +#define TARGET_SO_LINGER 13 +#define TARGET_SO_PASSSEC 34#endif
@@ -268,7 +272,7 @@#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 )
Signature · 1 line
2.14.1
PATCH 2/3 linux-user: add SO_LINGER to setsockopt
Original implementation by Chen Gang; all bugs mine
--- linux-user/syscall.c | 15 +++++++++++++++ linux-user/syscall_defs.h | 5 +++++ 2 files changed, 20 insertions(+)
linux-user/syscall.c +15 -0
@@ -3071,6 +3071,21 @@ set_timeout:unlock_user (dev_ifname, optval_addr,0 );return ret; } +case TARGET_SO_LINGER : + { +struct linger lg; +struct target_linger *tlg; + +if (!lock_user_struct (VERIFY_READ , tlg, optval_addr,1 )) { +return -TARGET_EFAULT ; + } +__get_user (lg.l_onoff, &tlg->l_onoff); +__get_user (lg.l_linger, &tlg->l_linger); + ret =get_errno (setsockopt (sockfd,SOL_SOCKET ,SO_LINGER , + &lg, optlen)); +unlock_user_struct (tlg, optval_addr,0 ); +return ret; + }/* Options with 'int' argument. */ case TARGET_SO_DEBUG : optname =SO_DEBUG ;
linux-user/syscall_defs.h +5 -0
@@ -202,6 +202,11 @@ struct target_ip_mreq_source {uint32_t imr_sourceaddr; }; +struct target_linger { +abi_int l_onoff;/* Linger active */ +abi_int l_linger;/* How long to linger for */ +}; +struct target_timeval {abi_long tv_sec;abi_long tv_usec;
Signature · 1 line
2.14.1
Re: [Qemu-devel] [PATCH 2/3] linux-user: add SO_LINGER to setsockopt
Le 20/09/2017 à 01:06, Carlo Marcelo Arenas Belón a écrit :
Quoted 14 lines
Original implementation by Chen Gang; all bugs mine
Signed-off-by Chen Gang <[email protected]>Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>--- linux-user/syscall.c | 15 +++++++++++++++ linux-user/syscall_defs.h | 5 +++++ 2 files changed, 20 insertions(+)linux-user/syscall.c +0
-0
linux-user/syscall.c set_timeout: · line 3078
Quoted 8 lines
linux-user/syscall.c +5
-0@@ -3071,6 +3071,21 @@ set_timeout:unlock_user (dev_ifname, optval_addr,0 );return ret; } +case TARGET_SO_LINGER : + { +struct linger lg; +struct target_linger *tlg; +
Why did you remove "optname = SO_LINGER" and "if (optlen != sizeof(struct target_linger))"?
Thanks,
Laurent
Re: [Qemu-devel] [PATCH 2/3] linux-user: add SO_LINGER to setsockopt
Quoted 4 lines · Laurent Vivier
Why did you remove "optname =
SO_LINGER" and "if (optlen !=sizeof(struct target_linger))"?
the optname assignment is not really needed, since it is only used for the setsockopt call and that call is clearer using SO_LINGER directly, so to avoid hard to see bugs like :
http://lists.nongnu.org/archive/html/qemu-devel/2016-01/msg00980.html
the test for optlen is replaced by passing optlen to the underlying setsockopt call directly, who would do the test and return the right error.
as an interesting note, I noticed when testing (in ubuntu artful x86_64) that regardless of how you interpret the documentation, setsockopt won't fail just because the len is smaller than the size of the struct, and therefore that code was not equivalent to the setsockopt it was trying to emulate, and therefore this change doesn't only make the code simpler but also more correct IMHO
Carlo
Re: [Qemu-devel] [PATCH 2/3] linux-user: add SO_LINGER to setsockopt
Le 20/09/2017 à 19:29, Carlo Arenas a écrit :
Quoted 7 lines
On Wed, Sep 20, 2017 at 1:39 AM, Laurent Vivier <[email protected] <mailto:[email protected]>> wrote:
Why did you remove "optname = SO_LINGER" and "if (optlen != sizeof(struct target_linger))"?
Quoted 5 lines
the optname assignment is not really needed, since it is only used for the setsockopt call and that call is clearer using
SO_LINGERdirectly, so to avoid hard to see bugs like :http://lists.nongnu.org/archive/html/qemu-devel/2016-01/msg00980.html
Okay
Quoted 2 lines
the test for optlen is replaced by passing optlen to the underlying setsockopt call directly, who would do the test and return the right error.
You can't do that, because sizeof(struct linger) may be different from sizeof(struct target_linger).
Quoted 3 lines
as an interesting note, I noticed when testing (in ubuntu artful
x86_64) that regardless of how you interpret the documentation, setsockopt won't fail just because the len is smaller than the size of the struct, and
Right, see:
http://elixir.free-electrons.com/linux/latest/source/net/core/sock.c#L830
Quoted 3 lines
therefore that code was not equivalent to the setsockopt it was trying to emulate, and therefore this change doesn't only make the code simpler but also more correct IMHO
Next time add a revision history in your series explaining your changes (and don't reply to the previous patch series for the new series, it's better to start a new email thread).
Thanks,
Laurent
Re: [Qemu-devel] [PATCH 2/3] linux-user: add SO_LINGER to setsockopt
Quoted 7 lines · Laurent Vivier
Quoted 2 lines
the test for optlen is replaced by passing optlen to the underlying setsockopt call directly, who would do the test and return the right
error.
You can't do that, because sizeof(struct linger) may be different from
sizeof(struct target_linger).
Good point, will correct it, but considering that was mostly what I changed from 陈刚's code, could we merge his instead so I can rebase my changes on top of it?
just out of curiosity, do you know any such architecture? I assumed that for everything qemu will care, a struct with 2 ints would be 8 bytes long.
Quoted 7 lines
Quoted 3 lines
as an interesting note, I noticed when testing (in ubuntu artful
x86_64) that regardless of how you interpret the documentation, setsockopt won't fail just because the len is smaller than the size of the struct, andRight, see:
http://elixir.free-electrons.com/linux/latest/source/net/core/sock.c#L830
Sorry; got confused and the one that doesn't fail is actually getsockopt:
http://elixir.free-electrons.com/linux/latest/source/net/core/sock.c#L1178
Quoted 7 lines
therefore that code was not equivalent to the setsockopt it was trying
Quoted 2 lines
to emulate, and therefore this change doesn't only make the code simpler but also more correct IMHO
Next time add a revision history in your series explaining your changes (and don't reply to the previous patch series for the new series, it's better to start a new email thread).
Sorry about that, my original intent was to get the original submission to add support of SO_LINGER to setsockopt out of patchwork limbo[1], hence the threading and inherited CC
I see there is a lot more work to be done here though, specially when I found out while trying to test my change for sparc that SOL_SOCKET was also wrong[2]
is there any testing infrastructure that could be used here to make sure that no regression is introduced?
Carlo
[1] https://patchwork.ozlabs.org/patch/565659/
[2] https://patchwork.ozlabs.org/patch/816043/
PATCH 3/3 linux-user: add SO_LINGER to getsockopt
In reply to Carlo Marcelo Arenas Belón--- linux-user/syscall.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-)
linux-user/syscall.c +33 -1
@@ -3178,7 +3178,6 @@ static abi_long do_getsockopt(int sockfd, int level, int optname, level =SOL_SOCKET ;switch (optname) {/* These don't just return a single integer */ -case TARGET_SO_LINGER :case TARGET_SO_RCVTIMEO :case TARGET_SO_SNDTIMEO :case TARGET_SO_PEERNAME :
@@ -3216,6 +3215,39 @@ static abi_long do_getsockopt(int sockfd, int level, int optname, }break ; } +case TARGET_SO_LINGER : + { +struct linger lg; +socklen_t lglen; +struct target_linger *tlg; + +if (get_user_u32 (len, optlen)) { +return -TARGET_EFAULT ; + } +if (len <0 ) { +return -TARGET_EINVAL ; + } + + lglen =sizeof (lg); + ret =get_errno (getsockopt (sockfd, level,SO_LINGER , + &lg, &lglen)); +if (ret <0 ) { +return ret; + } +if (len > lglen) { + len = lglen; + } +if (!lock_user_struct (VERIFY_WRITE , tlg, optval_addr,0 )) { +return -TARGET_EFAULT ; + } +__put_user (lg.l_onoff, &tlg->l_onoff); +__put_user (lg.l_linger, &tlg->l_linger); +unlock_user_struct (tlg, optval_addr,1 ); +if (put_user_u32 (len, optlen)) { +return -TARGET_EFAULT ; + } +break ; + }/* Options with 'int' argument. */ case TARGET_SO_DEBUG : optname =SO_DEBUG ;
Signature · 1 line
2.14.1
Re: [Qemu-devel] [PATCH 3/3] linux-user: add SO_LINGER to getsockopt
Le 20/09/2017 à 01:06, Carlo Marcelo Arenas Belón a écrit :
Quoted 48 lines
Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>--- linux-user/syscall.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-)linux-user/syscall.c +26
-1@@ -3178,7 +3178,6 @@ static abi_long do_getsockopt(int sockfd, int level, int optname, level =SOL_SOCKET ;switch (optname) {/* These don't just return a single integer */ -case TARGET_SO_LINGER :case TARGET_SO_RCVTIMEO :case TARGET_SO_SNDTIMEO :case TARGET_SO_PEERNAME :@@ -3216,6 +3215,39 @@ static abi_long do_getsockopt(int sockfd, int level, int optname, }break ; } +case TARGET_SO_LINGER : + { +struct linger lg; +socklen_t lglen; +struct target_linger *tlg; + +if (get_user_u32 (len, optlen)) { +return -TARGET_EFAULT ; + } +if (len <0 ) { +return -TARGET_EINVAL ; + } + + lglen =sizeof (lg); + ret =get_errno (getsockopt (sockfd, level,SO_LINGER , + &lg, &lglen)); +if (ret <0 ) { +return ret; + } +if (len > lglen) { + len = lglen; + } +if (!lock_user_struct (VERIFY_WRITE , tlg, optval_addr,0 )) { +return -TARGET_EFAULT ; + } +__put_user (lg.l_onoff, &tlg->l_onoff);
linux-user/syscall.c static abi_long do_getsockopt(int sockfd, int level, int optname, · line 3253
Quoted 10 lines
linux-user/syscall.c +7
-0+__put_user (lg.l_linger, &tlg->l_linger); +unlock_user_struct (tlg, optval_addr,1 ); +if (put_user_u32 (len, optlen)) { +return -TARGET_EFAULT ; + } +break ; + }/* Options with 'int' argument. */ case TARGET_SO_DEBUG : optname =SO_DEBUG ;
You should merge PATCH 2/3 and PATCH 3/3.
Re: [Qemu-devel] [PATCH 1/3] linux-user: fix TARGET_SO_LINGER for sparc
In reply to Carlo Marcelo Arenas BelónLe 20/09/2017 à 01:06, Carlo Marcelo Arenas Belón a écrit :
Quoted 11 lines
Signed-off-by Carlo Marcelo Arenas Belón <[email protected]>--- linux-user/socket.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)linux-user/socket.h +0
-0@@ -246,8 +246,12 @@#define TARGET_SOCK_MAX (TARGET_SOCK_PACKET + 1)
linux-user/socket.h line 256
Quoted 10 lines
linux-user/socket.h +4
-0#define TARGET_SOCK_TYPE_MASK 0xf/* Covers up to TARGET_SOCK_MAX-1. */ +#define TARGET_SO_LINGER 0x0080 +#define TARGET_SO_PASSSEC 31#else +#define TARGET_SO_LINGER 13 +#define TARGET_SO_PASSSEC 34#endif
The change is correct but it should be added below, in the "For setsockopt(2)" section, adding a TARGET_SPARC section, like we have a TARGET_PPC section.
linux-user/socket.h line 275
Quoted 7 lines
linux-user/socket.h +1
-1@@ -268,7 +272,7 @@#define TARGET_SO_OOBINLINE 10#define TARGET_SO_NO_CHECK 11#define TARGET_SO_PRIORITY 12 -#define TARGET_SO_LINGER 13 +
Don't add a blank line
linux-user/socket.h line 278
Quoted 4 lines
linux-user/socket.h +0
-0#define TARGET_SO_BSDCOMPAT 14/* To add :#define TARGET_SO_REUSEPORT 15 */ #if defined(TARGET_PPC )
But a better change would be to move all socket defines to a new file linux-user/sparc/sockbits.h (like for TARGET_HPPA).
Laurent