[libvirt] [PATCH] qemu: support live update of an interface's filter
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@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); +} + +bool +virNWFilterHashTableEqual(virNWFilterHashTablePtr a, + virNWFilterHashTablePtr b) +{ + if (!(a || b)) + return true; + if (!(a && b)) + return false; + if (a->nNames != b->nNames) + return false; + if (!(a->hashTable || b->hashTable)) + return true; + if (!(a->hashTable && b->hashTable)) + return false; + + return virHashEqual(a->hashTable, b->hashTable, virNWFilterHashTableCompare); +} static bool isValidVarName(const char *var) diff --git a/src/conf/nwfilter_params.h b/src/conf/nwfilter_params.h index cedf9cd..e310bb1 100644 --- a/src/conf/nwfilter_params.h +++ b/src/conf/nwfilter_params.h @@ -1,7 +1,7 @@ /* * nwfilter_params.h: parsing and data maintenance of filter parameters * - * Copyright (C) 2011 Red Hat, Inc. + * Copyright (C) 2011-2012 Red Hat, Inc. * Copyright (C) 2010 IBM Corporation * * This library is free software; you can redistribute it and/or @@ -85,6 +85,8 @@ void *virNWFilterHashTableRemoveEntry(virNWFilterHashTablePtr table, const char *name); int virNWFilterHashTablePutAll(virNWFilterHashTablePtr src, virNWFilterHashTablePtr dest); +bool virNWFilterHashTableEqual(virNWFilterHashTablePtr a, + virNWFilterHashTablePtr b); # define VALID_VARNAME \ "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_" diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 756d7bd..d637142 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -946,6 +946,7 @@ virNWFilterIPAddrMapShutdown; # nwfilter_params.h virNWFilterHashTableCreate; +virNWFilterHashTableEqual; virNWFilterHashTableFree; virNWFilterHashTablePut; virNWFilterHashTablePutAll; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index ae8381e..581204a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1330,6 +1330,43 @@ cleanup: return ret; } +static int +qemuDomainChangeNetFilter(virConnectPtr conn, + virDomainObjPtr vm, + virDomainNetDefPtr olddev, + virDomainNetDefPtr newdev) +{ + /* make sure this type of device supports filters. */ + switch (virDomainNetGetActualType(newdev)) { + case VIR_DOMAIN_NET_TYPE_ETHERNET: + case VIR_DOMAIN_NET_TYPE_BRIDGE: + case VIR_DOMAIN_NET_TYPE_NETWORK: + break; + default: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("filters not supported on interfaces of type %s"), + virDomainNetTypeToString(virDomainNetGetActualType(newdev))); + return -1; + } + + virDomainConfNWFilterTeardown(olddev); + + if (virDomainConfNWFilterInstantiate(conn, vm->def->uuid, newdev) < 0) { + virErrorPtr errobj; + + virReportError(VIR_ERR_OPERATION_FAILED, + _("failed to add new filter rules to '%s' " + "- attempting to restore old rules"), + olddev->ifname); + errobj = virSaveLastError(); + ignore_value(virDomainConfNWFilterInstantiate(conn, vm->def->uuid, olddev)); + virSetError(errobj); + virFreeError(errobj); + return -1; + } + return 0; +} + int qemuDomainChangeNetLinkState(struct qemud_driver *driver, virDomainObjPtr vm, virDomainNetDefPtr dev, @@ -1373,6 +1410,7 @@ qemuDomainChangeNet(struct qemud_driver *driver, int oldType, newType; bool needReconnect = false; bool needBridgeChange = false; + bool needFilterChange = false; bool needLinkStateChange = false; bool needReplaceDevDef = false; int ret = -1; @@ -1506,8 +1544,10 @@ qemuDomainChangeNet(struct qemud_driver *driver, } /* (end of device info checks) */ - if (STRNEQ_NULLABLE(olddev->filter, newdev->filter)) - needReconnect = true; + if (STRNEQ_NULLABLE(olddev->filter, newdev->filter) || + !virNWFilterHashTableEqual(olddev->filterparams, newdev->filterparams)) { + needFilterChange = true; + } /* bandwidth can be modified, and will be checked later */ /* vlan can be modified, and will be checked later */ @@ -1665,7 +1705,16 @@ qemuDomainChangeNet(struct qemud_driver *driver, goto cleanup; /* we successfully switched to the new bridge, and we've * determined that the rest of newdev is equivalent to olddev, - * so move newdev into place, so that the */ + * so move newdev into place */ + needReplaceDevDef = true; + } + + if (needFilterChange) { + if (qemuDomainChangeNetFilter(dom->conn, vm, olddev, newdev) < 0) + goto cleanup; + /* we successfully switched to the new filter, and we've + * determined that the rest of newdev is equivalent to olddev, + * so move newdev into place */ needReplaceDevDef = true; } -- 1.7.11.7
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@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;
+ 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.
+ if (!(a->hashTable || b->hashTable)) + return true;
Did you mean the following? if (a->hashTable == b->hashTable) return true; 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.
+ + return virHashEqual(a->hashTable, b->hashTable, virNWFilterHashTableCompare); +}
The rest I think should work if we get the above in order. Regards, Stefan
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@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.
participants (2)
-
Laine Stump -
Stefan Berger