Re: [Qemu-devel] [PATCH 1/4] qemu-user: Impl. setsockopt(SO_BINDTODEVICE)

[Joakim Tjernlund]

Peter Maydell <address@hidden> wrote on 2014/07/11 17:46:28:

> From: Peter Maydell <address@hidden>
> To: Joakim Tjernlund <address@hidden>, 
> Cc: QEMU Developers <address@hidden>
> Date: 2014/07/11 17:46
> Subject: Re: [Qemu-devel] [PATCH 1/4] qemu-user: Impl. 
setsockopt(SO_BINDTODEVICE)
> 
> On 11 July 2014 16:18, Joakim Tjernlund <address@hidden> 
wrote:
> > Signed-off-by: Joakim Tjernlund <address@hidden>
> > ---
> >  linux-user/syscall.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index 5a272d3..1380f4e 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -1497,6 +1497,18 @@ set_timeout:
> >                  unlock_user_struct(tfprog, optval_addr, 1);
> >                  return ret;
> >          }
> > +       case TARGET_SO_BINDTODEVICE:
> > +       {
> > +               char *dev_ifname;
> > +
> > +               if (optlen > IFNAMSIZ)
> > +                       return -TARGET_EINVAL;
> 
> This needs braces for our coding style; you might like
> to run your patches through scripts/checkpatch.pl, which
> will warn about this kind of thing.

Ahh, I figured you had kernel style.

> 
> Also, the kernel implementation handles overlong
> lengths via
>      if (optlen > IFNAMSIZ - 1) {
>          optlen = IFNAMSIZ - 1;
>      }
> 
> which effectively truncates overlong strings rather
> than rejecting them. We should do the same (there will
> be complication required to ensure we pass the kernel
> a NUL-terminated string even if we don't get one from
> the guest; may be easiest to use a local array, given
> IFNAMSIZ is only 16).

hmm, should we not pass through as is to the kernel?
Since we don't copy anything we could just remove this
check and let the kernel decide policy?

> 
> > +
> > +               dev_ifname = lock_user(VERIFY_READ, optval_addr, 
optlen, 1);
> > +               ret = get_errno(setsockopt(sockfd, level, optname, 
dev_ifname, optlen));
> 
> This is passing the target optname through to the
> host kernel; you need to set 'optname = SO_BINDTODEVICE;'
> (look at the other cases in this switch).

Yes, I see. Thanks