1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] remote: fix systemd IP socket activation with virtproxyd

Daniel P. Berrangé posted 1 patch 4 years, 6 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190924150717.14790-1-berrange@redhat.com
src/remote/remote_daemon.c | 9 +++++++++
1 file changed, 9 insertions(+)
[libvirt] [PATCH] remote: fix systemd IP socket activation with virtproxyd
Posted by Daniel P. Berrangé 4 years, 6 months ago 
We recently forbid the use of --listen with socket activation:

  commit 3a6a725b8f575890ee6c151ad1f46ea0ceea1f3b
  Author: Daniel P. Berrangé <berrange@redhat.com>
  Date:   Thu Aug 22 14:52:16 2019 +0100

      remote: forbid the --listen arg when systemd socket activation

In this change we forgot that virtproxyd doesn't have a --listen
parameter, and instead behaves as if it was always present. Thus
when systemd socket activation is present, we must disable this
built-in default

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/remote/remote_daemon.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index 7195ac9218..43409edd24 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -423,11 +423,20 @@ daemonSetupNetworking(virNetServerPtr srv,
         return -1;
 
 #ifdef WITH_IP
+# ifdef (LIBVIRTD
     if (act && ipsock) {
         VIR_ERROR(_("--listen parameter not permitted with systemd activation "
                     "sockets, see 'man libvirtd' for further guidance"));
         return -1;
     }
+# else /* ! LIBVIRTD */
+    /* We don't have a --listen arg with virtproxyd, we're just
+     * hardcoded to assume --listen. Thus with systemd we must
+     * change that default
+     */
+    if (act)
+        ipsock = 0;
+# endif /* ! LIBVIRTD */
 #endif /* ! WITH_IP */
 
     if (config->unix_sock_group) {
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] remote: fix systemd IP socket activation with virtproxyd
Posted by Erik Skultety 4 years, 6 months ago 
On Tue, Sep 24, 2019 at 04:07:17PM +0100, Daniel P. Berrangé wrote:
> We recently forbid the use of --listen with socket activation:
>
>   commit 3a6a725b8f575890ee6c151ad1f46ea0ceea1f3b
>   Author: Daniel P. Berrangé <berrange@redhat.com>
>   Date:   Thu Aug 22 14:52:16 2019 +0100
>
>       remote: forbid the --listen arg when systemd socket activation
>
> In this change we forgot that virtproxyd doesn't have a --listen
> parameter, and instead behaves as if it was always present. Thus
> when systemd socket activation is present, we must disable this
> built-in default
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/remote/remote_daemon.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
> index 7195ac9218..43409edd24 100644
> --- a/src/remote/remote_daemon.c
> +++ b/src/remote/remote_daemon.c
> @@ -423,11 +423,20 @@ daemonSetupNetworking(virNetServerPtr srv,
>          return -1;
>
>  #ifdef WITH_IP
> +# ifdef (LIBVIRTD

^fails to compile:
s/(//

>      if (act && ipsock) {
>          VIR_ERROR(_("--listen parameter not permitted with systemd activation "
>                      "sockets, see 'man libvirtd' for further guidance"));
>          return -1;
>      }
> +# else /* ! LIBVIRTD */
> +    /* We don't have a --listen arg with virtproxyd, we're just
> +     * hardcoded to assume --listen. Thus with systemd we must
> +     * change that default
> +     */
> +    if (act)
> +        ipsock = 0;

I'm a bit confused with this bit wrt to what actually happens later in the
code. Basically this @ipsock is only relevant up until the point where we start
registering services listening for traffic e.g.virNetServerAddServiceTCP (this
is one is easier as an example). If I look at the condition:

if (((ipsock && config->listen_tcp) || act) ...

why does it even matter that we clear ipsock when socket activation is enabled?
The condition is true regardless of @ipsock and it's also not populated further
into the function being called unlike @act, so this bit is making me confused,
so what exactly is happening if we don't clear @ipsock with virtproxyd?

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] remote: fix systemd IP socket activation with virtproxyd
Posted by Daniel P. Berrangé 4 years, 6 months ago 
On Wed, Sep 25, 2019 at 09:42:57AM +0200, Erik Skultety wrote:
> On Tue, Sep 24, 2019 at 04:07:17PM +0100, Daniel P. Berrangé wrote:
> > We recently forbid the use of --listen with socket activation:
> >
> >   commit 3a6a725b8f575890ee6c151ad1f46ea0ceea1f3b
> >   Author: Daniel P. Berrangé <berrange@redhat.com>
> >   Date:   Thu Aug 22 14:52:16 2019 +0100
> >
> >       remote: forbid the --listen arg when systemd socket activation
> >
> > In this change we forgot that virtproxyd doesn't have a --listen
> > parameter, and instead behaves as if it was always present. Thus
> > when systemd socket activation is present, we must disable this
> > built-in default
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  src/remote/remote_daemon.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
> > index 7195ac9218..43409edd24 100644
> > --- a/src/remote/remote_daemon.c
> > +++ b/src/remote/remote_daemon.c
> > @@ -423,11 +423,20 @@ daemonSetupNetworking(virNetServerPtr srv,
> >          return -1;
> >
> >  #ifdef WITH_IP
> > +# ifdef (LIBVIRTD
> 
> ^fails to compile:
> s/(//

Sigh, clearly I didn't test :-(

> 
> >      if (act && ipsock) {
> >          VIR_ERROR(_("--listen parameter not permitted with systemd activation "
> >                      "sockets, see 'man libvirtd' for further guidance"));
> >          return -1;
> >      }
> > +# else /* ! LIBVIRTD */
> > +    /* We don't have a --listen arg with virtproxyd, we're just
> > +     * hardcoded to assume --listen. Thus with systemd we must
> > +     * change that default
> > +     */
> > +    if (act)
> > +        ipsock = 0;
> 
> I'm a bit confused with this bit wrt to what actually happens later in the
> code. Basically this @ipsock is only relevant up until the point where we start
> registering services listening for traffic e.g.virNetServerAddServiceTCP (this
> is one is easier as an example). If I look at the condition:
> 
> if (((ipsock && config->listen_tcp) || act) ...
> 
> why does it even matter that we clear ipsock when socket activation is enabled?
> The condition is true regardless of @ipsock and it's also not populated further
> into the function being called unlike @act, so this bit is making me confused,
> so what exactly is happening if we don't clear @ipsock with virtproxyd?

Currently there are *no* ill effects if we don't clear @ipsock. The
important bit of the patch is simply avoiding the VIR_ERROR in the
first part of the ifdef.  I chose to clear @ipsock, to reduce chance
of suprises later if we refactor again.

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] remote: fix systemd IP socket activation with virtproxyd
Posted by Erik Skultety 4 years, 6 months ago 
On Fri, Sep 27, 2019 at 01:09:31PM +0100, Daniel P. Berrangé wrote:
> On Wed, Sep 25, 2019 at 09:42:57AM +0200, Erik Skultety wrote:
> > On Tue, Sep 24, 2019 at 04:07:17PM +0100, Daniel P. Berrangé wrote:
> > > We recently forbid the use of --listen with socket activation:
> > >
> > >   commit 3a6a725b8f575890ee6c151ad1f46ea0ceea1f3b
> > >   Author: Daniel P. Berrangé <berrange@redhat.com>
> > >   Date:   Thu Aug 22 14:52:16 2019 +0100
> > >
> > >       remote: forbid the --listen arg when systemd socket activation
> > >
> > > In this change we forgot that virtproxyd doesn't have a --listen
> > > parameter, and instead behaves as if it was always present. Thus
> > > when systemd socket activation is present, we must disable this
> > > built-in default
> > >
> > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > ---
> > >  src/remote/remote_daemon.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
> > > index 7195ac9218..43409edd24 100644
> > > --- a/src/remote/remote_daemon.c
> > > +++ b/src/remote/remote_daemon.c
> > > @@ -423,11 +423,20 @@ daemonSetupNetworking(virNetServerPtr srv,
> > >          return -1;
> > >
> > >  #ifdef WITH_IP
> > > +# ifdef (LIBVIRTD
> >
> > ^fails to compile:
> > s/(//
>
> Sigh, clearly I didn't test :-(
>
> >
> > >      if (act && ipsock) {
> > >          VIR_ERROR(_("--listen parameter not permitted with systemd activation "
> > >                      "sockets, see 'man libvirtd' for further guidance"));
> > >          return -1;
> > >      }
> > > +# else /* ! LIBVIRTD */
> > > +    /* We don't have a --listen arg with virtproxyd, we're just
> > > +     * hardcoded to assume --listen. Thus with systemd we must
> > > +     * change that default
> > > +     */
> > > +    if (act)
> > > +        ipsock = 0;
> >
> > I'm a bit confused with this bit wrt to what actually happens later in the
> > code. Basically this @ipsock is only relevant up until the point where we start
> > registering services listening for traffic e.g.virNetServerAddServiceTCP (this
> > is one is easier as an example). If I look at the condition:
> >
> > if (((ipsock && config->listen_tcp) || act) ...
> >
> > why does it even matter that we clear ipsock when socket activation is enabled?
> > The condition is true regardless of @ipsock and it's also not populated further
> > into the function being called unlike @act, so this bit is making me confused,
> > so what exactly is happening if we don't clear @ipsock with virtproxyd?
>
> Currently there are *no* ill effects if we don't clear @ipsock. The
> important bit of the patch is simply avoiding the VIR_ERROR in the
> first part of the ifdef.  I chose to clear @ipsock, to reduce chance
> of suprises later if we refactor again.

Ah, thanks for explanation. With the build fix:
Reviewed-by: Erik Skultety <eskultet@redhat.com>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] remote: fix systemd IP socket activation with virtproxyd
Posted by Daniel P. Berrangé 4 years, 6 months ago 
On Fri, Sep 27, 2019 at 02:15:45PM +0200, Erik Skultety wrote:
> On Fri, Sep 27, 2019 at 01:09:31PM +0100, Daniel P. Berrangé wrote:
> > On Wed, Sep 25, 2019 at 09:42:57AM +0200, Erik Skultety wrote:
> > > On Tue, Sep 24, 2019 at 04:07:17PM +0100, Daniel P. Berrangé wrote:
> > > > We recently forbid the use of --listen with socket activation:
> > > >
> > > >   commit 3a6a725b8f575890ee6c151ad1f46ea0ceea1f3b
> > > >   Author: Daniel P. Berrangé <berrange@redhat.com>
> > > >   Date:   Thu Aug 22 14:52:16 2019 +0100
> > > >
> > > >       remote: forbid the --listen arg when systemd socket activation
> > > >
> > > > In this change we forgot that virtproxyd doesn't have a --listen
> > > > parameter, and instead behaves as if it was always present. Thus
> > > > when systemd socket activation is present, we must disable this
> > > > built-in default
> > > >
> > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > > ---
> > > >  src/remote/remote_daemon.c | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
> > > > index 7195ac9218..43409edd24 100644
> > > > --- a/src/remote/remote_daemon.c
> > > > +++ b/src/remote/remote_daemon.c
> > > > @@ -423,11 +423,20 @@ daemonSetupNetworking(virNetServerPtr srv,
> > > >          return -1;
> > > >
> > > >  #ifdef WITH_IP
> > > > +# ifdef (LIBVIRTD
> > >
> > > ^fails to compile:
> > > s/(//
> >
> > Sigh, clearly I didn't test :-(
> >
> > >
> > > >      if (act && ipsock) {
> > > >          VIR_ERROR(_("--listen parameter not permitted with systemd activation "
> > > >                      "sockets, see 'man libvirtd' for further guidance"));
> > > >          return -1;
> > > >      }
> > > > +# else /* ! LIBVIRTD */
> > > > +    /* We don't have a --listen arg with virtproxyd, we're just
> > > > +     * hardcoded to assume --listen. Thus with systemd we must
> > > > +     * change that default
> > > > +     */
> > > > +    if (act)
> > > > +        ipsock = 0;
> > >
> > > I'm a bit confused with this bit wrt to what actually happens later in the
> > > code. Basically this @ipsock is only relevant up until the point where we start
> > > registering services listening for traffic e.g.virNetServerAddServiceTCP (this
> > > is one is easier as an example). If I look at the condition:
> > >
> > > if (((ipsock && config->listen_tcp) || act) ...
> > >
> > > why does it even matter that we clear ipsock when socket activation is enabled?
> > > The condition is true regardless of @ipsock and it's also not populated further
> > > into the function being called unlike @act, so this bit is making me confused,
> > > so what exactly is happening if we don't clear @ipsock with virtproxyd?
> >
> > Currently there are *no* ill effects if we don't clear @ipsock. The
> > important bit of the patch is simply avoiding the VIR_ERROR in the
> > first part of the ifdef.  I chose to clear @ipsock, to reduce chance
> > of suprises later if we refactor again.
> 
> Ah, thanks for explanation. With the build fix:
> Reviewed-by: Erik Skultety <eskultet@redhat.com>

I'm going to comit with this expanded comment since this code is quite
subtle in its behaviour

    /*
     * "ipsock" traditionally reflected whether --listen is set.
     * The listen_tcp & listen_tls params in libvirtd.conf were
     * not honoured unless --listen was set.
     *
     * In virtproxyd we dropped --listen, and have listen_tcp and
     * listen_tls in the config file both default to 0. The user
     * can turn on listening simply by setting the libvirtd.conf
     * file setting and don't have to worry about also adding
     * --listen which is saner.
     *
     * Hence, we initialized ipsock == 1 by default with virtproxyd.
     * When using systemd activation though, we clear ipsock, so
     * later code doesn't have any surprising behaviour differences
     * for virtproxyd vs libvirtd.
     */

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] remote: fix systemd IP socket activation with virtproxyd
Posted by Erik Skultety 4 years, 6 months ago 
On Fri, Sep 27, 2019 at 01:20:51PM +0100, Daniel P. Berrangé wrote:
> On Fri, Sep 27, 2019 at 02:15:45PM +0200, Erik Skultety wrote:
> > On Fri, Sep 27, 2019 at 01:09:31PM +0100, Daniel P. Berrangé wrote:
> > > On Wed, Sep 25, 2019 at 09:42:57AM +0200, Erik Skultety wrote:
> > > > On Tue, Sep 24, 2019 at 04:07:17PM +0100, Daniel P. Berrangé wrote:
> > > > > We recently forbid the use of --listen with socket activation:
> > > > >
> > > > >   commit 3a6a725b8f575890ee6c151ad1f46ea0ceea1f3b
> > > > >   Author: Daniel P. Berrangé <berrange@redhat.com>
> > > > >   Date:   Thu Aug 22 14:52:16 2019 +0100
> > > > >
> > > > >       remote: forbid the --listen arg when systemd socket activation
> > > > >
> > > > > In this change we forgot that virtproxyd doesn't have a --listen
> > > > > parameter, and instead behaves as if it was always present. Thus
> > > > > when systemd socket activation is present, we must disable this
> > > > > built-in default
> > > > >
> > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > > > ---
> > > > >  src/remote/remote_daemon.c | 9 +++++++++
> > > > >  1 file changed, 9 insertions(+)
> > > > >
> > > > > diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
> > > > > index 7195ac9218..43409edd24 100644
> > > > > --- a/src/remote/remote_daemon.c
> > > > > +++ b/src/remote/remote_daemon.c
> > > > > @@ -423,11 +423,20 @@ daemonSetupNetworking(virNetServerPtr srv,
> > > > >          return -1;
> > > > >
> > > > >  #ifdef WITH_IP
> > > > > +# ifdef (LIBVIRTD
> > > >
> > > > ^fails to compile:
> > > > s/(//
> > >
> > > Sigh, clearly I didn't test :-(
> > >
> > > >
> > > > >      if (act && ipsock) {
> > > > >          VIR_ERROR(_("--listen parameter not permitted with systemd activation "
> > > > >                      "sockets, see 'man libvirtd' for further guidance"));
> > > > >          return -1;
> > > > >      }
> > > > > +# else /* ! LIBVIRTD */
> > > > > +    /* We don't have a --listen arg with virtproxyd, we're just
> > > > > +     * hardcoded to assume --listen. Thus with systemd we must
> > > > > +     * change that default
> > > > > +     */
> > > > > +    if (act)
> > > > > +        ipsock = 0;
> > > >
> > > > I'm a bit confused with this bit wrt to what actually happens later in the
> > > > code. Basically this @ipsock is only relevant up until the point where we start
> > > > registering services listening for traffic e.g.virNetServerAddServiceTCP (this
> > > > is one is easier as an example). If I look at the condition:
> > > >
> > > > if (((ipsock && config->listen_tcp) || act) ...
> > > >
> > > > why does it even matter that we clear ipsock when socket activation is enabled?
> > > > The condition is true regardless of @ipsock and it's also not populated further
> > > > into the function being called unlike @act, so this bit is making me confused,
> > > > so what exactly is happening if we don't clear @ipsock with virtproxyd?
> > >
> > > Currently there are *no* ill effects if we don't clear @ipsock. The
> > > important bit of the patch is simply avoiding the VIR_ERROR in the
> > > first part of the ifdef.  I chose to clear @ipsock, to reduce chance
> > > of suprises later if we refactor again.
> >
> > Ah, thanks for explanation. With the build fix:
> > Reviewed-by: Erik Skultety <eskultet@redhat.com>
>
> I'm going to comit with this expanded comment since this code is quite
> subtle in its behaviour
>
>     /*
>      * "ipsock" traditionally reflected whether --listen is set.
>      * The listen_tcp & listen_tls params in libvirtd.conf were
>      * not honoured unless --listen was set.
>      *
>      * In virtproxyd we dropped --listen, and have listen_tcp and
>      * listen_tls in the config file both default to 0. The user
>      * can turn on listening simply by setting the libvirtd.conf
>      * file setting and don't have to worry about also adding
>      * --listen which is saner.
>      *
>      * Hence, we initialized ipsock == 1 by default with virtproxyd.
>      * When using systemd activation though, we clear ipsock, so
>      * later code doesn't have any surprising behaviour differences
>      * for virtproxyd vs libvirtd.
>      */

Appreciated.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.