Re: [libvirt PATCH] network: do not assume dnsmasq in networkUpdateState

[Ján Tomko <jtomko@xxxxxxxxxx>]

On a Thursday in 2023, Michal Prívozník wrote:
> On 3/22/23 11:07, Michal Prívozník wrote:
> > On 3/17/23 15:59, Peter Krempa wrote:
> > > On Thu, Mar 16, 2023 at 14:21:27 +0100, Ján Tomko wrote:
> > > > If we don't have dnsmasq, it's pointless to try to find
> > > > its pidfile.
> > > >
> > > > Also, dnsmasqCapsGetBinaryPath would access a NULL pointer.
> > > >
> > > > Fixes: 4b68c982e283471575bacbf87302495864da46fe
> > > > Foxes: https://gitlab.com/libvirt/libvirt/-/issues/456
> > >
> > > Awww ^_^
> > >
> > > > Signed-off-by: Ján Tomko <jtomko@xxxxxxxxxx>
> > > > ---
> > > >  src/network/bridge_driver.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> > > > index 3fa56bfc09..ee4bbd4a93 100644
> > > > --- a/src/network/bridge_driver.c
> > > > +++ b/src/network/bridge_driver.c
> > > > @@ -492,7 +492,7 @@ networkUpdateState(virNetworkObj *obj,
> > > >      virNetworkObjPortForEach(obj, networkUpdatePort, obj);
> > > >
> > > >      /* Try and read dnsmasq pids of active networks */
> > > > -    if (virNetworkObjIsActive(obj) && def->ips && (def->nips > 0)) {
> > > > +    if (dnsmasq_caps && virNetworkObjIsActive(obj) && def->ips && (def->nips > 0)) {
> > > >          pid_t dnsmasqPid;
> > >
> > > Based on the fact that at the beginning of this function we check:
> > >
> > >  if (!virNetworkObjIsActive(obj))
> > >      return 0;
> > >
> > > If we get to this place and don't have the capabilities this must mean
> > > that the 'dnsmasq' binary was removed during runtime, right?
> > >
> > > In such case we should still be able to read the pidfile though as the
> > > process is supposed to be around.
> >
> > Yeah, for this particular bug we may need to go with:
> >
> > diff --git i/src/network/bridge_driver.c w/src/network/bridge_driver.c
> > index 3fa56bfc09..a11a53501f 100644
> > --- i/src/network/bridge_driver.c
> > +++ w/src/network/bridge_driver.c
> > @@ -493,15 +493,19 @@ networkUpdateState(virNetworkObj *obj,
> >
> >      /* Try and read dnsmasq pids of active networks */
> >      if (virNetworkObjIsActive(obj) && def->ips && (def->nips > 0)) {
> > +        const char *binpath = NULL;
> >          pid_t dnsmasqPid;
> >
> >          if (networkSetMacMap(cfg, obj) < 0)
> >              return -1;
> >
> > +        if (dnsmasq_caps)
> > +            binpath = dnsmasqCapsGetBinaryPath(dnsmasq_caps);
> > +
> >          ignore_value(virPidFileReadIfAlive(cfg->pidDir,
> >                                             def->name,
> >                                             &dnsmasqPid,
> > -                                           dnsmasqCapsGetBinaryPath(dnsmasq_caps)));
> > +                                           binpath));
> >          virNetworkObjSetDnsmasqPid(obj, dnsmasqPid);
> >      }
>
> Jano, you do want to resubmit the patch?

I do not.

Going with your proposed changes, we'd lose the pid reuse protection and
I do not have the energy to rewrite how we deal with dnsmasq pidfiles.

Jano

> Michal

Attachment: signature.asc
Description: PGP signature