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. 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)


> 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>
>
> 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     |  22 +++++-
>  chardev/char.c            |   2 +
>  tests/test-util-sockets.c | 155 ++++++++++++++++++++------------------
>  util/qemu-sockets.c       |  54 ++++++++++---
>  5 files changed, 156 insertions(+), 91 deletions(-)
>
> --
> 2.26.2
>
>
>

-- 
Marc-André Lureau