Re: [Qemu-devel] [PATCH 1/3] Unified Datagram Socket Transport

[Jason Wang]

On 2017年07月19日 13:48, Anton Ivanov wrote:
> On 19/07/17 06:39, Jason Wang wrote:
> > On 2017年07月19日 01:08, address@hidden wrote:
> > > From: Anton Ivanov <address@hidden>
> > >
> > > 1. Creates a common backend for socket transports using
> > > recvmmsg().
> > > 2. Migrates L2TPv3 to the new backend
> >
> > It would be better if you could further split out 2 from this patch.
> >
> > > Signed-off-by: Anton Ivanov <address@hidden>
> > > ---
> > >   configure         |  10 +-
> > >   net/Makefile.objs |   2 +-
> > >   net/l2tpv3.c      | 531 
> > > +++++++++---------------------------------------------
> > >   net/net.c         |   4 +-
> > >   net/unified.c     | 406 +++++++++++++++++++++++++++++++++++++++++
> > >   net/unified.h     | 118 ++++++++++++
> > >   6 files changed, 613 insertions(+), 458 deletions(-)
> > >   create mode 100644 net/unified.c
> > >   create mode 100644 net/unified.h
> > >
> > > diff --git a/configure b/configure
> > > index a3f0522e8f..99a60b723c 100755
> > > --- a/configure
> > > +++ b/configure
> > > @@ -1862,7 +1862,7 @@ if ! compile_object -Werror ; then
> > >   fi
> > >     ##########################################
> > > -# L2TPV3 probe
> > > +# UNIFIED probe
> > >     cat > $TMPC <<EOF
> > >   #include <sys/socket.h>
> > > @@ -1870,9 +1870,9 @@ cat > $TMPC <<EOF
> > >   int main(void) { return sizeof(struct mmsghdr); }
> > >   EOF
> > >   if compile_prog "" "" ; then
> > > -  l2tpv3=yes
> > > +  unified=yes
> > >   else
> > > -  l2tpv3=no
> > > +  unified=no
> > >   fi
> > >     ##########################################
> > > @@ -5458,8 +5458,8 @@ fi
> > >   if test "$netmap" = "yes" ; then
> > >     echo "CONFIG_NETMAP=y" >> $config_host_mak
> > >   fi
> > > -if test "$l2tpv3" = "yes" ; then
> > > -  echo "CONFIG_L2TPV3=y" >> $config_host_mak
> > > +if test "$unified" = "yes" ; then
> > > +  echo "CONFIG_UNIFIED=y" >> $config_host_mak
> > >   fi
> >
> > Could we keep l2tpv3 option?
>
> The l2tpv3 test is actually a test for recvmmsg. If you can do one 
> recvmmsg transport you can do all of them.

Yes, but I wonder whether or not the check for recvmmsg is too simple. 
We probably want something like what AV_VSOCK did, test the support of 
each transport through socket().

> > >   if test "$cap_ng" = "yes" ; then
> > >     echo "CONFIG_LIBCAP=y" >> $config_host_mak
> > > diff --git a/net/Makefile.objs b/net/Makefile.objs
> > > index 67ba5e26fb..8026ad778a 100644
> > > --- a/net/Makefile.objs
> > > +++ b/net/Makefile.objs
> > > @@ -2,7 +2,7 @@ common-obj-y = net.o queue.o checksum.o util.o hub.o
> > >   common-obj-y += socket.o
> > >   common-obj-y += dump.o
> > >   common-obj-y += eth.o
> > > -common-obj-$(CONFIG_L2TPV3) += l2tpv3.o
> > > +common-obj-$(CONFIG_UNIFIED) += l2tpv3.o unified.o
> > >   common-obj-$(CONFIG_POSIX) += vhost-user.o
> > >   common-obj-$(CONFIG_SLIRP) += slirp.o
> > >   common-obj-$(CONFIG_VDE) += vde.o

[...]

> > > >   -    s = DO_UPCAST(NetL2TPV3State, nc, nc);
> > > > +    s->params = p;
> > > >   +    s->form_header = &l2tpv3_form_header;
> > > > +    s->verify_header = &l2tpv3_verify_header;
> > > >       s->queue_head = 0;
> > > >       s->queue_tail = 0;
> > > >       s->header_mismatch = false;
> > >
> > > Why not move all above into qemu_new_unified_net()?
>
> Only queue head/tail assignment can move.
>
> raw which uses same backend does not use header_mismatch. Form/verify 
> header are different for each sub-transport. F.e. for gre you need the 
> gre one, for raw you need the raw one, etc.

Right, I mean pass function pointer to qemu_new_unified_net().

> > > @@ -549,9 +188,9 @@ int net_init_l2tpv3(const Netdev *netdev,
> > >       l2tpv3 = &netdev->u.l2tpv3;
> > >         if (l2tpv3->has_ipv6 && l2tpv3->ipv6) {
> > > -        s->ipv6 = l2tpv3->ipv6;
> > > +        p->ipv6 = l2tpv3->ipv6;
> > >       } else {
> > > -        s->ipv6 = false;
> > > +        p->ipv6 = false;

[...]

> > >   diff --git a/net/unified.c b/net/unified.c
> >
> > Not a native speaker, but I think we need a better name here e.g udst 
> > which is short for Unified Datagram Socket Transport?
>
> I am not a native speaker either :)
>
> I am OK - let's call it udst as this is more descriptive and this 
> clearly delineates that you cannot
> migrate tcp/socket to it.

Ok.

> > >

[...]

> > > +
> > > +static ssize_t net_unified_receive_dgram_iov(NetClientState *nc,
> > > +                    const struct iovec *iov,
> > > +                    int iovcnt)
> > > +{
> > > +    NetUnifiedState *s = DO_UPCAST(NetUnifiedState, nc, nc);
> > > +
> > > +    struct msghdr message;
> > > +    int ret;
> > > +
> > > +    if (iovcnt > MAX_UNIFIED_IOVCNT - 1) {
> > > +        error_report(
> > > +            "iovec too long %d > %d, change unified.h",
> > > +            iovcnt, MAX_UNIFIED_IOVCNT
> > > +        );
> > > +        return -1;
> > > +    }
> > > +    if (s->offset > 0) {
> >
> > net_l2tpv3_receive_dgram_iov() does not have this check. I guess it 
> > s->offset=0 will be used by other transport. Maybe it's better to 
> > delay this change until is has a real user or add a comment here.
>
> The real user is in patch No 2. Raw.

Ok.

Thanks.