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

[Ján Tomko]

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 at redhat.com>
>>>> ---
>>>>  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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20230413/a2c78749/attachment.sig>