| Message ID | 20201102094422.173975-1-armbru@redhat.com |
|---|---|
| Headers | show |
| Series | sockets: Attempt to drain the abstract socket swamp | expand |
On 11/2/20 3:44 AM, Markus Armbruster wrote: > Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket > support" neglected to update socket_sockaddr_to_address_unix(). The > function returns a non-abstract socket address for abstract > sockets (wrong) with a null @path (also wrong; a non-optional QAPI str > member must never be null). > > The null @path is due to confused code going back all the way to > commit 17c55decec "sockets: add helpers for creating SocketAddress > from a socket". > > Add the required special case, and simplify the confused code. > > Fixes: 776b97d3605ed0fc94443048fdf988c7725e38a9 > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > util/qemu-sockets.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 11/2/20 10:44 AM, Markus Armbruster wrote: > Fixes: 4d3a329af59ef8acd076f99f05e82531d8129b34 > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > tests/test-util-sockets.c | 1 + > 1 file changed, 1 insertion(+) Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On 11/2/20 3:44 AM, Markus Armbruster wrote: > The abstract socket namespace is a non-portable Linux extension. An > attempt to use it elsewhere should fail with ENOENT (the abstract > address looks like a "" pathname, which does not resolve). We report > this failure like > > Failed to connect socket abc: No such file or directory > > Tolerable, although ENOTSUP would be better. > > However, 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. > > The above failure becomes > > Parameter 'backend.data.addr.data.abstract' is unexpected > > I consider this an improvement. > Commit message lacks mention of the fact that we are now explicitly not outputting 'strict' for non-abstract sockets (in fact, that change could be squashed in 9/11 if you wanted to do it there). But as this cleans up the code I mentioned in 9/11, I'll leave it up to Dan if the commit message needs a tweak; the end result is fine if we don't feel like a v3 spin just for moving hunks around. Reviewed-by: Eric Blake <eblake@redhat.com> > +++ b/chardev/char-socket.c > @@ -444,14 +444,20 @@ static char *qemu_chr_socket_address(SocketChardev *s, const char *prefix) > break; > case SOCKET_ADDRESS_TYPE_UNIX: > { > + const char *tight = "", *abstract = ""; > UnixSocketAddress *sa = &s->addr->u.q_unix; > > - return g_strdup_printf("%sunix:%s%s%s%s", prefix, > - s->addr->u.q_unix.path, > - sa->has_abstract && sa->abstract > - ? ",abstract" : "", > - sa->has_tight && sa->tight > - ? ",tight" : "", Unconditional output if tight is true (which is its stated default)... > +#ifdef CONFIG_LINUX > + if (sa->has_abstract && sa->abstract) { > + abstract = ",abstract"; > + if (sa->has_tight && sa->tight) { > + tight = ",tight"; > + } > + } > +#endif > + > + return g_strdup_printf("%sunix:%s%s%s%s", prefix, sa->path, > + abstract, tight, ...vs. the now-nicer conditional where tight is only present if abstract. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Eric Blake <eblake@redhat.com> writes: > On 11/2/20 3:44 AM, Markus Armbruster wrote: >> The abstract socket namespace is a non-portable Linux extension. An >> attempt to use it elsewhere should fail with ENOENT (the abstract >> address looks like a "" pathname, which does not resolve). We report >> this failure like >> >> Failed to connect socket abc: No such file or directory >> >> Tolerable, although ENOTSUP would be better. >> >> However, 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. >> >> The above failure becomes >> >> Parameter 'backend.data.addr.data.abstract' is unexpected >> >> I consider this an improvement. >> > > Commit message lacks mention of the fact that we are now explicitly not > outputting 'strict' for non-abstract sockets (in fact, that change could I trust you mean 'tight'. > be squashed in 9/11 if you wanted to do it there). Less churn. I'll do it if I need to respin. > But as this cleans > up the code I mentioned in 9/11, I'll leave it up to Dan if the commit > message needs a tweak; the end result is fine if we don't feel like a v3 > spin just for moving hunks around. Neglecting to mention the change in the commit message isn't *too* bad, because the change "only" corrects something new in this series, which makes a future question "why did this go away?" relatively unlikely. That said, I'm happy to respin if you think it's worthwhile. Just ask. > Reviewed-by: Eric Blake <eblake@redhat.com> > >> +++ b/chardev/char-socket.c >> @@ -444,14 +444,20 @@ static char *qemu_chr_socket_address(SocketChardev *s, const char *prefix) >> break; >> case SOCKET_ADDRESS_TYPE_UNIX: >> { >> + const char *tight = "", *abstract = ""; >> UnixSocketAddress *sa = &s->addr->u.q_unix; >> >> - return g_strdup_printf("%sunix:%s%s%s%s", prefix, >> - s->addr->u.q_unix.path, >> - sa->has_abstract && sa->abstract >> - ? ",abstract" : "", >> - sa->has_tight && sa->tight >> - ? ",tight" : "", > > Unconditional output if tight is true (which is its stated default)... > >> +#ifdef CONFIG_LINUX >> + if (sa->has_abstract && sa->abstract) { >> + abstract = ",abstract"; >> + if (sa->has_tight && sa->tight) { >> + tight = ",tight"; >> + } >> + } >> +#endif >> + >> + return g_strdup_printf("%sunix:%s%s%s%s", prefix, sa->path, >> + abstract, tight, > > ...vs. the now-nicer conditional where tight is only present if abstract.
In my opinion, the Linux-specific abstract UNIX domain socket feature introduced in 5.1 should have been rejected. The feature is niche, the interface clumsy, the implementation buggy and incomplete, and the test coverage insufficient. Review fail. Fixing the parts we can still fix now is regrettably expensive. If I had the power to decide, I'd unceremoniously revert the feature, compatibility to 5.1 be damned. But I don't, so here we go. I'm not sure this set of fixes is complete. However, I already spent too much time on this, so out it goes. Lightly tested. For additional information, see Subject: Our abstract UNIX domain socket support is a mess Date: Wed, 28 Oct 2020 13:41:06 +0100 Message-ID: <87o8kmwmjh.fsf@dusky.pond.sub.org> v2: * PATCH 03: Have unix_server_thread_func(), unix_client_thread_func() store their parameter in a variable in the hope of communicating its type more clearly [Daniel]. No lasting effect, as the former goes away in PATCH 5, and the latter is rewritten in PATCH 6. * PATCH 05: Fix commit message typo [Eric] * PATCH 07: Improve commit message [Paolo] * PATCH 08: Fix computation of @tight [Paolo, Eric], and copying of @sun_path [Eric] * PATCH 11: Tweak QAPI schema [Eric], less ugly ifdeffery [Eric], fix logic error in use of new saddr_is_tight(). Markus Armbruster (11): test-util-sockets: Plug file descriptor leak test-util-sockets: Correct to set has_abstract, has_tight test-util-sockets: Clean up SocketAddress construction test-util-sockets: Factor out test_socket_unix_abstract_one() test-util-sockets: Synchronize properly, don't sleep(1) test-util-sockets: Test the complete abstract socket matrix sockets: Fix default of UnixSocketAddress member @tight sockets: Fix socket_sockaddr_to_address_unix() for abstract sockets char-socket: Fix qemu_chr_socket_address() for abstract sockets sockets: Bypass "replace empty @path" for abstract unix sockets sockets: Make abstract UnixSocketAddress depend on CONFIG_LINUX qapi/sockets.json | 14 ++-- chardev/char-socket.c | 24 +++++- chardev/char.c | 2 + tests/test-util-sockets.c | 155 ++++++++++++++++++++------------------ util/qemu-sockets.c | 54 ++++++++++--- 5 files changed, 157 insertions(+), 92 deletions(-)