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

[Jason Wang <jasowang@redhat.com>]

On 2017年07月19日 13:48, Anton Ivanov wrote:
>
>
> On 19/07/17 06:39, Jason Wang wrote:
>>
>>
>> On 2017年07月19日 01:08, anton.ivanov@cambridgegreys.com wrote:
>>> From: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>>>
>>> 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 <anton.ivanov@cambridgegreys.com>
>>> ---
>>>   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.