[libvirt] [PATCH 2/2] nwfilter: Only free inetaddr on virNWFilterIPAddrMapAddIPAddr failure

John Ferlan jferlan at redhat.com
Fri Sep 29 16:12:43 UTC 2017



On 09/29/2017 11:44 AM, Ján Tomko wrote:
> On Thu, Sep 28, 2017 at 03:26:49PM -0400, John Ferlan wrote:
>> Flag when virNWFilterIPAddrMapAddIPAddr to allow deletion - keep
>> @inetaddr around to message after virNWFilterInstantiateFilterLate
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>> src/nwfilter/nwfilter_learnipaddr.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/nwfilter/nwfilter_learnipaddr.c
>> b/src/nwfilter/nwfilter_learnipaddr.c
>> index 5b95f0e61..567897221 100644
>> --- a/src/nwfilter/nwfilter_learnipaddr.c
>> +++ b/src/nwfilter/nwfilter_learnipaddr.c
>> @@ -610,6 +610,7 @@ learnIPAddressThread(void *arg)
>>         sa.data.inet4.sin_family = AF_INET;
>>         sa.data.inet4.sin_addr.s_addr = vmaddr;
>>         char *inetaddr;
>> +        bool failmap = false;
> 
> The name looks confusing. I'd prefer the name to document either what
> happened
> (adding_failed, address_consumed) or what should happen (free_addr). Or
> rather dropping the bool completely, log the IP address before the
> AddIpAddr call and remove it from the other VIR_DEBUG message.

to quote Michal from a recent series "Naming is hard"

I like the idea of logging the IP before...  That resolves some of the
oddities.

>>
>>         /* It is necessary to unlock interface here to avoid
>> updateMutex and
>>          * interface ordering deadlocks. Otherwise we are going to
>> @@ -625,6 +626,7 @@ learnIPAddressThread(void *arg)
>>             if (virNWFilterIPAddrMapAddIPAddr(req->ifname, inetaddr) <
>> 0) {
>>                 VIR_ERROR(_("Failed to add IP address %s to IP address "
>>                           "cache for interface %s"), inetaddr,
>> req->ifname);
>> +                failmap = true;
> 
> This assumes that virNWFilterIPAddrMapAddIPAddr consumes inetaddr only
> if it returns zero. However in a fraction of the unlikely cases when
> this function call can fail (all of them on OOM), it can consume it even
> though it returns -1 -- if virNWFilterVarValueCreateSimple succeeds,
> but virNWFilterHashTablePut fails.
> 

Ugh...  I hate this code.

>>             }
>>
>>             ret = virNWFilterInstantiateFilterLate(req->driver,
>> @@ -637,7 +639,8 @@ learnIPAddressThread(void *arg)
>>                                                    req->filterparams);
>>             VIR_DEBUG("Result from applying firewall rules on "
>>                       "%s with IP addr %s : %d", req->ifname,
>> inetaddr, ret);
> 
> At this point, inetaddr belongs to the hash table protected by the lock
> that AddIPAddr already released. Can we safely access it here?

We have been - it's a VIR_DEBUG though...

I'll work on a followup.

John

> 
> Jan
> 
>> -            VIR_FREE(inetaddr);
>> +            if (failmap)
>> +                VIR_FREE(inetaddr);
>>         }
>>     } else {
>>         if (showError)
>> -- 
>> 2.13.5
>>
>> -- 
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list




More information about the libvir-list mailing list