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

[Laurent Vivier <lvivier@redhat.com>]

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

> 
>> +#
>> +# 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")


>>   #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