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.
The point is we have four possible combinations
NON-ABSTRACT + FULL SIZE
NON-ABSTRACT + MINIMAL SIZE (default)
ABSTRACT + FULL SIZE
ABSTRACT + MINIMAL SIZE (default)
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.
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
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' } }
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
>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
>
> Signed-off-by: xiaoqiang zhao <zxq_yx_...@163.com>
> ---
> util/qemu-sockets.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index bcc06d0e01..7ba9c497ab 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -939,6 +939,7 @@ static int unix_connect_saddr(UnixSocketAddress *saddr,
> Error **errp)
> struct sockaddr_un un;
> int sock, rc;
> size_t pathlen;
> + socklen_t serverlen;
>
> if (saddr->path == NULL) {
> error_setg(errp, "unix connect: no path specified");
> @@ -963,10 +964,17 @@ static int unix_connect_saddr(UnixSocketAddress *saddr,
> Error **errp)
> un.sun_family = AF_UNIX;
> memcpy(un.sun_path, saddr->path, pathlen);
>
> + if (saddr->path[0] == '@') {
> + un.sun_path[0] = '\0';
> + serverlen = pathlen + offsetof(struct sockaddr_un, sun_path);
> + } else {
> + serverlen = sizeof(un);
> + }
> +
> /* connect to peer */
> do {
> rc = 0;
> - if (connect(sock, (struct sockaddr *) &un, sizeof(un)) < 0) {
> + if (connect(sock, (struct sockaddr *) &un, serverlen) < 0) {
> rc = -errno;
> }
> } while (rc == -EINTR);
> --
> 2.17.1
>
>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
