qemu-devel
[Top][All Lists]
Advanced

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

Our abstract UNIX domain socket support is a mess


From: Markus Armbruster
Subject: Our abstract UNIX domain socket support is a mess
Date: Wed, 28 Oct 2020 13:41:06 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)

Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket
support" (v5.1) has made a mess.

First, what are abstract UNIX domain sockets?  They use a separate
"abstract" namespace which is independent of the filesystem.  This is a
non-portable Linux extension.  See unix(7).

Traditional pathname sockets have a zero-terminated string in
sockaddr_un member sun_path.  Abstract sockets have a binary blob
starting with a 0 byte.  Works, because the empty string is not a valid
pathname.

This interface is a hack.  The leading 0 byte is ugly.  The size of the
blob is given by the socklen_t argument passed with the struct
sockaddr_un.  People use strings, then forget to either cut the length
to the size of the string, or pad to the blob to sizeof(sockaddr_un),
and do it consistently.

QEMU's interface is differently messy.

Our equivalent to struct sockaddr_un is QAPI type UnixSocketAddress:

    { 'struct': 'UnixSocketAddress',
      'data': {
        'path': 'str' }

@path corresponds to sockaddr_un member sun_path.  sun_family = AF_UNIX
and socklen_t sizeof(sockaddr_un) are implied.

We didn't repurpose @path for abstract sockets like the Linux kernel did
with sun_path.  Instead, we added a flag @abstract (default false).
When it's on, we make a binary blob by prefixing @path with a 0 byte,
and pad it with more 0 bytes.

We added a second flag @tight (default true) to optionally cut the
socklen_t to the end of the string (the terminating 0 byte is not
included).

Six revisions were posted, and the QAPI maintainers were cc'ed on none
of them.  I spotted @tight in the pull request by chance, and found it
odd.  I inquired, and Daniel explained:

    In the abstract namespace the length of the socket is critical
    information. ie

       "\0foo" (length == 4,  tight=true)

    is a completely different socket from

       "\0foo\0..repeated...\0" (length == sizeof(sun_path), tight=false)

    In theory you can have any length in between those extremes,
    each being a different socket, but in practice no one is that
    insane. Apps either use the full length, or the minimal
    length. The "tight" terminology is copied from "socat" which
    uses this same option name

    Message-ID: <87imgqr8g9.fsf@dusky.pond.sub.org>
    https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg05812.html

I did not press further, or dig deeper.  I should have.  I did only when
Kevin reported @tight's default is backward in QMP.  I'm not liking what
I found.


Issue#1: our interface is differently ugly, for no good reason

Like the Linux kernel, we also appropriate existing @path for abstract
sockets.  With less excuse, though; we could have created a neater
interface, easily.

Unlike the Linux kernel, we don't do blobs.  In other words, our variant
of the hack is not general.

Elsewhere in QMP, we use base64 for blobs.

Aside: QMP can do embedded 0 bytes.  It represents them as overlong
UTF-8 “\xC0\x80", though.

Not sure the interface is worth fixing now.  Abstract sockets are niche.
In my opinion, we should've said no.


Issue#2: we offer abstract sockets even on non-Linux hosts

I expect this to fail with ENOENT elsehwere.  I tested OpenBSD, and
fails that way.  We report this failure like

    Failed to connect socket abc: No such file or directory

Tolerable, although ENOTSUP would be better.

Introspection lies: it has @abstract regardless of host support.  Easy
enough to fix: since Linux provides them since 2.2, 'if':
'defined(CONFIG_LINUX)' should do.

This should also change the error report to

    Parameter 'backend.data.addr.data.abstract' is unexpected

Improvement, I think.


Issue#3: @tight's default is backward in QMP

Incorrect use of the QAPI type in C, plus insufficient test coverage.

Fixable.  Treat 5.1's default as bug.


Final plea: please copy the QAPI schema maintainers on all
schema-changing patches!  We happen to have some interface design
expertise.  Make use of it!  Also, please give us a chance to correct
mistakes while corrections are easy and cheap.




reply via email to

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