| Message ID | 20201029133833.3450220-1-armbru@redhat.com |
|---|---|
| Headers | show |
| Series | sockets: Attempt to drain the abstract socket swamp | expand |
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 > > >
On 29/10/20 14:38, Markus Armbruster 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. > > 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*. Apart from the nits pointed out in patch 7 (commit message) and 8 (code), Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Thanks, and don't forget to fix the hole that your head has left in the wall. Paolo
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. Patch history: v1: https://lists.nongnu.org/archive/html/qemu-devel/2020-04/msg03799.html This version repurposes @path starting with '@' for abstract sockets, always tight. Only connect, no tests, no documentation. R-by Marc-André. 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.
Paolo Bonzini <pbonzini@redhat.com> writes: > On 29/10/20 14:38, Markus Armbruster 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. >> >> 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*. > > Apart from the nits pointed out in patch 7 (commit message) and 8 (code), > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > > Thanks, and don't forget to fix the hole that your head has left in the > wall. Thanks for the review, and thanks for cheering my up!
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
Daniel P. Berrangé <berrange@redhat.com> writes: > 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. Daniel, do you object, yes or no? I need to know to avoid wasting even more time. >> > 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. If this was patch review, I'd now ask the patch submitter to work it into the commit message. Thanks anyway :) [...]
On 02/11/20 09:44, Markus Armbruster wrote: >>> Reverting the feature requires rough consensus and a patch. >>> >>> I can provide a patch, but let's give everybody a chance to object >>> first. > Daniel, do you object, yes or no? I think we should keep the patch, especially since you have cleaned up everything already. The interaction with namespaces is interesting. Abstract sockets also do not have the usual issue with needing to unlink the socket before bind(), because they are cleaned up automatically when their last file descriptor is closed, including on SIGKILL. Paolo
On Mon, Nov 02, 2020 at 09:44:49AM +0100, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > 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. > > Daniel, do you object, yes or no? Yes, I object to removing the feature as it is clearly useful. Regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Mon, Nov 02, 2020 at 09:44:49AM +0100, Markus Armbruster wrote: >> Daniel P. Berrangé <berrange@redhat.com> writes: >> >> > 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: [...] >> >> >> 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. >> >> Daniel, do you object, yes or no? > > Yes, I object to removing the feature as it is clearly useful. Thanks. I sent v2 of my fixes. Can you take care of getting them merged, hopefully in time for 5.2?
On Mon, Nov 02, 2020 at 10:59:43AM +0100, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Mon, Nov 02, 2020 at 09:44:49AM +0100, Markus Armbruster wrote: > >> Daniel P. Berrangé <berrange@redhat.com> writes: > >> > >> > 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: > [...] > >> >> >> 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. > >> > >> Daniel, do you object, yes or no? > > > > Yes, I object to removing the feature as it is clearly useful. > > Thanks. I sent v2 of my fixes. Can you take care of getting them > merged, hopefully in time for 5.2? Sure, I'll aim to review & send PR today / tomorrow. Regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Mon, Nov 02, 2020 at 10:59:43AM +0100, Markus Armbruster wrote: >> Daniel P. Berrangé <berrange@redhat.com> writes: >> >> > On Mon, Nov 02, 2020 at 09:44:49AM +0100, Markus Armbruster wrote: >> >> Daniel P. Berrangé <berrange@redhat.com> writes: >> >> >> >> > 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: >> [...] >> >> >> >> 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. >> >> >> >> Daniel, do you object, yes or no? >> > >> > Yes, I object to removing the feature as it is clearly useful. >> >> Thanks. I sent v2 of my fixes. Can you take care of getting them >> merged, hopefully in time for 5.2? > > Sure, I'll aim to review & send PR today / tomorrow. Excellent. I'll be around to address whatever review comments you may have.
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. 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*. 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(-)