Re: [PATCH 00/11] sockets: Attempt to drain the abstract socket swamp

[Daniel P . Berrangé]

On Fri, Oct 30, 2020 at 11:11:19AM +0100, Markus Armbruster wrote:
> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> 
> > Hi Markus,
> >
> > On Thu, Oct 29, 2020 at 5:43 PM Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> 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.
> >>
> >
> > I also failed (as chardev maintainer..) to not only review but weigh in and
> > discuss the merits or motivations behind it.
> >
> > I agree with you. Also the commit lacks motivation behind this "feature".
> >
> >
> >> 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.
> >>
> >> Regardless, I *will* make time for ripping the feature out if we
> >> decide to do that.  Quick & easy way to avoid reviewing this series
> >> *hint* *hint*.
> >>
> >
> > well, fwiw, I would also take that approach too, to the risk of upsetting
> > the users.
> 
> Reverting the feature requires rough consensus and a patch.
> 
> I can provide a patch, but let's give everybody a chance to object
> first.
> 
> >            But maybe we can get a clear reason behind it before that
> > happens. (sorry, I didn't dig the ML archive is such evidence is there, it
> > should have been in the commit message too)
> 
> I just did, and found next to nothing.
> 
> This is the final cover letter:
> 
>     qemu-sockets: add abstract UNIX domain socket support
> 
>     qemu does not support abstract UNIX domain
>     socket address. Add this ability to make qemu handy
>     when abstract address is needed.
> 
> Boils down to "$feature is needed because it's handy when it's needed",
> which is less than helpful.

Well if you have an existing application that uses abstract sockets,
and you want to connect QEMU to it, then QEMU needs to support it,
or you are forced to interject a proxy between your app and QEMU
which is an extra point of failure and lantency. I was interested
in whether there was a specific application they were using, but
that is largely just from a curiosity POV. There's enough usage of
abstract sockets in apps in general that is it clearly a desirable
feature to enable.

Even if no external app is involved and you're just connecting
together 2 QEMU processes' chardevs, abstract sockets are interesting
because of their differing behaviour to non-abstract sockets.

Most notably non-abstract sockets are tied to the filesystem mount
namespace in Linux, where as abstract sockets are tied to the network
namespace. This is a useful distinction in the container world. Ordinarily
you can't connect QEMUs in 2 separate containers together, because they
always have distinct mount namespaces. There is often the ability to
share network namespaces between containers though, and thus unlock
use of abstract sockets for communications. 

> v2: https://lists.nongnu.org/archive/html/qemu-devel/2020-04/msg03803.html
> 
>     Minor cleanup.
> 
>     Daniel asks why it's needed, points out listen is missing, and
>     suggests the two boolean flags abstract, tight.
> 
> v3: https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg02291.html
> 
>     Implement interface proposed by Daniel, default of @tight broken,
>     tests (which don't catch the broken default), documentation.
> 
>     Eric suggests QAPI schema doc improvements (but doesn't challenge
>     the interface).
> 
>     R-by Daniel for the code.  He asks for randomized @path in tests.
> 
> v4: https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg04036.html
> 
>     Daniel points out style nits in tests.
> 
>     Eric suggests a few more QAPI schema doc improvements.
> 
> v5: https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg04144.html
> 
>     R-by Daniel for the tests.
> 
> v6: https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg04508.html
> 
>     No further review comments.
> 
> PR: https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg05747.html
> 
>     Pull request catches my eye.  The interface looks odd, and I
>     challenge @tight.  I silently accept Daniel's defense of it without
>     digging deeper.
> 
> This is a review failure.  I do not blame the patch submitter.

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 :|