Re: [PATCH v7 05/14] qapi: net: add stream and dgram netdevs

[Markus Armbruster <armbru@redhat.com>]

Laurent Vivier <lvivier@redhat.com> writes:

> On 02/08/2022 10:37, Markus Armbruster wrote:
>> Laurent Vivier <lvivier@redhat.com> writes:
>> 
> ...
>>> diff --git a/qemu-options.hx b/qemu-options.hx
>>> index 79e00916a11f..170117e1adf0 100644
>>> --- a/qemu-options.hx
>>> +++ b/qemu-options.hx
>>> @@ -2726,6 +2726,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"
>
>  From v6:
>> This part needs to match NetdevStreamOptions above.
>> 
>> Missing here: the optional members of InetSocketAddress: numeric, to,
>> ipv4, ...  Do we care?
>
> At this patch level, no, because we decode them manually and not using 
> socket_connect()/socket_listen(). But the doc should be updated for PATCH 13/14 as I move 
> stream.c to QIO.
>
>> 
>> The next part needs to match NetdevDgramOptions above.
>
>
>>> +    "-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"
>
>  From v6:
>> I figure this covers table rows
>> 
>>    #    @remote         @local      |   okay?
>>    #    ----------------------------+--------
>>    #    multicast       absent      |   yes
>>    #    multicast       present     |   yes
>> 
>> for remote.type=inet and any local.type.
>> 
>> What about remote.type=fd?
>
> multicast is only supported with remote.type=inet, not fd or unix
>
> In net_dgram_init(), we initiate a multicast connection if remote.type is inet and address 
> type is multicast, otherwise it's an unicast connection.

Hmm.

With .type=inet, .host=... determines multicast.

With .type=fd, we *assume* unicast?  What if the underlying socket is
actually bound to a multicast address?

>>> +    "-netdev dgram,id=str,local.type=inet,local.host=addr,local.port=port[,remote.type=inet,remote.host=addr,remote.port=port]\n"
>
>  From v6:
>> I figure this covers table rows
>> 
>>     #    absent          present     |   yes
>>     #    not multicast   present     |   yes
>> 
>> for *.type=inet.
>
>
>>> +    "-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"
>
>  From v6:
>> I figure this covers table row
>> 
>>    #    absent          present     |   yes
>> 
>> for local.type=fd.
>> 
>> Together, they cover table row
>> 
>>     #    absent          present     |   yes
>> 
>> for any local.type.  Good.
>> 
>> Table row
>> 
>>     #    not multicast   present     |   yes
>> 
>> is only covered for *.type=inet.  Could either of the types be fd?
>
> In v7, I've update the table to include the case of fd:
>
>      =============  ========  =====
>      remote         local     okay?
>      =============  ========  =====
>      absent         absent    no
>      absent         not fd    no
>      absent         fd        yes
>      multicast      absent    yes
>      multicast      present   yes
>      not multicast  absent    no
>      not multicast  present   yes
>      =============  ========  =====
>
> For local, if it's not specified otherwise, fd is supported.
> Remote and local type must be the same (inet or unix), if local is fd, remote must not be 
> provided.

My brain is melting.