| Message ID | 20170919230643.87425-2-carenas@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] linux-user: fix TARGET_SO_LINGER for sparc | expand |
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/
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;