Re: [PATCH v2] qemu-sockets: add abstract UNIX domain socket support

[Eric Blake <eblake@redhat.com>]

On 4/23/20 4:00 AM, Daniel P. Berrangé wrote:
> Adding Eric & Markus for QAPI modelling questions
> 
> On Thu, Apr 23, 2020 at 11:56:40AM +0800, xiaoqiang zhao wrote:
>> unix_connect_saddr now support abstract address type
>>
>> By default qemu does not support abstract UNIX domain
>> socket address. Add this ability to make qemu handy
>> when abstract address is needed.
> 
> Was that a specific app you're using with QEMU that needs this ?
> 
>> Abstract address is marked by prefixing the address name with a '@'.
> 
> For full support of the abstract namespace we would ned to allow
> the "sun_path" to contain an arbitrary mix of NULs and non-NULs
> characters, and allow connect() @addrlen to be an arbitrary size.
> 
> This patch only allows a single initial NUL, and reqiures @addrlen
> to be the full size of sun_path, padding with trailing NULs. This
> limitation is impossible to lift with QEMU's current approach to
> UNIX sockets, as it relies on passing around a NULL terminated
> string, so there's no way to have embedded NULs. Since there's
> no explicit length, we have to chooose between forcing the full
> sun_path size as @addrlen, or forcing the string length as the
> @addrlen value.
> 
> IIUC, socat makes the latter decision by default, but has a
> flag to switch to the former.
> 
>    [man socat]
>    unix-tightsocklen=[0|1]
>    On  socket  operations,  pass  a  socket  address  length that does not
>    include the whole struct sockaddr_un record but (besides  other  compo‐
>    nents)  only  the  relevant  part  of  the filename or abstract string.
>    Default is 1.
>    [/man]
> 
> This actually is supported for both abstract and non-abstract
> sockets, though IIUC this doesn't make a semantic difference
> for non-abstract sockets.

Yes, that matches my experience as well.  '^@a' length 2 is a different 
socket than '^@a^@' length three, but 'a' length 1 and 'a^@' length 2 
are the same socket because only non-abstract sockets end at the earlier 
of the first NUL or the length.

> 
> The point is we have four possible combinations
> 
>   NON-ABSTRACT + FULL SIZE
>   NON-ABSTRACT + MINIMAL SIZE  (default)

Two combinations, but only one behavior.

>   ABSTRACT + FULL SIZE
>   ABSTRACT + MINIMAL SIZE  (default)

And two more behaviors.

> 
> With your patch doing the latter, it means QEMU supports
> only two combinations
> 
>    NON+ABSTRACT + FULL SIZE
>    ABSTRACT + MINIMAL SIZE
> 
> and also can't use "@somerealpath" for a non-abstract
> socket, though admittedly this is unlikely.
> 
> Socat uses a special option to request use of abstract
> sockets. eg ABSTRACT:somepath, and automatically adds
> the leading NUL, so there's no need for a special "@"
> character. This means that UNIX:@somepath still resolves
> to a filesystem path and not a abstract socket path.

Yes, having an additional knob to express abstract sockets seems better 
than overloading a specific character (although @sockname is a relative 
name, and relative socket names already come with their own set of 
problems).

> 
> Finally, the patch as only added support for connect()
> not listen(). I think if QEMU wants to support abstract
> sockets we must do both, and also have unit tests added
> to tests/test-util-sockets.c

Indeed.

> 
> 
> The question is whether we're ok with this simple
> approach in QEMU, or should do a full approach with
> more explicit modelling.
> 
> ie should we change QAPI thus:
> 
> { 'struct': 'UnixSocketAddress',
>    'data': {
>      'path': 'str',
>      'tight': 'bool',
>      'abstract': 'bool' } }

You'd want to make 'tight' and 'abstract' optional, with defaults to the 
existing practice, but otherwise this looks sane to me.


> 
> where 'tight' is a flag indicating whether to set @addrlen
> to the minimal string length, or the maximum sun_path length.
> And 'abstract' indicates that we automagically add a leading
> NUL.
> 
> This would *not* allow for NULs in the middle of path,
> but I'm not so bothered about that, since I can't see that
> being widely used. If we really did need that it could be
> added via a 'base64': 'bool' flag, to indicate that @path
> is base64 encoded and thus may contain NULs

Agreed that this is less likely to be needed.

> 
>>From a CLI POV, this could be mapped to QAPI thus
> 
>   *  -chardev unix:somepath
> 
>        @path==somepath
>        @tight==false
>        @abstract==false
> 
>   *  -chardev unix:somepath,tight
> 
>        @path==somepath
>        @tight==true
>        @abstract==false
> 
>   *  -chardev unix-abstract:somepath
> 
>        @path==somepath
>        @tight==false
>        @abstract==true
> 
>   *  -chardev unix-abstract:somepath,tight
> 
>        @path==somepath
>        @tight==true
>        @abstract==true
> 

Also sounds reasonable to me.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org