[libvirt] [PATCH] qemu: support live update of an interface's filter

[Laine Stump]

On 11/21/2012 10:46 PM, Stefan Berger wrote:
> On 11/20/2012 01:49 PM, Laine Stump wrote:
>> Since we can't (currently) rely on the ability to provide blanket
>> support for all possible network changes by calling the toplevel
>> netdev hostside disconnect/connect functions (due to qemu only
>> supporting a lockstep between initialization of host side and guest
>> side of devices), in order to support live change of an interface's
>> nwfilter we need to make a special purpose function to only call the
>> nwfilter teardown and setup functions if the filter for an interface
>> (or its parameters) changes. The pattern is nearly identical to that
>> used to change the bridge that an interface is connected to.
>>
>> This patch was inspired by a request from Guido Winkelmann
>> <guido at sagersystems.de>, who tested an earlier version, and wrote an
>> initial version of the nwfilterHashTable comparison function.
>>
>> I didn't spend any time trying to understand the contents of the
>> nwfilterHashTable entries, or whether the comparison function is
>> fully/correctly comparing the entries. I would appreciate if someone
>> with better knowledge of that data structure (Stefan?) could check it
>> out and provide suggestions.
>> ---
>>   src/conf/nwfilter_params.c | 24 ++++++++++++++++++++
>>   src/conf/nwfilter_params.h |  4 +++-
>>   src/libvirt_private.syms   |  1 +
>>   src/qemu/qemu_hotplug.c    | 55
>> +++++++++++++++++++++++++++++++++++++++++++---
>>   4 files changed, 80 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c
>> index 6dc4baa..e42a54c 100644
>> --- a/src/conf/nwfilter_params.c
>> +++ b/src/conf/nwfilter_params.c
>> @@ -764,6 +764,30 @@ err_exit:
>>       return -1;
>>   }
>>
>> +static int
>> +virNWFilterHashTableCompare(const void *a, const void *b)
>> +{
>> +    /* need to return 0 if equal */
>> +    return STRNEQ_NULLABLE(a, b);
>
> The contents of this table should be of type virNWFilterVarValuePtr
> and therefore would have to call a function comparing two objects of
> this type. The needed function virNWFilterVarValueEqual()
> unfortunately does not exist.
>
>> +}
>> +
>> +bool
>> +virNWFilterHashTableEqual(virNWFilterHashTablePtr a,
>> +                          virNWFilterHashTablePtr b)
>> +{
>> +    if (!(a || b))
>> +        return true;
>> +    if (!(a && b))
>> +        return false;
>
> Do you mean to compare NULL pointers above?
> if (!a || !b)
>     return false;
>
> Simple equality:
>
> if (a == b)
>     return true;

I wasn't looking for equality of two proper hashtables, just ignorantly
eliminating two simple cases - if both are NULL, they're effectively the
same; if one is NULL but the other isn't NULL, then they're definitely
CAN'T be equal.

>
>> +    if (a->nNames != b->nNames)
>> +        return false;
>
> I would not test for this one. The nNames and array depends on whether
> copies of hash map entries were made. If they were added with
> different copy option, then you may end up with different size of
> array here.

Okay. I don't remember if I did this myself or took it from an earlier
attempt by Guido, but at any rate it was based on an incomplete
understanding of the data structure (which is why I was hoping you'd
take a look and pick it apart :-)


>
>> +    if (!(a->hashTable || b->hashTable))
>> +        return true;
>
> Did you mean the following?
>
> if (a->hashTable == b->hashTable)
>     return true;

No. I meant "if both a->hashTable and b->hashTable are NULL, they are
effectively equal (since they're empty)". Does that make sense?

>
> This should be covered by a==b, otherwise something would be wrong.
>
>> +    if (!(a->hashTable && b->hashTable))
>> +        return false;
>
> a->hashTable == NULL -> a should not exist.

If you're certain of this, then I'll remove the above two checks.


>
>> +
>> +    return virHashEqual(a->hashTable, b->hashTable,
>> virNWFilterHashTableCompare);
>> +}
>
>
> The rest I think should work if we get the above in order.

Great! And thanks for the virNWFilterVarValueEqual function you sent me
in private email. I'll put that all together and submit a new patch
tonight or tomorrow morning.