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); } But this is broken by design. We let dnsmasq write the PID file and it just so happens that dnsmasq doesn't keep it locked. So we play a guessing game and check whether the pid we've read from the pidfile belongs to a dnsmasq process. Any dnsmasq process, not just the one the pidfile belongs to. We do the same with passt though, and when I pointed that out in my review I was referred to this code. Michal
