Re: [RFC PATCH 0/6] qapi: net: add unix socket type support to netdev backend

[Laurent Vivier <lvivier@redhat.com>]

On 10/05/2022 10:26, Daniel P. Berrangé wrote:
> On Mon, May 09, 2022 at 07:36:12PM +0200, Laurent Vivier wrote:
>> "-netdev socket" only supports inet sockets.
>>
>> It's not a complex task to add support for unix sockets, but
>> the socket netdev parameters are not defined to manage well unix
>> socket parameters.
>>
>> As discussed in:
>>
>>    "socket.c added support for unix domain socket datagram transport"
>>    https://lore.kernel.org/qemu-devel/1C0E1BC5-904F-46B0-8044-68E43E67BE60@gmail.com/
>>
>> This series adds support of unix socket type using SocketAddress QAPI structure.
>>
>> A new netdev backend "socket-ng" is added, that is barely a copy of "socket"
>> backend but it uses the SocketAddress QAPI to provide socket parameters.
>> And then it also implement unix sockets (TCP and UDP).
> 
> So pulling in the QAPI from patch 2
> 
>     { 'enum': 'NetdevSocketNGMode',
>       'data':  [ 'dgram', 'server', 'client' ] }
> 
>     { 'struct': 'NetdevSocketNGOptions',
>       'data': {
>         'mode':    'NetdevSocketNGMode',
>         '*addr':   'SocketAddress',
>         '*remote': 'SocketAddress',
>         '*local':  'SocketAddress' } }
> 
>> Some examples of CLI syntax:
>>
>>    for TCP:
>>
>>    -netdev socket-ng,id=socket0,mode=server,addr.type=inet,addr.host=localhost,addr.port=1234
>>    -netdev socket-ng,id=socket0,mode=client,addr.type=inet,addr.host=localhost,addr.port=1234
>>
>>    -netdev socket-ng,id=socket0,mode=dgram,\
>>            local.type=inet,local.host=localhost,local.port=1234,\
>>            remote.type=inet,remote.host=localhost,remote.port=1235
>>
>>    for UNIX:
>>
>>    -netdev socket-ng,id=socket0,mode=server,addr.type=unix,addr.path=/tmp/qemu0
>>    -netdev socket-ng,id=socket0,mode=client,addr.type=unix,addr.path=/tmp/qemu0
>>
>>    -netdev socket-ng,id=socket0,mode=dgram,\
>>            local.type=unix,local.path=/tmp/qemu0,\
>>            remote.type=unix,remote.path=/tmp/qemu1
>>
>>    for FD:
>>
>>    -netdev socket-ng,id=socket0,mode=server,addr.type=fd,addr.str=4
>>    -netdev socket-ng,id=socket0,mode=client,addr.type=fd,addr.str=5
>>
>>    -netdev socket-ng,id=socket0,mode=dgram,local.type=fd,addr.str=4
> 
>                                                            ^^^ local.str=4
> 
> I notice that in all these examples, mode=client/server always use
> the 'addr' field, and mode=dgram always uses the 'local'/'remote'
> fields. IOW, there is almost no commonality between the dgram scenario
> and the stream scenario, which feels sub-optimal.
> 
> Two alternatives come to mind
> 
>   - mode=client could use 'remote' and mode=server could use 'local',
>     removing the 'addr' field entirely
> 
>   - Have completely separate backends, ie '-netdev stream' for
>     client/server TCP/UNIX sockets, and '-netdev dgram' for UDP
>     sockets, removing 'mode' field.
> 
> I'd have a slight preference for the second, since I'm not thrilled
> by the 'socket-ng' name :-)

It seems reasonable, I'm going to update my series in this way:

    { 'struct': 'NetdevStreamOptions',
      'data': {
        'addr':   'SocketAddress' } }

    { 'struct': 'NetdevDgramOptions',
      'data': {
       '*local':   'SocketAddress',
       '*remote':   'SocketAddress' } }

Both parameters are optional because:
multicast needs 'remote' for the multicast address and optionally a local for source 
address or fd.
unicast needs both, except for fd where local provides fd and remote is needed only if 
local is not an fd.

    { 'union': 'Netdev',
      'base': { 'id': 'str', 'type': 'NetClientDriver' },
      'discriminator': 'type',
      'data': {
      ...
      'stream': 'NetdevStreamOptions',
      'dgram': 'NetdevDgramOptions' } }

Thanks,
Laurent