qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Eric Blake
Subject: Re: [PATCH v2] qemu-sockets: add abstract UNIX domain socket support
Date: Mon, 27 Apr 2020 08:47:29 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]