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é <berra...@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é <berra...@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 <eskul...@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
