Original implementation from Chen Gang; code moved around as per v2
Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
linux-user/syscall.c | 16 ++++++++++++++++
linux-user/syscall_defs.h | 5 +++++
2 files changed, 21 insertions(+)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 9b6364a266..d3c500ca78 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -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;
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 40c5027e93..a60d6bb163 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -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;
--
2.11.0 (Apple Git-81)
Le 19/09/2017 à 10:15, Carlo Marcelo Arenas Belón a écrit :
> Original implementation from Chen Gang; code moved around as per v2
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> linux-user/syscall.c | 16 ++++++++++++++++
> linux-user/syscall_defs.h | 5 +++++
> 2 files changed, 21 insertions(+)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 9b6364a266..d3c500ca78 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -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. */
It looks good.
Could you also add it for getsockopt()?
> case TARGET_SO_DEBUG:
> optname = SO_DEBUG;
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 40c5027e93..a60d6bb163 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -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;
>
Could you update the definition of TARGET_SO_LINGER for sparc (kernel
says 0x0080, and we use 13)
Thanks,
Laurent
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
linux-user/socket.h | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/linux-user/socket.h b/linux-user/socket.h
index 7051cd2cf4..129f9b4713 100644
--- a/linux-user/socket.h
+++ b/linux-user/socket.h
@@ -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)
--
2.14.1
Le 20/09/2017 à 01:06, Carlo Marcelo Arenas Belón a écrit : > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > linux-user/socket.h | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/linux-user/socket.h b/linux-user/socket.h > index 7051cd2cf4..129f9b4713 100644 > --- a/linux-user/socket.h > +++ b/linux-user/socket.h > @@ -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 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. > > @@ -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 > #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
Original implementation by Chen Gang; all bugs mine
Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
linux-user/syscall.c | 15 +++++++++++++++
linux-user/syscall_defs.h | 5 +++++
2 files changed, 20 insertions(+)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 9b6364a266..ad689dad50 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -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;
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 40c5027e93..a60d6bb163 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -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;
--
2.14.1
Le 20/09/2017 à 01:06, Carlo Marcelo Arenas Belón a écrit :
> Original implementation by Chen Gang; all bugs mine
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> linux-user/syscall.c | 15 +++++++++++++++
> linux-user/syscall_defs.h | 5 +++++
> 2 files changed, 20 insertions(+)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 9b6364a266..ad689dad50 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -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
On Wed, Sep 20, 2017 at 1:39 AM, Laurent Vivier <laurent@vivier.eu> wrote: > > 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
Le 20/09/2017 à 19:29, Carlo Arenas a écrit : > On Wed, Sep 20, 2017 at 1:39 AM, Laurent Vivier <laurent@vivier.eu > <mailto:laurent@vivier.eu>> wrote: > > 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 Okay > 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). > 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 > 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
On Wed, Sep 20, 2017 at 11:53 AM, Laurent Vivier <laurent@vivier.eu> wrote: > > 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. > > 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 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 > 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). > 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/
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
linux-user/syscall.c | 34 +++++++++++++++++++++++++++++++++-
1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index ad689dad50..91bd27c63a 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -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;
--
2.14.1
Le 20/09/2017 à 01:06, Carlo Marcelo Arenas Belón a écrit :
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> linux-user/syscall.c | 34 +++++++++++++++++++++++++++++++++-
> 1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index ad689dad50..91bd27c63a 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -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;
>
You should merge PATCH 2/3 and PATCH 3/3.
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
© 2016 - 2025 Red Hat, Inc.