Re: [RFC PATCH v4 04/11] qapi: net: add stream and dgram netdevs

[Markus Armbruster]

Laurent Vivier <lvivier@redhat.com> writes:

> On 24/06/2022 11:41, Markus Armbruster wrote:
>> Laurent Vivier <lvivier@redhat.com> writes:
>> 
>>> Copied from socket netdev file and modified to use SocketAddress
>>> to be able to introduce new features like unix socket.
>>>
>>> "udp" and "mcast" are squashed into dgram netdev, multicast is detected
>>> according to the IP address type.
>>> "listen" and "connect" modes are managed by stream netdev. An optional
>>> parameter "server" defines the mode (server by default)
>>>
>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>> Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
>>> ---
>> 
>> [...]
>> 
>>> diff --git a/qapi/net.json b/qapi/net.json
>>> index d6f7cfd4d656..32a9b1a5ac6c 100644
>>> --- a/qapi/net.json
>>> +++ b/qapi/net.json
>>> @@ -7,6 +7,7 @@
>>>   ##
>>>   
>>>   { 'include': 'common.json' }
>>> +{ 'include': 'sockets.json' }
>>>   
>>>   ##
>>>   # @set_link:
>>> @@ -566,6 +567,42 @@
>>>       '*isolated':  'bool' },
>>>     'if': 'CONFIG_VMNET' }
>>>   
>>> +##
>>> +# @NetdevStreamOptions:
>>> +#
>>> +# Configuration info for stream socket netdev
>>> +#
>>> +# @addr: socket address to listen on (server=true)
>>> +#        or connect to (server=false)
>>> +# @server: create server socket (default: true)
>>> +#
>>> +# Since: 7.1
>>> +##
>>> +{ 'struct': 'NetdevStreamOptions',
>>> +  'data': {
>>> +    'addr':   'SocketAddress',
>>> +    '*server': 'bool' } }
>>> +
>>> +##
>>> +# @NetdevDgramOptions:
>>> +#
>>> +# Configuration info for datagram socket netdev.
>>> +#
>>> +# @remote: remote address
>>> +# @local: local address
>>> +#
>>> +# The code checks there is at least one of these options and reports an 
>>> error
>>> +# if not. If remote address is present and it's a multicast address, local
>>> +# address is optional. Otherwise local address is required and remote 
>>> address
>>> +# is optional.
>> 
>> I need to make a table to understand this.
>> 
>> 
>>      @remote         @local      |   okay?
>>      ----------------------------+--------
>>      absent          present     |   yes
>>      multicast       absent      |   yes
>>      multicast       present     |   yes
>>      not multicast   absent      |   no
>>      not multicast   present     |   yes
>> 
>> Correct?
>
> yes

Forgot the row

        absent          absent      |   no

Fortunately, the comment is perfectly clear there.

>>> +#
>>> +# Since: 7.1
>>> +##
>>> +{ 'struct': 'NetdevDgramOptions',
>>> +  'data': {
>>> +    '*local':  'SocketAddress',
>>> +    '*remote': 'SocketAddress' } }
>>> +
>>>   ##
>>>   # @NetClientDriver:
>>>   #
>>> @@ -579,8 +616,9 @@
>>>   #        @vmnet-bridged since 7.1
>>>   ##
>>>   { 'enum': 'NetClientDriver',
>>> -  'data': [ 'none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'vde',
>>> -            'bridge', 'hubport', 'netmap', 'vhost-user', 'vhost-vdpa',
>>> +  'data': [ 'none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'stream',
>>> +            'dgram', 'vde', 'bridge', 'hubport', 'netmap', 'vhost-user',
>>> +            'vhost-vdpa',
>>>               { 'name': 'vmnet-host', 'if': 'CONFIG_VMNET' },
>>>               { 'name': 'vmnet-shared', 'if': 'CONFIG_VMNET' },
>>>               { 'name': 'vmnet-bridged', 'if': 'CONFIG_VMNET' }] }
>>> @@ -610,6 +648,8 @@
>>>       'tap':      'NetdevTapOptions',
>>>       'l2tpv3':   'NetdevL2TPv3Options',
>>>       'socket':   'NetdevSocketOptions',
>>> +    'stream':   'NetdevStreamOptions',
>>> +    'dgram':    'NetdevDgramOptions',
>>>       'vde':      'NetdevVdeOptions',
>>>       'bridge':   'NetdevBridgeOptions',
>>>       'hubport':  'NetdevHubPortOptions',
>>> diff --git a/qemu-options.hx b/qemu-options.hx
>>> index 377d22fbd82f..03d58da6f8ed 100644
>>> --- a/qemu-options.hx
>>> +++ b/qemu-options.hx
>>> @@ -2722,6 +2722,18 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
>>>       "-netdev socket,id=str[,fd=h][,udp=host:port][,localaddr=host:port]\n"
>>>       "                configure a network backend to connect to another 
>>> network\n"
>>>       "                using an UDP tunnel\n"
>>> +    "-netdev 
>>> stream,id=str[,server=on|off],addr.type=inet,addr.host=host,addr.port=port\n"
>>> +    "-netdev stream,id=str[,server=on|off],addr.type=fd,addr.str=h\n"
>>> +    "                configure a network backend to connect to another 
>>> network\n"
>>> +    "                using a socket connection in stream mode.\n"
>> 
>> This shows -netdev stream with address types 'inet' and 'fd' only.  Are
>> address types 'unix' and and 'vsock' rejected?
>
> Yes, in net_stream_client_init() we have a switch that manages only inet and 
> fd (I'm going 
> to update the patch because server and client doesn't manage the error in the 
> same way).
> This is updated in patch "net: stream: add unix socket".
>
>> 
>>> +    "-netdev 
>>> dgram,id=str,remote.type=inet,remote.host=maddr,remote.port=port[,local.type=inet,local.host=addr]\n"
>>> +    "-netdev 
>>> dgram,id=str,remote.type=inet,remote.host=maddr,remote.port=port[,local.type=fd,local.str=h]\n"
>>> +    "                configure a network backend to connect to a multicast 
>>> maddr and port\n"
>>> +    "                use 'local.host=addr' to specify the host address to 
>>> send packets from\n"
>> 
>> I think we use ``local.host=addr`` markup.
>
> ok
>
>> 
>> Since this part is about multicast, only remote.type=inet makes sense
>> (other types can't be multicast).
>
> right
>
>> Are local address types 'unix' and 'vsock' rejected?
>
> if the type is not inet, we go into the not multicast code, and it accepts 
> only inet and fd.
>
>> 
>>> +    "-netdev 
>>> dgram,id=str,local.type=inet,local.host=host,local.port=port[,remote.type=inet,remote.host=host,remote.port=port]\n"
>>> +    "-netdev dgram,id=str,local.type=fd,local.str=h\n"
>>> +    "                configure a network backend to connect to another 
>>> network\n"
>>> +    "                using an UDP tunnel\n"
>> 
>> Is this unicast only?
>
> yes
>
>> Are other combinations of local.type and remote.type rejected?
>
> If we have remote, it must not be fd type and remote type must be equal to 
> local type.
> local type can only be inet or fd (updated with unix in patch "net: dgram: 
> add unix socket")

So, only some combinations of socket address types are valid.  Shouldn't
this be documented in net.json?

>>>   #ifdef CONFIG_VDE
>>>       "-netdev 
>>> vde,id=str[,sock=socketpath][,port=n][,group=groupname][,mode=octalmode]\n"
>>>       "                configure a network backend to connect to port 'n' 
>>> of a vde switch\n"
>> 
>
> Thanks,
> Laurent