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