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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx
>> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
