On 29.11.2012 19:30, Laine Stump wrote:

From: Stefan Berger <stefanb@us.ibm.com>

To detect if an interface's nwfilter has changed, we need to also compare the filterparams, which is a hashtable of virNWFilterVarValue. virHashEqual can do this nicely, but requires a pointer to a function that will compare two of the items being stored in the hashes. --- src/conf/nwfilter_params.c | 31 +++++++++++++++++++++++++++++++ src/conf/nwfilter_params.h | 2 ++ src/libvirt_private.syms | 1 + 3 files changed, 34 insertions(+)

diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c index 6dc4baa..3ac1303 100644 --- a/src/conf/nwfilter_params.c +++ b/src/conf/nwfilter_params.c @@ -189,6 +189,37 @@ virNWFilterVarValueGetCardinality(const virNWFilterVarValuePtr val) return 0; }

+bool +virNWFilterVarValueEqual(const virNWFilterVarValuePtr a, + const virNWFilterVarValuePtr b) +{ + unsigned int card, i, j; + const char *s; + + if (!a || !b) + return false; + + card = virNWFilterVarValueGetCardinality(a); + if (card != virNWFilterVarValueGetCardinality(b)) + return false; + + /* brute force O(n^2) comparison */ + for (i = 0; i < card; i++) { + bool eq = false; + + s = virNWFilterVarValueGetNthValue(a, i); + for (j = 0; j < card; j++) { + if (STREQ_NULLABLE(s, virNWFilterVarValueGetNthValue(b, j))) { + eq = true; + break; + } + } + if (!eq) + return false; + } + return true; +} +

Seems reasonable. The quadratic time complexity could be avoided if @a and @b items are sorted. And since this is just a callback to virHashEqual() we shouldn't be doing anything here but comparing.

int virNWFilterVarValueAddValue(virNWFilterVarValuePtr val, char *value) { diff --git a/src/conf/nwfilter_params.h b/src/conf/nwfilter_params.h index cedf9cd..96d3033 100644 --- a/src/conf/nwfilter_params.h +++ b/src/conf/nwfilter_params.h @@ -57,6 +57,8 @@ const char *virNWFilterVarValueGetSimple(const virNWFilterVarValuePtr val); const char *virNWFilterVarValueGetNthValue(virNWFilterVarValuePtr val, unsigned int idx); unsigned int virNWFilterVarValueGetCardinality(const virNWFilterVarValuePtr); +bool virNWFilterVarValueEqual(const virNWFilterVarValuePtr a, + const virNWFilterVarValuePtr b); int virNWFilterVarValueAddValue(virNWFilterVarValuePtr val, char *value); int virNWFilterVarValueDelValue(virNWFilterVarValuePtr val, const char *value);

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2573b8a..ada73fb 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -968,6 +968,7 @@ virNWFilterVarValueCopy; virNWFilterVarValueCreateSimple; virNWFilterVarValueCreateSimpleCopyValue; virNWFilterVarValueDelValue; +virNWFilterVarValueEqual; virNWFilterVarValueFree; virNWFilterVarValueGetCardinality; virNWFilterVarValueGetNthValue;

ACK Michal

On 29.11.2012 19:30, 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. --- src/conf/nwfilter_params.c | 21 ++++++++++++++++++ src/conf/nwfilter_params.h | 4 +++- src/libvirt_private.syms | 1 + src/qemu/qemu_hotplug.c | 55 +++++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 77 insertions(+), 4 deletions(-)

diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c index 3ac1303..7254519 100644 --- a/src/conf/nwfilter_params.c +++ b/src/conf/nwfilter_params.c @@ -795,6 +795,27 @@ err_exit: return -1; }

+/* The general purpose function virNWFilterVarValueEqual returns a + * bool, but the comparison callback for virHashEqual (called below) + * needs to return an int of 0 for == and non-0 for != + */ +static int +virNWFilterVarValueCompare(const void *a, const void *b) +{ + return virNWFilterVarValueEqual((const virNWFilterVarValuePtr) a, + (const virNWFilterVarValuePtr) b) ? 0 : 1; +} + +bool +virNWFilterHashTableEqual(virNWFilterHashTablePtr a, + virNWFilterHashTablePtr b) +{ + if (!(a || b)) + return true; + if (!(a && b)) + return false; + return virHashEqual(a->hashTable, b->hashTable, virNWFilterVarValueCompare); +}

static bool isValidVarName(const char *var) diff --git a/src/conf/nwfilter_params.h b/src/conf/nwfilter_params.h index 96d3033..6c3ce54 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 @@ -87,6 +87,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 ada73fb..5598682 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -952,6 +952,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 0bc2259..5058198 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; }

ACK Michal