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

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(a)redhat.com&gt; >>> --- >>> 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?

Michal