[libvirt] [PATCH] remote: fix systemd IP socket activation with virtproxyd
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
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
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 :|
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>
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 :|
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
participants (2)
-
Daniel P. Berrangé -
Erik Skultety