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

[Erik Skultety]

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 at 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 at 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 at redhat.com>