[libvirt] [PATCH v3 00/20] nwfilter: refactor the driver to make it independent of virt drivers

v1: https://www.redhat.com/archives/libvir-list/2018-April/msg02616.html v2: https://www.redhat.com/archives/libvir-list/2018-May/msg01145.html Today the nwfilter driver is entangled with the virt drivers in both directions. At various times when rebuilding filters nwfilter will call out to the virt driver to iterate over running guest's NICs. This has caused very complicated lock ordering rules to be required. If we are to split the virt drivers out into separate daemons we need to get rid of this coupling since we don't want the separate daemons calling each other, as that risks deadlock if all of the RPC workers are busy. The obvious way to solve this is to have the nwfilter driver remember all the filters it has active, avoiding the need to iterate over running guests. Still todo - Document the new XML format Changed in v3: - Updated API version numbers - Use accessors for virNWFilterBindingObjPtr struct - Other fixes John mentioned Changed in v2: - The virNWFilterBindingPtr was renamed virNWFilterBindingDefPtr - New virNWFilterBindingObjPtr & virNWFilterBindingObjListPtr structs added to track the objects in the driver - New virNWFilterBindingPtr public API type was added - New public APIs for listing filter bindings, querying XML, and creating/deleting them - Convert the virt drivers to use the public API for creating and deleting bindings - Persistent active bindings out to disk so they're preserved across restarts - Added RNG schema and XML-2-XML test - New virsh commands for listing/querying XML/creating/deleting bindings Daniel P. Berrangé (20): conf: change virNWFilterBindingPtr to virNWFilterBindingDefPtr conf: add missing virxml.h include for nwfilter_params.h conf: move virNWFilterBindingDefPtr into its own files conf: add support for parsing/formatting virNWFilterBindingDefPtr schemas: add schema for nwfilter binding XML document nwfilter: export port binding concept in the public API access: add nwfilter binding object permissions remote: add support for nwfilter binding objects virsh: add nwfilter binding commands nwfilter: convert the gentech driver code to use virNWFilterBindingDefPtr nwfilter: convert IP address learning code to virNWFilterBindingDefPtr nwfilter: convert DHCP address snooping code to virNWFilterBindingDefPtr conf: report an error if nic needs filtering by no driver is present conf: introduce a virNWFilterBindingObjPtr struct conf: introduce a virNWFilterBindingObjListPtr struct nwfilter: keep track of active filter bindings nwfilter: remove virt driver callback layer for rebuilding filters nwfilter: wire up new APIs for listing and querying filter bindings nwfilter: wire up new APIs for creating and deleting nwfilter bindings nwfilter: convert virt drivers to use public API for nwfilter bindings docs/schemas/domaincommon.rng | 27 +- docs/schemas/nwfilter.rng | 29 +- docs/schemas/nwfilter_params.rng | 32 ++ docs/schemas/nwfilterbinding.rng | 49 ++ include/libvirt/libvirt-nwfilter.h | 39 ++ include/libvirt/virterror.h | 2 + src/access/viraccessdriver.h | 5 + src/access/viraccessdrivernop.c | 10 + src/access/viraccessdriverpolkit.c | 21 + src/access/viraccessdriverstack.c | 24 + src/access/viraccessmanager.c | 15 + src/access/viraccessmanager.h | 5 + src/access/viraccessperm.c | 7 +- src/access/viraccessperm.h | 38 ++ src/conf/Makefile.inc.am | 6 + src/conf/domain_nwfilter.c | 125 ++++- src/conf/domain_nwfilter.h | 13 - src/conf/nwfilter_conf.c | 188 +------ src/conf/nwfilter_conf.h | 68 +-- src/conf/nwfilter_params.h | 1 + src/conf/virnwfilterbindingdef.c | 280 ++++++++++ src/conf/virnwfilterbindingdef.h | 65 +++ src/conf/virnwfilterbindingobj.c | 299 +++++++++++ src/conf/virnwfilterbindingobj.h | 69 +++ src/conf/virnwfilterbindingobjlist.c | 487 ++++++++++++++++++ src/conf/virnwfilterbindingobjlist.h | 69 +++ src/conf/virnwfilterobj.c | 4 +- src/conf/virnwfilterobj.h | 4 + src/datatypes.c | 67 +++ src/datatypes.h | 31 ++ src/driver-nwfilter.h | 30 ++ src/libvirt-nwfilter.c | 305 +++++++++++ src/libvirt_private.syms | 45 +- src/libvirt_public.syms | 9 + src/lxc/lxc_driver.c | 28 - src/nwfilter/nwfilter_dhcpsnoop.c | 158 +++--- src/nwfilter/nwfilter_dhcpsnoop.h | 7 +- src/nwfilter/nwfilter_driver.c | 218 ++++++-- src/nwfilter/nwfilter_gentech_driver.c | 337 ++++++------ src/nwfilter/nwfilter_gentech_driver.h | 22 +- src/nwfilter/nwfilter_learnipaddr.c | 104 ++-- src/nwfilter/nwfilter_learnipaddr.h | 7 +- src/qemu/qemu_driver.c | 25 - src/remote/remote_daemon_dispatch.c | 15 + src/remote/remote_driver.c | 20 + src/remote/remote_protocol.x | 90 +++- src/remote_protocol-structs | 43 ++ src/rpc/gendispatch.pl | 15 +- src/uml/uml_driver.c | 29 -- src/util/virerror.c | 12 + tests/Makefile.am | 7 + .../filter-vars.xml | 11 + .../virnwfilterbindingxml2xmldata/simple.xml | 9 + tests/virnwfilterbindingxml2xmltest.c | 112 ++++ tests/virschematest.c | 1 + tools/virsh-completer.c | 45 ++ tools/virsh-completer.h | 4 + tools/virsh-nwfilter.c | 317 ++++++++++++ tools/virsh-nwfilter.h | 8 + 59 files changed, 3283 insertions(+), 829 deletions(-) create mode 100644 docs/schemas/nwfilter_params.rng create mode 100644 docs/schemas/nwfilterbinding.rng create mode 100644 src/conf/virnwfilterbindingdef.c create mode 100644 src/conf/virnwfilterbindingdef.h create mode 100644 src/conf/virnwfilterbindingobj.c create mode 100644 src/conf/virnwfilterbindingobj.h create mode 100644 src/conf/virnwfilterbindingobjlist.c create mode 100644 src/conf/virnwfilterbindingobjlist.h create mode 100644 tests/virnwfilterbindingxml2xmldata/filter-vars.xml create mode 100644 tests/virnwfilterbindingxml2xmldata/simple.xml create mode 100644 tests/virnwfilterbindingxml2xmltest.c -- 2.17.0

We are going to want to expose the NWFilter binding concept in the public API, so the virNWFilterBindingPtr type needs to be used there. Our internal type will shortly gain an XML representation, so rename it to virNWFilterBindingDefPtr which follows our normal conventions. Reviewed-by: John Ferlan <jferlan@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/nwfilter_conf.c | 24 ++++++++++++------------ src/conf/nwfilter_conf.h | 12 ++++++------ src/libvirt_private.syms | 4 ++-- 3 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 452daa214a..6422f6b8ea 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -3268,25 +3268,25 @@ virNWFilterRuleIsProtocolEthernet(virNWFilterRuleDefPtr rule) void -virNWFilterBindingFree(virNWFilterBindingPtr binding) +virNWFilterBindingDefFree(virNWFilterBindingDefPtr def) { - if (!binding) + if (!def) return; - VIR_FREE(binding->ownername); - VIR_FREE(binding->portdevname); - VIR_FREE(binding->linkdevname); - VIR_FREE(binding->filter); - virHashFree(binding->filterparams); + VIR_FREE(def->ownername); + VIR_FREE(def->portdevname); + VIR_FREE(def->linkdevname); + VIR_FREE(def->filter); + virHashFree(def->filterparams); - VIR_FREE(binding); + VIR_FREE(def); } -virNWFilterBindingPtr -virNWFilterBindingCopy(virNWFilterBindingPtr src) +virNWFilterBindingDefPtr +virNWFilterBindingDefCopy(virNWFilterBindingDefPtr src) { - virNWFilterBindingPtr ret; + virNWFilterBindingDefPtr ret; if (VIR_ALLOC(ret) < 0) return NULL; @@ -3316,6 +3316,6 @@ virNWFilterBindingCopy(virNWFilterBindingPtr src) return ret; error: - virNWFilterBindingFree(ret); + virNWFilterBindingDefFree(ret); return NULL; } diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 6fcbba4bd5..c72171f2f2 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -545,10 +545,10 @@ struct _virNWFilterDef { virNWFilterEntryPtr *filterEntries; }; -typedef struct virNWFilterBinding virNWFilterBinding; -typedef virNWFilterBinding *virNWFilterBindingPtr; +typedef struct _virNWFilterBindingDef virNWFilterBindingDef; +typedef virNWFilterBindingDef *virNWFilterBindingDefPtr; -struct virNWFilterBinding { +struct _virNWFilterBindingDef { char *ownername; unsigned char owneruuid[VIR_UUID_BUFLEN]; char *portdevname; @@ -664,9 +664,9 @@ bool virNWFilterRuleIsProtocolEthernet(virNWFilterRuleDefPtr rule); void -virNWFilterBindingFree(virNWFilterBindingPtr binding); -virNWFilterBindingPtr -virNWFilterBindingCopy(virNWFilterBindingPtr src); +virNWFilterBindingDefFree(virNWFilterBindingDefPtr binding); +virNWFilterBindingDefPtr +virNWFilterBindingDefCopy(virNWFilterBindingDefPtr src); VIR_ENUM_DECL(virNWFilterRuleAction); VIR_ENUM_DECL(virNWFilterRuleDirection); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ea24f2847c..a8e9469286 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -788,8 +788,8 @@ virDomainNumatuneSpecifiedMaxNode; # conf/nwfilter_conf.h -virNWFilterBindingCopy; -virNWFilterBindingFree; +virNWFilterBindingDefCopy; +virNWFilterBindingDefFree; virNWFilterCallbackDriversLock; virNWFilterCallbackDriversUnlock; virNWFilterChainSuffixTypeToString; -- 2.17.0

The nwfilter_params.h header references the xmlNodePtr type, so must include the virxml.h header to get the libxml2 types defined. Reviewed-by: John Ferlan <jferlan@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/nwfilter_params.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/nwfilter_params.h b/src/conf/nwfilter_params.h index 9bdf65c033..f7355c37df 100644 --- a/src/conf/nwfilter_params.h +++ b/src/conf/nwfilter_params.h @@ -26,6 +26,7 @@ # include "virhash.h" # include "virbuffer.h" # include "virmacaddr.h" +# include "virxml.h" typedef enum { NWFILTER_VALUE_TYPE_SIMPLE, -- 2.17.0

There's no code sharing between virNWFilterDef and virNWFilterBindingDefPtr types, so it is clearer if they live in separate source files and headers. Reviewed-by: John Ferlan <jferlan@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/Makefile.inc.am | 2 + src/conf/nwfilter_conf.c | 54 --------------------- src/conf/nwfilter_conf.h | 17 ------- src/conf/virnwfilterbindingdef.c | 83 ++++++++++++++++++++++++++++++++ src/conf/virnwfilterbindingdef.h | 47 ++++++++++++++++++ src/libvirt_private.syms | 7 ++- 6 files changed, 137 insertions(+), 73 deletions(-) create mode 100644 src/conf/virnwfilterbindingdef.c create mode 100644 src/conf/virnwfilterbindingdef.h diff --git a/src/conf/Makefile.inc.am b/src/conf/Makefile.inc.am index 6d7b0f076b..f5fb323233 100644 --- a/src/conf/Makefile.inc.am +++ b/src/conf/Makefile.inc.am @@ -85,6 +85,8 @@ NWFILTER_CONF_SOURCES = \ conf/nwfilter_conf.h \ conf/virnwfilterobj.c \ conf/virnwfilterobj.h \ + conf/virnwfilterbindingdef.c \ + conf/virnwfilterbindingdef.h \ $(NULL) STORAGE_CONF_SOURCES = \ diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 6422f6b8ea..de26a6d034 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -3265,57 +3265,3 @@ virNWFilterRuleIsProtocolEthernet(virNWFilterRuleDefPtr rule) return true; return false; } - - -void -virNWFilterBindingDefFree(virNWFilterBindingDefPtr def) -{ - if (!def) - return; - - VIR_FREE(def->ownername); - VIR_FREE(def->portdevname); - VIR_FREE(def->linkdevname); - VIR_FREE(def->filter); - virHashFree(def->filterparams); - - VIR_FREE(def); -} - - -virNWFilterBindingDefPtr -virNWFilterBindingDefCopy(virNWFilterBindingDefPtr src) -{ - virNWFilterBindingDefPtr ret; - - if (VIR_ALLOC(ret) < 0) - return NULL; - - if (VIR_STRDUP(ret->ownername, src->ownername) < 0) - goto error; - - memcpy(ret->owneruuid, src->owneruuid, sizeof(ret->owneruuid)); - - if (VIR_STRDUP(ret->portdevname, src->portdevname) < 0) - goto error; - - if (VIR_STRDUP(ret->linkdevname, src->linkdevname) < 0) - goto error; - - ret->mac = src->mac; - - if (VIR_STRDUP(ret->filter, src->filter) < 0) - goto error; - - if (!(ret->filterparams = virNWFilterHashTableCreate(0))) - goto error; - - if (virNWFilterHashTablePutAll(src->filterparams, ret->filterparams) < 0) - goto error; - - return ret; - - error: - virNWFilterBindingDefFree(ret); - return NULL; -} diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index c72171f2f2..08fc07c55c 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -545,19 +545,6 @@ struct _virNWFilterDef { virNWFilterEntryPtr *filterEntries; }; -typedef struct _virNWFilterBindingDef virNWFilterBindingDef; -typedef virNWFilterBindingDef *virNWFilterBindingDefPtr; - -struct _virNWFilterBindingDef { - char *ownername; - unsigned char owneruuid[VIR_UUID_BUFLEN]; - char *portdevname; - char *linkdevname; - virMacAddr mac; - char *filter; - virHashTablePtr filterparams; -}; - typedef enum { STEP_APPLY_NEW, @@ -663,10 +650,6 @@ virNWFilterRuleIsProtocolIPv6(virNWFilterRuleDefPtr rule); bool virNWFilterRuleIsProtocolEthernet(virNWFilterRuleDefPtr rule); -void -virNWFilterBindingDefFree(virNWFilterBindingDefPtr binding); -virNWFilterBindingDefPtr -virNWFilterBindingDefCopy(virNWFilterBindingDefPtr src); VIR_ENUM_DECL(virNWFilterRuleAction); VIR_ENUM_DECL(virNWFilterRuleDirection); diff --git a/src/conf/virnwfilterbindingdef.c b/src/conf/virnwfilterbindingdef.c new file mode 100644 index 0000000000..c7533d4063 --- /dev/null +++ b/src/conf/virnwfilterbindingdef.c @@ -0,0 +1,83 @@ +/* + * virnwfilterbindingdef.c: network filter binding XML processing + * + * Copyright (C) 2018 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#include "viralloc.h" +#include "virerror.h" +#include "virstring.h" +#include "nwfilter_params.h" +#include "virnwfilterbindingdef.h" + + +#define VIR_FROM_THIS VIR_FROM_NWFILTER + +void +virNWFilterBindingDefFree(virNWFilterBindingDefPtr def) +{ + if (!def) + return; + + VIR_FREE(def->ownername); + VIR_FREE(def->portdevname); + VIR_FREE(def->linkdevname); + VIR_FREE(def->filter); + virHashFree(def->filterparams); + + VIR_FREE(def); +} + + +virNWFilterBindingDefPtr +virNWFilterBindingDefCopy(virNWFilterBindingDefPtr src) +{ + virNWFilterBindingDefPtr ret; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + if (VIR_STRDUP(ret->ownername, src->ownername) < 0) + goto error; + + memcpy(ret->owneruuid, src->owneruuid, sizeof(ret->owneruuid)); + + if (VIR_STRDUP(ret->portdevname, src->portdevname) < 0) + goto error; + + if (VIR_STRDUP(ret->linkdevname, src->linkdevname) < 0) + goto error; + + ret->mac = src->mac; + + if (VIR_STRDUP(ret->filter, src->filter) < 0) + goto error; + + if (!(ret->filterparams = virNWFilterHashTableCreate(0))) + goto error; + + if (virNWFilterHashTablePutAll(src->filterparams, ret->filterparams) < 0) + goto error; + + return ret; + + error: + virNWFilterBindingDefFree(ret); + return NULL; +} diff --git a/src/conf/virnwfilterbindingdef.h b/src/conf/virnwfilterbindingdef.h new file mode 100644 index 0000000000..e3b18af151 --- /dev/null +++ b/src/conf/virnwfilterbindingdef.h @@ -0,0 +1,47 @@ +/* + * virnwfilterbindingdef.h: network filter binding XML processing + * + * Copyright (C) 2018 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ +#ifndef VIR_NWFILTER_BINDING_DEF_H +# define VIR_NWFILTER_BINDING_DEF_H + +# include "internal.h" +# include "virmacaddr.h" +# include "virhash.h" + +typedef struct _virNWFilterBindingDef virNWFilterBindingDef; +typedef virNWFilterBindingDef *virNWFilterBindingDefPtr; + +struct _virNWFilterBindingDef { + char *ownername; + unsigned char owneruuid[VIR_UUID_BUFLEN]; + char *portdevname; + char *linkdevname; + virMacAddr mac; + char *filter; + virHashTablePtr filterparams; +}; + + +void +virNWFilterBindingDefFree(virNWFilterBindingDefPtr binding); +virNWFilterBindingDefPtr +virNWFilterBindingDefCopy(virNWFilterBindingDefPtr src); + +#endif /* VIR_NWFILTER_BINDING_DEF_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a8e9469286..8518a853ff 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -788,8 +788,6 @@ virDomainNumatuneSpecifiedMaxNode; # conf/nwfilter_conf.h -virNWFilterBindingDefCopy; -virNWFilterBindingDefFree; virNWFilterCallbackDriversLock; virNWFilterCallbackDriversUnlock; virNWFilterChainSuffixTypeToString; @@ -1050,6 +1048,11 @@ virNodeDeviceObjListNumOfDevices; virNodeDeviceObjListRemove; +# conf/virnwfilterbindingdef.h +virNWFilterBindingDefCopy; +virNWFilterBindingDefFree; + + # conf/virnwfilterobj.h virNWFilterObjGetDef; virNWFilterObjGetNewDef; -- 2.17.0

A typical XML representation of the virNWFilterBindingDefPtr struct looks like this: <filterbinding> <owner> <name>f25arm7</name> <uuid>12ac8b8c-4f23-4248-ae42-fdcd50c400fd</uuid> </owner> <portdev name='vnet1'/> <mac address='52:54:00:9d:81:b1'/> <filterref filter='clean-traffic'> <parameter name='MAC' value='52:54:00:9d:81:b1'/> </filterref> </filterbinding> Reviewed-by: John Ferlan <jferlan@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/virnwfilterbindingdef.c | 197 ++++++++++++++++++ src/conf/virnwfilterbindingdef.h | 18 ++ src/libvirt_private.syms | 5 + tests/Makefile.am | 7 + .../filter-vars.xml | 11 + .../virnwfilterbindingxml2xmldata/simple.xml | 9 + tests/virnwfilterbindingxml2xmltest.c | 112 ++++++++++ 7 files changed, 359 insertions(+) create mode 100644 tests/virnwfilterbindingxml2xmldata/filter-vars.xml create mode 100644 tests/virnwfilterbindingxml2xmldata/simple.xml create mode 100644 tests/virnwfilterbindingxml2xmltest.c diff --git a/src/conf/virnwfilterbindingdef.c b/src/conf/virnwfilterbindingdef.c index c7533d4063..facca61833 100644 --- a/src/conf/virnwfilterbindingdef.c +++ b/src/conf/virnwfilterbindingdef.c @@ -25,6 +25,7 @@ #include "virstring.h" #include "nwfilter_params.h" #include "virnwfilterbindingdef.h" +#include "viruuid.h" #define VIR_FROM_THIS VIR_FROM_NWFILTER @@ -81,3 +82,199 @@ virNWFilterBindingDefCopy(virNWFilterBindingDefPtr src) virNWFilterBindingDefFree(ret); return NULL; } + + +static virNWFilterBindingDefPtr +virNWFilterBindingDefParseXML(xmlXPathContextPtr ctxt) +{ + virNWFilterBindingDefPtr ret; + char *uuid = NULL; + char *mac = NULL; + xmlNodePtr node; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + ret->portdevname = virXPathString("string(./portdev/@name)", ctxt); + if (!ret->portdevname) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("filter binding has no port dev name")); + goto cleanup; + } + + if (virXPathNode("./linkdev", ctxt)) { + ret->linkdevname = virXPathString("string(./linkdev/@name)", ctxt); + if (!ret->linkdevname) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("filter binding has no link dev name")); + goto cleanup; + } + } + + ret->ownername = virXPathString("string(./owner/name)", ctxt); + if (!ret->ownername) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("filter binding has no owner name")); + goto cleanup; + } + + uuid = virXPathString("string(./owner/uuid)", ctxt); + if (!uuid) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("filter binding has no owner UUID")); + goto cleanup; + } + + if (virUUIDParse(uuid, ret->owneruuid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to parse UUID '%s'"), uuid); + VIR_FREE(uuid); + goto cleanup; + } + VIR_FREE(uuid); + + mac = virXPathString("string(./mac/@address)", ctxt); + if (!mac) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("filter binding has no MAC address")); + goto cleanup; + } + + if (virMacAddrParse(mac, &ret->mac) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to parse MAC '%s'"), mac); + VIR_FREE(mac); + goto cleanup; + } + VIR_FREE(mac); + + ret->filter = virXPathString("string(./filterref/@filter)", ctxt); + if (!ret->filter) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("filter binding has no filter reference")); + goto cleanup; + } + + node = virXPathNode("./filterref", ctxt); + if (node && + !(ret->filterparams = virNWFilterParseParamAttributes(node))) + goto cleanup; + + return ret; + + cleanup: + virNWFilterBindingDefFree(ret); + return NULL; +} + + +virNWFilterBindingDefPtr +virNWFilterBindingDefParseNode(xmlDocPtr xml, + xmlNodePtr root) +{ + xmlXPathContextPtr ctxt = NULL; + virNWFilterBindingDefPtr def = NULL; + + if (STRNEQ((const char *)root->name, "filterbinding")) { + virReportError(VIR_ERR_XML_ERROR, + "%s", + _("unknown root element for nwfilter binding")); + goto cleanup; + } + + ctxt = xmlXPathNewContext(xml); + if (ctxt == NULL) { + virReportOOMError(); + goto cleanup; + } + + ctxt->node = root; + def = virNWFilterBindingDefParseXML(ctxt); + + cleanup: + xmlXPathFreeContext(ctxt); + return def; +} + + +static virNWFilterBindingDefPtr +virNWFilterBindingDefParse(const char *xmlStr, + const char *filename) +{ + virNWFilterBindingDefPtr def = NULL; + xmlDocPtr xml; + + if ((xml = virXMLParse(filename, xmlStr, _("(nwfilterbinding_definition)")))) { + def = virNWFilterBindingDefParseNode(xml, xmlDocGetRootElement(xml)); + xmlFreeDoc(xml); + } + + return def; +} + + +virNWFilterBindingDefPtr +virNWFilterBindingDefParseString(const char *xmlStr) +{ + return virNWFilterBindingDefParse(xmlStr, NULL); +} + + +virNWFilterBindingDefPtr +virNWFilterBindingDefParseFile(const char *filename) +{ + return virNWFilterBindingDefParse(NULL, filename); +} + + +char * +virNWFilterBindingDefFormat(const virNWFilterBindingDef *def) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (virNWFilterBindingDefFormatBuf(&buf, def) < 0) { + virBufferFreeAndReset(&buf); + return NULL; + } + + if (virBufferCheckError(&buf) < 0) + return NULL; + + return virBufferContentAndReset(&buf); +} + + +int +virNWFilterBindingDefFormatBuf(virBufferPtr buf, + const virNWFilterBindingDef *def) +{ + char uuid[VIR_UUID_STRING_BUFLEN]; + char mac[VIR_MAC_STRING_BUFLEN]; + + virBufferAddLit(buf, "<filterbinding>\n"); + + virBufferAdjustIndent(buf, 2); + + virBufferAddLit(buf, "<owner>\n"); + virBufferAdjustIndent(buf, 2); + virBufferEscapeString(buf, "<name>%s</name>\n", def->ownername); + virUUIDFormat(def->owneruuid, uuid); + virBufferAsprintf(buf, "<uuid>%s</uuid>\n", uuid); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</owner>\n"); + + virBufferEscapeString(buf, "<portdev name='%s'/>\n", def->portdevname); + if (def->linkdevname) + virBufferEscapeString(buf, "<linkdev name='%s'/>\n", def->linkdevname); + + virMacAddrFormat(&def->mac, mac); + virBufferAsprintf(buf, "<mac address='%s'/>\n", mac); + + if (virNWFilterFormatParamAttributes(buf, def->filterparams, def->filter) < 0) + return -1; + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</filterbinding>\n"); + + return 0; +} diff --git a/src/conf/virnwfilterbindingdef.h b/src/conf/virnwfilterbindingdef.h index e3b18af151..af7fab6064 100644 --- a/src/conf/virnwfilterbindingdef.h +++ b/src/conf/virnwfilterbindingdef.h @@ -24,6 +24,7 @@ # include "internal.h" # include "virmacaddr.h" # include "virhash.h" +# include "virbuffer.h" typedef struct _virNWFilterBindingDef virNWFilterBindingDef; typedef virNWFilterBindingDef *virNWFilterBindingDefPtr; @@ -44,4 +45,21 @@ virNWFilterBindingDefFree(virNWFilterBindingDefPtr binding); virNWFilterBindingDefPtr virNWFilterBindingDefCopy(virNWFilterBindingDefPtr src); +virNWFilterBindingDefPtr +virNWFilterBindingDefParseNode(xmlDocPtr xml, + xmlNodePtr root); + +virNWFilterBindingDefPtr +virNWFilterBindingDefParseString(const char *xml); + +virNWFilterBindingDefPtr +virNWFilterBindingDefParseFile(const char *filename); + +char * +virNWFilterBindingDefFormat(const virNWFilterBindingDef *def); + +int +virNWFilterBindingDefFormatBuf(virBufferPtr buf, + const virNWFilterBindingDef *def); + #endif /* VIR_NWFILTER_BINDING_DEF_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8518a853ff..7f364ec9a1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1050,7 +1050,12 @@ virNodeDeviceObjListRemove; # conf/virnwfilterbindingdef.h virNWFilterBindingDefCopy; +virNWFilterBindingDefFormat; +virNWFilterBindingDefFormatBuf; virNWFilterBindingDefFree; +virNWFilterBindingDefParseFile; +virNWFilterBindingDefParseNode; +virNWFilterBindingDefParseString; # conf/virnwfilterobj.h diff --git a/tests/Makefile.am b/tests/Makefile.am index 99c79e3208..f5872fa42b 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -157,6 +157,7 @@ EXTRA_DIST = \ virmock.h \ virnetdaemondata \ virnetdevtestdata \ + virnwfilterbindingxml2xmldata \ virpcitestdata \ virscsidata \ virsh-uriprecedence \ @@ -352,6 +353,7 @@ test_programs += storagebackendsheepdogtest endif WITH_STORAGE_SHEEPDOG test_programs += nwfilterxml2xmltest +test_programs += virnwfilterbindingxml2xmltest if WITH_NWFILTER test_programs += nwfilterebiptablestest @@ -848,6 +850,11 @@ nwfilterxml2xmltest_SOURCES = \ testutils.c testutils.h nwfilterxml2xmltest_LDADD = $(LDADDS) +virnwfilterbindingxml2xmltest_SOURCES = \ + virnwfilterbindingxml2xmltest.c \ + testutils.c testutils.h +virnwfilterbindingxml2xmltest_LDADD = $(LDADDS) + if WITH_NWFILTER nwfilterebiptablestest_SOURCES = \ nwfilterebiptablestest.c \ diff --git a/tests/virnwfilterbindingxml2xmldata/filter-vars.xml b/tests/virnwfilterbindingxml2xmldata/filter-vars.xml new file mode 100644 index 0000000000..dcff9640ce --- /dev/null +++ b/tests/virnwfilterbindingxml2xmldata/filter-vars.xml @@ -0,0 +1,11 @@ +<filterbinding> + <owner> + <name>memtest</name> + <uuid>d54df46f-1ab5-4a22-8618-4560ef5fac2c</uuid> + </owner> + <portdev name='vnet0'/> + <mac address='52:54:00:7b:35:93'/> + <filterref filter='clean-traffic'> + <parameter name='MAC' value='52:54:00:7b:35:93'/> + </filterref> +</filterbinding> diff --git a/tests/virnwfilterbindingxml2xmldata/simple.xml b/tests/virnwfilterbindingxml2xmldata/simple.xml new file mode 100644 index 0000000000..4577729a3c --- /dev/null +++ b/tests/virnwfilterbindingxml2xmldata/simple.xml @@ -0,0 +1,9 @@ +<filterbinding> + <owner> + <name>memtest</name> + <uuid>d54df46f-1ab5-4a22-8618-4560ef5fac2c</uuid> + </owner> + <portdev name='vnet0'/> + <mac address='52:54:00:7b:35:93'/> + <filterref filter='clean-traffic'/> +</filterbinding> diff --git a/tests/virnwfilterbindingxml2xmltest.c b/tests/virnwfilterbindingxml2xmltest.c new file mode 100644 index 0000000000..a3948084af --- /dev/null +++ b/tests/virnwfilterbindingxml2xmltest.c @@ -0,0 +1,112 @@ +/* + * virnwfilterbindingxml2xmltest.c: network filter binding XML testing + * + * Copyright (C) 2018 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> +#include <string.h> + +#include <sys/types.h> +#include <fcntl.h> + +#include "internal.h" +#include "testutils.h" +#include "virxml.h" +#include "virnwfilterbindingdef.h" +#include "testutilsqemu.h" +#include "virstring.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +static int +testCompareXMLToXMLFiles(const char *xml) +{ + char *actual = NULL; + int ret = -1; + virNWFilterBindingDefPtr dev = NULL; + + virResetLastError(); + + if (!(dev = virNWFilterBindingDefParseFile(xml))) + goto fail; + + if (!(actual = virNWFilterBindingDefFormat(dev))) + goto fail; + + if (virTestCompareToFile(actual, xml) < 0) + goto fail; + + ret = 0; + + fail: + VIR_FREE(actual); + virNWFilterBindingDefFree(dev); + return ret; +} + +typedef struct test_parms { + const char *name; +} test_parms; + +static int +testCompareXMLToXMLHelper(const void *data) +{ + int result = -1; + const test_parms *tp = data; + char *xml = NULL; + + if (virAsprintf(&xml, "%s/virnwfilterbindingxml2xmldata/%s.xml", + abs_srcdir, tp->name) < 0) { + goto cleanup; + } + + result = testCompareXMLToXMLFiles(xml); + + cleanup: + VIR_FREE(xml); + + return result; +} + +static int +mymain(void) +{ + int ret = 0; + +#define DO_TEST(NAME) \ + do { \ + test_parms tp = { \ + .name = NAME, \ + }; \ + if (virTestRun("NWFilter XML-2-XML " NAME, \ + testCompareXMLToXMLHelper, (&tp)) < 0) \ + ret = -1; \ + } while (0) + + DO_TEST("simple"); + DO_TEST("filter-vars"); + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIR_TEST_MAIN(mymain) -- 2.17.0

Reviewed-by: John Ferlan <jferlan@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/schemas/domaincommon.rng | 27 +----------------- docs/schemas/nwfilter.rng | 29 +------------------ docs/schemas/nwfilter_params.rng | 32 +++++++++++++++++++++ docs/schemas/nwfilterbinding.rng | 49 ++++++++++++++++++++++++++++++++ tests/virschematest.c | 1 + 5 files changed, 84 insertions(+), 54 deletions(-) create mode 100644 docs/schemas/nwfilter_params.rng create mode 100644 docs/schemas/nwfilterbinding.rng diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 4a454dddb4..3f0c780ef3 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -5,6 +5,7 @@ <include href='storagecommon.rng'/> <include href='networkcommon.rng'/> <include href='cputypes.rng'/> + <include href='nwfilter_params.rng'/> <!-- description and title element, may be placed anywhere under the root @@ -5394,22 +5395,6 @@ </element> </define> - <define name="filterref-node-attributes"> - <attribute name="filter"> - <data type="NCName"/> - </attribute> - <zeroOrMore> - <element name="parameter"> - <attribute name="name"> - <ref name="filter-param-name"/> - </attribute> - <attribute name="value"> - <ref name="filter-param-value"/> - </attribute> - </element> - </zeroOrMore> - </define> - <define name="deviceBoot"> <element name="boot"> <attribute name="order"> @@ -6136,16 +6121,6 @@ <param name="pattern">[a-zA-Z0-9_\.\+\-/]+</param> </data> </define> - <define name="filter-param-name"> - <data type="string"> - <param name="pattern">[a-zA-Z0-9_]+</param> - </data> - </define> - <define name="filter-param-value"> - <data type="string"> - <param name="pattern">[a-zA-Z0-9_\.:]+</param> - </data> - </define> <define name="spaprvioReg"> <data type="string"> <param name="pattern">(0x)?[0-9a-fA-F]{1,16}</param> diff --git a/docs/schemas/nwfilter.rng b/docs/schemas/nwfilter.rng index cca6ff2954..05a79da112 100644 --- a/docs/schemas/nwfilter.rng +++ b/docs/schemas/nwfilter.rng @@ -1,6 +1,7 @@ <?xml version="1.0" encoding="UTF-8"?> <grammar ns="" xmlns="http://relaxng.org/ns/structure/1.0" datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes"> <include href='basictypes.rng'/> + <include href='nwfilter_params.rng'/> <start> <ref name="filter"/> </start> @@ -245,22 +246,6 @@ </optional> </define> - <define name="filterref-node-attributes"> - <attribute name="filter"> - <data type="NCName"/> - </attribute> - <zeroOrMore> - <element name="parameter"> - <attribute name="name"> - <ref name="filter-param-name"/> - </attribute> - <attribute name="value"> - <ref name="filter-param-value"/> - </attribute> - </element> - </zeroOrMore> - </define> - <define name="rule-node-attributes"> <attribute name="action"> <ref name='action-type'/> @@ -937,18 +922,6 @@ </choice> </define> - <define name="filter-param-name"> - <data type="string"> - <param name="pattern">[a-zA-Z0-9_]+</param> - </data> - </define> - - <define name="filter-param-value"> - <data type="string"> - <param name="pattern">[a-zA-Z0-9_\.:]+</param> - </data> - </define> - <define name='action-type'> <choice> <value>drop</value> diff --git a/docs/schemas/nwfilter_params.rng b/docs/schemas/nwfilter_params.rng new file mode 100644 index 0000000000..a3e7b35b6f --- /dev/null +++ b/docs/schemas/nwfilter_params.rng @@ -0,0 +1,32 @@ +<?xml version="1.0"?> +<!-- network-related definitions used in multiple grammars --> +<grammar xmlns="http://relaxng.org/ns/structure/1.0" datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes"> + + <define name="filterref-node-attributes"> + <attribute name="filter"> + <data type="NCName"/> + </attribute> + <zeroOrMore> + <element name="parameter"> + <attribute name="name"> + <ref name="filter-param-name"/> + </attribute> + <attribute name="value"> + <ref name="filter-param-value"/> + </attribute> + </element> + </zeroOrMore> + </define> + + <define name="filter-param-name"> + <data type="string"> + <param name="pattern">[a-zA-Z0-9_]+</param> + </data> + </define> + <define name="filter-param-value"> + <data type="string"> + <param name="pattern">[a-zA-Z0-9_\.:]+</param> + </data> + </define> + +</grammar> diff --git a/docs/schemas/nwfilterbinding.rng b/docs/schemas/nwfilterbinding.rng new file mode 100644 index 0000000000..9c8e5a83cf --- /dev/null +++ b/docs/schemas/nwfilterbinding.rng @@ -0,0 +1,49 @@ +<?xml version="1.0"?> +<grammar xmlns="http://relaxng.org/ns/structure/1.0" datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes"> + <!-- domain-related definitions used in multiple grammars --> + <include href='basictypes.rng'/> + <include href='nwfilter_params.rng'/> + + <start> + <ref name="filterbinding"/> + </start> + + <define name="filterbinding"> + <element name="filterbinding"> + <interleave> + <element name="owner"> + <element name="name"> + <text/> + </element> + <element name="uuid"> + <ref name="UUID"/> + </element> + </element> + + <element name="portdev"> + <attribute name="name"/> + <empty/> + </element> + + <optional> + <element name="linkdev"> + <attribute name="name"/> + <empty/> + </element> + </optional> + + <element name="mac"> + <attribute name="address"> + <ref name="uniMacAddr"/> + </attribute> + <empty/> + </element> + + <element name="filterref"> + <ref name="filterref-node-attributes"/> + </element> + </interleave> + </element> + </define> + +</grammar> diff --git a/tests/virschematest.c b/tests/virschematest.c index 2d35833919..5bae022111 100644 --- a/tests/virschematest.c +++ b/tests/virschematest.c @@ -230,6 +230,7 @@ mymain(void) "networkxml2xmlout", "networkxml2confdata"); DO_TEST_DIR("nodedev.rng", "nodedevschemadata"); DO_TEST_DIR("nwfilter.rng", "nwfilterxml2xmlout"); + DO_TEST_DIR("nwfilterbinding.rng", "virnwfilterbindingxml2xmldata"); DO_TEST_DIR("secret.rng", "secretxml2xmlin"); DO_TEST_DIR("storagepool.rng", "storagepoolxml2xmlin", "storagepoolxml2xmlout", "storagepoolschemadata"); -- 2.17.0

When the daemons are split there will need to be a way for the virt drivers and/or network driver to create and delete bindings between network ports and network filters. This defines a set of public APIs that are suitable for managing this facility. Reviewed-by: John Ferlan <jferlan@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- include/libvirt/libvirt-nwfilter.h | 39 ++++ include/libvirt/virterror.h | 2 + src/datatypes.c | 67 +++++++ src/datatypes.h | 31 +++ src/driver-nwfilter.h | 30 +++ src/libvirt-nwfilter.c | 305 +++++++++++++++++++++++++++++ src/libvirt_private.syms | 1 + src/libvirt_public.syms | 9 + src/util/virerror.c | 12 ++ 9 files changed, 496 insertions(+) diff --git a/include/libvirt/libvirt-nwfilter.h b/include/libvirt/libvirt-nwfilter.h index 9f01c175a9..20e6d1ff9a 100644 --- a/include/libvirt/libvirt-nwfilter.h +++ b/include/libvirt/libvirt-nwfilter.h @@ -43,6 +43,23 @@ typedef struct _virNWFilter virNWFilter; */ typedef virNWFilter *virNWFilterPtr; +/** + * virNWFilterBinding: + * + * a virNWFilterBinding is a private structure representing a network + * filter binding to a port + */ +typedef struct _virNWFilterBinding virNWFilterBinding; + +/** + * virNWFilterBindingPtr: + * + * a virNWFilterBindingPtr is pointer to a virNWFilterBinding private + * structure, this is the type used to reference a network filter + * port binding in the API. + */ +typedef virNWFilterBinding *virNWFilterBindingPtr; + /* * List NWFilters @@ -92,4 +109,26 @@ int virNWFilterGetUUIDString (virNWFilterPtr nwfilter, char * virNWFilterGetXMLDesc (virNWFilterPtr nwfilter, unsigned int flags); + +virNWFilterBindingPtr virNWFilterBindingLookupByPortDev(virConnectPtr conn, + const char *portdev); + +const char * virNWFilterBindingGetPortDev(virNWFilterBindingPtr binding); +const char * virNWFilterBindingGetFilterName(virNWFilterBindingPtr binding); + +int virConnectListAllNWFilterBindings(virConnectPtr conn, + virNWFilterBindingPtr **bindings, + unsigned int flags); + +virNWFilterBindingPtr virNWFilterBindingCreateXML(virConnectPtr conn, + const char *xml, + unsigned int flags); + +char * virNWFilterBindingGetXMLDesc(virNWFilterBindingPtr binding, + unsigned int flags); + +int virNWFilterBindingDelete(virNWFilterBindingPtr binding); +int virNWFilterBindingRef(virNWFilterBindingPtr binding); +int virNWFilterBindingFree(virNWFilterBindingPtr binding); + #endif /* __VIR_LIBVIRT_NWFILTER_H__ */ diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index 5e58b6a3f9..57aadb8d16 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -321,6 +321,8 @@ typedef enum { to guest-sync command (DEPRECATED)*/ VIR_ERR_LIBSSH = 98, /* error in libssh transport driver */ VIR_ERR_DEVICE_MISSING = 99, /* fail to find the desired device */ + VIR_ERR_INVALID_NWFILTER_BINDING = 100, /* invalid nwfilter binding */ + VIR_ERR_NO_NWFILTER_BINDING = 101, /* no nwfilter binding */ } virErrorNumber; /** diff --git a/src/datatypes.c b/src/datatypes.c index 09b8eea5a2..878a1c5b5f 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -41,6 +41,7 @@ virClassPtr virInterfaceClass; virClassPtr virNetworkClass; virClassPtr virNodeDeviceClass; virClassPtr virNWFilterClass; +virClassPtr virNWFilterBindingClass; virClassPtr virSecretClass; virClassPtr virStreamClass; virClassPtr virStorageVolClass; @@ -54,6 +55,7 @@ static void virInterfaceDispose(void *obj); static void virNetworkDispose(void *obj); static void virNodeDeviceDispose(void *obj); static void virNWFilterDispose(void *obj); +static void virNWFilterBindingDispose(void *obj); static void virSecretDispose(void *obj); static void virStreamDispose(void *obj); static void virStorageVolDispose(void *obj); @@ -89,6 +91,7 @@ virDataTypesOnceInit(void) DECLARE_CLASS(virNetwork); DECLARE_CLASS(virNodeDevice); DECLARE_CLASS(virNWFilter); + DECLARE_CLASS(virNWFilterBinding); DECLARE_CLASS(virSecret); DECLARE_CLASS(virStream); DECLARE_CLASS(virStorageVol); @@ -830,6 +833,70 @@ virNWFilterDispose(void *obj) } +/** + * virGetNWFilterBinding: + * @conn: the hypervisor connection + * @portdev: pointer to the network filter port device name + * @filtername: name of the network filter + * + * Allocates a new network filter binding object. When the object is no longer + * needed, virObjectUnref() must be called in order to not leak data. + * + * Returns a pointer to the network filter binding object, or NULL on error. + */ +virNWFilterBindingPtr +virGetNWFilterBinding(virConnectPtr conn, const char *portdev, + const char *filtername) +{ + virNWFilterBindingPtr ret = NULL; + + if (virDataTypesInitialize() < 0) + return NULL; + + virCheckConnectGoto(conn, error); + virCheckNonNullArgGoto(portdev, error); + + if (!(ret = virObjectNew(virNWFilterBindingClass))) + goto error; + + if (VIR_STRDUP(ret->portdev, portdev) < 0) + goto error; + + if (VIR_STRDUP(ret->filtername, filtername) < 0) + goto error; + + ret->conn = virObjectRef(conn); + + return ret; + + error: + virObjectUnref(ret); + return NULL; +} + + +/** + * virNWFilterBindingDispose: + * @obj: the network filter binding to release + * + * Unconditionally release all memory associated with a nwfilter binding. + * The nwfilter binding object must not be used once this method returns. + * + * It will also unreference the associated connection object, + * which may also be released if its ref count hits zero. + */ +static void +virNWFilterBindingDispose(void *obj) +{ + virNWFilterBindingPtr binding = obj; + + VIR_DEBUG("release binding %p %s", binding, binding->portdev); + + VIR_FREE(binding->portdev); + virObjectUnref(binding->conn); +} + + /** * virGetDomainSnapshot: * @domain: the domain to snapshot diff --git a/src/datatypes.h b/src/datatypes.h index 192c86be80..e1b38706dc 100644 --- a/src/datatypes.h +++ b/src/datatypes.h @@ -36,6 +36,7 @@ extern virClassPtr virInterfaceClass; extern virClassPtr virNetworkClass; extern virClassPtr virNodeDeviceClass; extern virClassPtr virNWFilterClass; +extern virClassPtr virNWFilterBindingClass; extern virClassPtr virSecretClass; extern virClassPtr virStreamClass; extern virClassPtr virStorageVolClass; @@ -277,6 +278,20 @@ extern virClassPtr virAdmClientClass; } \ } while (0) +# define virCheckNWFilterBindingReturn(obj, retval) \ + do { \ + virNWFilterBindingPtr _nw = (obj); \ + if (!virObjectIsClass(_nw, virNWFilterBindingClass) || \ + !virObjectIsClass(_nw->conn, virConnectClass)) { \ + virReportErrorHelper(VIR_FROM_NWFILTER, \ + VIR_ERR_INVALID_NWFILTER_BINDING, \ + __FILE__, __FUNCTION__, __LINE__, \ + __FUNCTION__); \ + virDispatchError(NULL); \ + return retval; \ + } \ + } while (0) + # define virCheckDomainSnapshotReturn(obj, retval) \ do { \ virDomainSnapshotPtr _snap = (obj); \ @@ -676,6 +691,19 @@ struct _virNWFilter { }; +/** +* _virNWFilterBinding: +* +* Internal structure associated to a network filter port binding +*/ +struct _virNWFilterBinding { + virObject parent; + virConnectPtr conn; /* pointer back to the connection */ + char *portdev; /* the network filter port device name */ + char *filtername; /* the network filter name */ +}; + + /* * Helper APIs for allocating new object instances */ @@ -712,6 +740,9 @@ virStreamPtr virGetStream(virConnectPtr conn); virNWFilterPtr virGetNWFilter(virConnectPtr conn, const char *name, const unsigned char *uuid); +virNWFilterBindingPtr virGetNWFilterBinding(virConnectPtr conn, + const char *portdev, + const char *filtername); virDomainSnapshotPtr virGetDomainSnapshot(virDomainPtr domain, const char *name); diff --git a/src/driver-nwfilter.h b/src/driver-nwfilter.h index cb49542f92..2c3e480a32 100644 --- a/src/driver-nwfilter.h +++ b/src/driver-nwfilter.h @@ -57,6 +57,31 @@ typedef char * (*virDrvNWFilterGetXMLDesc)(virNWFilterPtr nwfilter, unsigned int flags); +typedef virNWFilterBindingPtr +(*virDrvNWFilterBindingLookupByPortDev)(virConnectPtr conn, + const char *portdev); + +typedef int +(*virDrvConnectListAllNWFilterBindings)(virConnectPtr conn, + virNWFilterBindingPtr **bindings, + unsigned int flags); + +typedef virNWFilterBindingPtr +(*virDrvNWFilterBindingCreateXML)(virConnectPtr conn, + const char *xml, + unsigned int flags); + +typedef char * +(*virDrvNWFilterBindingGetXMLDesc)(virNWFilterBindingPtr binding, + unsigned int flags); + +typedef int +(*virDrvNWFilterBindingDelete)(virNWFilterBindingPtr binding); +typedef int +(*virDrvNWFilterBindingRef)(virNWFilterBindingPtr binding); +typedef int +(*virDrvNWFilterBindingFree)(virNWFilterBindingPtr binding); + typedef struct _virNWFilterDriver virNWFilterDriver; typedef virNWFilterDriver *virNWFilterDriverPtr; @@ -77,6 +102,11 @@ struct _virNWFilterDriver { virDrvNWFilterDefineXML nwfilterDefineXML; virDrvNWFilterUndefine nwfilterUndefine; virDrvNWFilterGetXMLDesc nwfilterGetXMLDesc; + virDrvConnectListAllNWFilterBindings connectListAllNWFilterBindings; + virDrvNWFilterBindingLookupByPortDev nwfilterBindingLookupByPortDev; + virDrvNWFilterBindingCreateXML nwfilterBindingCreateXML; + virDrvNWFilterBindingDelete nwfilterBindingDelete; + virDrvNWFilterBindingGetXMLDesc nwfilterBindingGetXMLDesc; }; diff --git a/src/libvirt-nwfilter.c b/src/libvirt-nwfilter.c index 948c30deef..e572d46c18 100644 --- a/src/libvirt-nwfilter.c +++ b/src/libvirt-nwfilter.c @@ -513,3 +513,308 @@ virNWFilterRef(virNWFilterPtr nwfilter) virObjectRef(nwfilter); return 0; } + + +/** + * virConnectListAllNWFilterBindings: + * @conn: Pointer to the hypervisor connection. + * @bindings: Pointer to a variable to store the array containing the network + * filter objects or NULL if the list is not required (just returns + * number of network filters). + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Collect the list of network filters, and allocate an array to store those + * objects. + * + * Returns the number of network filters found or -1 and sets @filters to NULL + * in case of error. On success, the array stored into @filters is guaranteed to + * have an extra allocated element set to NULL but not included in the return count, + * to make iteration easier. The caller is responsible for calling + * virNWFilterFree() on each array element, then calling free() on @filters. + */ +int +virConnectListAllNWFilterBindings(virConnectPtr conn, + virNWFilterBindingPtr **bindings, + unsigned int flags) +{ + VIR_DEBUG("conn=%p, bindings=%p, flags=0x%x", conn, bindings, flags); + + virResetLastError(); + + if (bindings) + *bindings = NULL; + + virCheckConnectReturn(conn, -1); + + if (conn->nwfilterDriver && + conn->nwfilterDriver->connectListAllNWFilterBindings) { + int ret; + ret = conn->nwfilterDriver->connectListAllNWFilterBindings(conn, bindings, flags); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + + error: + virDispatchError(conn); + return -1; +} + + +/** + * virNWFilterBindingLookupByPortDev: + * @conn: pointer to the hypervisor connection + * @portdev: name for the network port device + * + * Try to lookup a network filter binding on the given hypervisor based + * on network port device name. + * + * virNWFilterBindingFree should be used to free the resources after the + * binding object is no longer needed. + * + * Returns a new binding object or NULL in case of failure. If the + * network filter cannot be found, then VIR_ERR_NO_NWFILTER_BINDING + * error is raised. + */ +virNWFilterBindingPtr +virNWFilterBindingLookupByPortDev(virConnectPtr conn, const char *portdev) +{ + VIR_DEBUG("conn=%p, name=%s", conn, NULLSTR(portdev)); + + virResetLastError(); + + virCheckConnectReturn(conn, NULL); + virCheckNonNullArgGoto(portdev, error); + + if (conn->nwfilterDriver && conn->nwfilterDriver->nwfilterBindingLookupByPortDev) { + virNWFilterBindingPtr ret; + ret = conn->nwfilterDriver->nwfilterBindingLookupByPortDev(conn, portdev); + if (!ret) + goto error; + return ret; + } + + virReportUnsupportedError(); + + error: + virDispatchError(conn); + return NULL; +} + + +/** + * virNWFilterBindingFree: + * @binding: a binding object + * + * Free the binding object. The running instance is kept alive. + * The data structure is freed and should not be used thereafter. + * + * Returns 0 in case of success and -1 in case of failure. + */ +int +virNWFilterBindingFree(virNWFilterBindingPtr binding) +{ + VIR_DEBUG("binding=%p", binding); + + virResetLastError(); + + virCheckNWFilterBindingReturn(binding, -1); + + virObjectUnref(binding); + return 0; +} + + +/** + * virNWFilterBindingGetPortDev: + * @binding: a binding object + * + * Get the port dev name for the network filter binding + * + * Returns a pointer to the name or NULL, the string need not be deallocated + * its lifetime will be the same as the binding object. + */ +const char * +virNWFilterBindingGetPortDev(virNWFilterBindingPtr binding) +{ + VIR_DEBUG("binding=%p", binding); + + virResetLastError(); + + virCheckNWFilterBindingReturn(binding, NULL); + + return binding->portdev; +} + + +/** + * virNWFilterBindingGetFilterName: + * @binding: a binding object + * + * Get the filter name for the network filter binding + * + * Returns a pointer to the name or NULL, the string need not be deallocated + * its lifetime will be the same as the binding object. + */ +const char * +virNWFilterBindingGetFilterName(virNWFilterBindingPtr binding) +{ + VIR_DEBUG("binding=%p", binding); + + virResetLastError(); + + virCheckNWFilterBindingReturn(binding, NULL); + + return binding->filtername; +} + + +/** + * virNWFilterBindingCreateXML: + * @conn: pointer to the hypervisor connection + * @xml: an XML description of the binding + * @flags: currently unused, pass 0 + * + * Define a new network filter, based on an XML description + * similar to the one returned by virNWFilterGetXMLDesc() + * + * virNWFilterFree should be used to free the resources after the + * binding object is no longer needed. + * + * Returns a new binding object or NULL in case of failure + */ +virNWFilterBindingPtr +virNWFilterBindingCreateXML(virConnectPtr conn, const char *xml, unsigned int flags) +{ + VIR_DEBUG("conn=%p, xml=%s", conn, NULLSTR(xml)); + + virResetLastError(); + + virCheckConnectReturn(conn, NULL); + virCheckNonNullArgGoto(xml, error); + virCheckReadOnlyGoto(conn->flags, error); + + if (conn->nwfilterDriver && conn->nwfilterDriver->nwfilterBindingCreateXML) { + virNWFilterBindingPtr ret; + ret = conn->nwfilterDriver->nwfilterBindingCreateXML(conn, xml, flags); + if (!ret) + goto error; + return ret; + } + + virReportUnsupportedError(); + + error: + virDispatchError(conn); + return NULL; +} + + +/** + * virNWFilterBindingDelete: + * @binding: a binding object + * + * Delete the binding object. This does not free the + * associated virNWFilterBindingPtr object. + * + * Returns 0 in case of success and -1 in case of failure. + */ +int +virNWFilterBindingDelete(virNWFilterBindingPtr binding) +{ + virConnectPtr conn; + VIR_DEBUG("binding=%p", binding); + + virResetLastError(); + + virCheckNWFilterBindingReturn(binding, -1); + conn = binding->conn; + + virCheckReadOnlyGoto(conn->flags, error); + + if (conn->nwfilterDriver && conn->nwfilterDriver->nwfilterBindingDelete) { + int ret; + ret = conn->nwfilterDriver->nwfilterBindingDelete(binding); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + + error: + virDispatchError(binding->conn); + return -1; +} + + +/** + * virNWFilterBindingGetXMLDesc: + * @binding: a binding object + * @flags: extra flags; not used yet, so callers should always pass 0 + * + * Provide an XML description of the network filter. The description may be + * reused later to redefine the network filter with virNWFilterCreateXML(). + * + * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error. + * the caller must free() the returned value. + */ +char * +virNWFilterBindingGetXMLDesc(virNWFilterBindingPtr binding, unsigned int flags) +{ + virConnectPtr conn; + VIR_DEBUG("binding=%p, flags=0x%x", binding, flags); + + virResetLastError(); + + virCheckNWFilterBindingReturn(binding, NULL); + conn = binding->conn; + + if (conn->nwfilterDriver && conn->nwfilterDriver->nwfilterBindingGetXMLDesc) { + char *ret; + ret = conn->nwfilterDriver->nwfilterBindingGetXMLDesc(binding, flags); + if (!ret) + goto error; + return ret; + } + + virReportUnsupportedError(); + + error: + virDispatchError(binding->conn); + return NULL; +} + + +/** + * virNWFilterBindingRef: + * @binding: the binding to hold a reference on + * + * Increment the reference count on the binding. For each + * additional call to this method, there shall be a corresponding + * call to virNWFilterFree to release the reference count, once + * the caller no longer needs the reference to this object. + * + * This method is typically useful for applications where multiple + * threads are using a connection, and it is required that the + * connection remain open until all threads have finished using + * it. ie, each new thread using an binding would increment + * the reference count. + * + * Returns 0 in case of success, -1 in case of failure. + */ +int +virNWFilterBindingRef(virNWFilterBindingPtr binding) +{ + VIR_DEBUG("binding=%p refs=%d", binding, + binding ? binding->parent.u.s.refs : 0); + + virResetLastError(); + + virCheckNWFilterBindingReturn(binding, -1); + + virObjectRef(binding); + return 0; +} diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7f364ec9a1..a323316607 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1200,6 +1200,7 @@ virGetInterface; virGetNetwork; virGetNodeDevice; virGetNWFilter; +virGetNWFilterBinding; virGetSecret; virGetStoragePool; virGetStorageVol; diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 3bf3c3f916..d4cdbd8b32 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -798,6 +798,15 @@ LIBVIRT_4.5.0 { virGetLastErrorDomain; virNodeGetSEVInfo; virDomainGetLaunchSecurityInfo; + virNWFilterBindingLookupByPortDev; + virConnectListAllNWFilterBindings; + virNWFilterBindingCreateXML; + virNWFilterBindingGetXMLDesc; + virNWFilterBindingDelete; + virNWFilterBindingRef; + virNWFilterBindingFree; + virNWFilterBindingGetPortDev; + virNWFilterBindingGetFilterName; } LIBVIRT_4.4.0; # .... define new API here using predicted next version number .... diff --git a/src/util/virerror.c b/src/util/virerror.c index 93632dbdf7..f198f27957 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -1494,6 +1494,18 @@ virErrorMsg(virErrorNumber error, const char *info) else errmsg = _("device not found: %s"); break; + case VIR_ERR_INVALID_NWFILTER_BINDING: + if (info == NULL) + errmsg = _("Invalid network filter binding"); + else + errmsg = _("Invalid network filter binding: %s"); + break; + case VIR_ERR_NO_NWFILTER_BINDING: + if (info == NULL) + errmsg = _("Network filter binding not found"); + else + errmsg = _("Network filter binding not found: %s"); + break; } return errmsg; } -- 2.17.0

Reviewed-by: John Ferlan <jferlan@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/access/viraccessdriver.h | 5 ++++ src/access/viraccessdrivernop.c | 10 ++++++++ src/access/viraccessdriverpolkit.c | 21 +++++++++++++++++ src/access/viraccessdriverstack.c | 24 +++++++++++++++++++ src/access/viraccessmanager.c | 15 ++++++++++++ src/access/viraccessmanager.h | 5 ++++ src/access/viraccessperm.c | 7 +++++- src/access/viraccessperm.h | 38 ++++++++++++++++++++++++++++++ src/rpc/gendispatch.pl | 3 ++- 9 files changed, 126 insertions(+), 2 deletions(-) diff --git a/src/access/viraccessdriver.h b/src/access/viraccessdriver.h index e3050b6439..3b25f36cab 100644 --- a/src/access/viraccessdriver.h +++ b/src/access/viraccessdriver.h @@ -47,6 +47,10 @@ typedef int (*virAccessDriverCheckNWFilterDrv)(virAccessManagerPtr manager, const char *driverName, virNWFilterDefPtr nwfilter, virAccessPermNWFilter av); +typedef int (*virAccessDriverCheckNWFilterBindingDrv)(virAccessManagerPtr manager, + const char *driverName, + virNWFilterBindingDefPtr binding, + virAccessPermNWFilterBinding av); typedef int (*virAccessDriverCheckSecretDrv)(virAccessManagerPtr manager, const char *driverName, virSecretDefPtr secret, @@ -80,6 +84,7 @@ struct _virAccessDriver { virAccessDriverCheckNetworkDrv checkNetwork; virAccessDriverCheckNodeDeviceDrv checkNodeDevice; virAccessDriverCheckNWFilterDrv checkNWFilter; + virAccessDriverCheckNWFilterBindingDrv checkNWFilterBinding; virAccessDriverCheckSecretDrv checkSecret; virAccessDriverCheckStoragePoolDrv checkStoragePool; virAccessDriverCheckStorageVolDrv checkStorageVol; diff --git a/src/access/viraccessdrivernop.c b/src/access/viraccessdrivernop.c index 86ceef37c2..98ef9206c5 100644 --- a/src/access/viraccessdrivernop.c +++ b/src/access/viraccessdrivernop.c @@ -75,6 +75,15 @@ virAccessDriverNopCheckNWFilter(virAccessManagerPtr manager ATTRIBUTE_UNUSED, return 1; /* Allow */ } +static int +virAccessDriverNopCheckNWFilterBinding(virAccessManagerPtr manager ATTRIBUTE_UNUSED, + const char *driverName ATTRIBUTE_UNUSED, + virNWFilterBindingDefPtr binding ATTRIBUTE_UNUSED, + virAccessPermNWFilterBinding perm ATTRIBUTE_UNUSED) +{ + return 1; /* Allow */ +} + static int virAccessDriverNopCheckSecret(virAccessManagerPtr manager ATTRIBUTE_UNUSED, const char *driverName ATTRIBUTE_UNUSED, @@ -112,6 +121,7 @@ virAccessDriver accessDriverNop = { .checkNetwork = virAccessDriverNopCheckNetwork, .checkNodeDevice = virAccessDriverNopCheckNodeDevice, .checkNWFilter = virAccessDriverNopCheckNWFilter, + .checkNWFilterBinding = virAccessDriverNopCheckNWFilterBinding, .checkSecret = virAccessDriverNopCheckSecret, .checkStoragePool = virAccessDriverNopCheckStoragePool, .checkStorageVol = virAccessDriverNopCheckStorageVol, diff --git a/src/access/viraccessdriverpolkit.c b/src/access/viraccessdriverpolkit.c index 48a83f66d7..6954d74a15 100644 --- a/src/access/viraccessdriverpolkit.c +++ b/src/access/viraccessdriverpolkit.c @@ -276,6 +276,26 @@ virAccessDriverPolkitCheckNWFilter(virAccessManagerPtr manager, attrs); } +static int +virAccessDriverPolkitCheckNWFilterBinding(virAccessManagerPtr manager, + const char *driverName, + virNWFilterBindingDefPtr binding, + virAccessPermNWFilterBinding perm) +{ + const char *attrs[] = { + "connect_driver", driverName, + "nwfilter_binding_portdev", binding->portdevname, + "nwfilter_binding_linkdev", binding->linkdevname, + "nwfilter_binding_filter", binding->filter, + NULL, + }; + + return virAccessDriverPolkitCheck(manager, + "nwfilter_binding", + virAccessPermNWFilterBindingTypeToString(perm), + attrs); +} + static int virAccessDriverPolkitCheckSecret(virAccessManagerPtr manager, const char *driverName, @@ -409,6 +429,7 @@ virAccessDriver accessDriverPolkit = { .checkNetwork = virAccessDriverPolkitCheckNetwork, .checkNodeDevice = virAccessDriverPolkitCheckNodeDevice, .checkNWFilter = virAccessDriverPolkitCheckNWFilter, + .checkNWFilterBinding = virAccessDriverPolkitCheckNWFilterBinding, .checkSecret = virAccessDriverPolkitCheckSecret, .checkStoragePool = virAccessDriverPolkitCheckStoragePool, .checkStorageVol = virAccessDriverPolkitCheckStorageVol, diff --git a/src/access/viraccessdriverstack.c b/src/access/viraccessdriverstack.c index b43a743027..0ffc6abaf3 100644 --- a/src/access/viraccessdriverstack.c +++ b/src/access/viraccessdriverstack.c @@ -197,6 +197,29 @@ virAccessDriverStackCheckNWFilter(virAccessManagerPtr manager, return ret; } +static int +virAccessDriverStackCheckNWFilterBinding(virAccessManagerPtr manager, + const char *driverName, + virNWFilterBindingDefPtr binding, + virAccessPermNWFilterBinding perm) +{ + virAccessDriverStackPrivatePtr priv = virAccessManagerGetPrivateData(manager); + int ret = 1; + size_t i; + + for (i = 0; i < priv->managersLen; i++) { + int rv; + /* We do not short-circuit on first denial - always check all drivers */ + rv = virAccessManagerCheckNWFilterBinding(priv->managers[i], driverName, binding, perm); + if (rv == 0 && ret != -1) + ret = 0; + else if (rv < 0) + ret = -1; + } + + return ret; +} + static int virAccessDriverStackCheckSecret(virAccessManagerPtr manager, const char *driverName, @@ -277,6 +300,7 @@ virAccessDriver accessDriverStack = { .checkNetwork = virAccessDriverStackCheckNetwork, .checkNodeDevice = virAccessDriverStackCheckNodeDevice, .checkNWFilter = virAccessDriverStackCheckNWFilter, + .checkNWFilterBinding = virAccessDriverStackCheckNWFilterBinding, .checkSecret = virAccessDriverStackCheckSecret, .checkStoragePool = virAccessDriverStackCheckStoragePool, .checkStorageVol = virAccessDriverStackCheckStorageVol, diff --git a/src/access/viraccessmanager.c b/src/access/viraccessmanager.c index b048a367e3..e7b5bf38da 100644 --- a/src/access/viraccessmanager.c +++ b/src/access/viraccessmanager.c @@ -296,6 +296,21 @@ int virAccessManagerCheckNWFilter(virAccessManagerPtr manager, return virAccessManagerSanitizeError(ret); } +int virAccessManagerCheckNWFilterBinding(virAccessManagerPtr manager, + const char *driverName, + virNWFilterBindingDefPtr binding, + virAccessPermNWFilterBinding perm) +{ + int ret = 0; + VIR_DEBUG("manager=%p(name=%s) driver=%s binding=%p perm=%d", + manager, manager->drv->name, driverName, binding, perm); + + if (manager->drv->checkNWFilterBinding) + ret = manager->drv->checkNWFilterBinding(manager, driverName, binding, perm); + + return virAccessManagerSanitizeError(ret); +} + int virAccessManagerCheckSecret(virAccessManagerPtr manager, const char *driverName, virSecretDefPtr secret, diff --git a/src/access/viraccessmanager.h b/src/access/viraccessmanager.h index e7eb15d30c..4fc86a1ff2 100644 --- a/src/access/viraccessmanager.h +++ b/src/access/viraccessmanager.h @@ -29,6 +29,7 @@ # include "conf/storage_conf.h" # include "conf/secret_conf.h" # include "conf/interface_conf.h" +# include "conf/virnwfilterbindingdef.h" # include "access/viraccessperm.h" typedef struct _virAccessManager virAccessManager; @@ -73,6 +74,10 @@ int virAccessManagerCheckNWFilter(virAccessManagerPtr manager, const char *driverName, virNWFilterDefPtr nwfilter, virAccessPermNWFilter perm); +int virAccessManagerCheckNWFilterBinding(virAccessManagerPtr manager, + const char *driverName, + virNWFilterBindingDefPtr binding, + virAccessPermNWFilterBinding perm); int virAccessManagerCheckSecret(virAccessManagerPtr manager, const char *driverName, virSecretDefPtr secret, diff --git a/src/access/viraccessperm.c b/src/access/viraccessperm.c index 0f58290173..d7cbb70b7b 100644 --- a/src/access/viraccessperm.c +++ b/src/access/viraccessperm.c @@ -29,7 +29,7 @@ VIR_ENUM_IMPL(virAccessPermConnect, "search_domains", "search_networks", "search_storage_pools", "search_node_devices", "search_interfaces", "search_secrets", - "search_nwfilters", + "search_nwfilters", "search_nwfilter_bindings", "detect_storage_pools", "pm_control", "interface_transaction"); @@ -66,6 +66,11 @@ VIR_ENUM_IMPL(virAccessPermNWFilter, "getattr", "read", "write", "save", "delete"); +VIR_ENUM_IMPL(virAccessPermNWFilterBinding, + VIR_ACCESS_PERM_NWFILTER_BINDING_LAST, + "getattr", "read", + "create", "delete"); + VIR_ENUM_IMPL(virAccessPermSecret, VIR_ACCESS_PERM_SECRET_LAST, "getattr", "read", "write", diff --git a/src/access/viraccessperm.h b/src/access/viraccessperm.h index 1817da73bc..5ac5ff3377 100644 --- a/src/access/viraccessperm.h +++ b/src/access/viraccessperm.h @@ -94,6 +94,12 @@ typedef enum { */ VIR_ACCESS_PERM_CONNECT_SEARCH_NWFILTERS, + /** + * @desc: List network filter bindings + * @message: Listing network filter bindings requires authorization + * @anonymous: 1 + */ + VIR_ACCESS_PERM_CONNECT_SEARCH_NWFILTER_BINDINGS, /** * @desc: Detect storage pools @@ -486,6 +492,37 @@ typedef enum { VIR_ACCESS_PERM_NWFILTER_LAST } virAccessPermNWFilter; +typedef enum { + + /** + * @desc: Access network filter + * @message: Accessing network filter requires authorization + * @anonymous: 1 + */ + VIR_ACCESS_PERM_NWFILTER_BINDING_GETATTR, + + /** + * @desc: Read network filter binding + * @message: Reading network filter configuration requires authorization + * @anonymous: 1 + */ + VIR_ACCESS_PERM_NWFILTER_BINDING_READ, + + /** + * @desc: Create network filter binding + * @message: Creating network filter binding requires authorization + */ + VIR_ACCESS_PERM_NWFILTER_BINDING_CREATE, + + /** + * @desc: Delete network filter binding + * @message: Deleting network filter binding requires authorization + */ + VIR_ACCESS_PERM_NWFILTER_BINDING_DELETE, + + VIR_ACCESS_PERM_NWFILTER_BINDING_LAST +} virAccessPermNWFilterBinding; + typedef enum { /** @@ -657,6 +694,7 @@ VIR_ENUM_DECL(virAccessPermInterface); VIR_ENUM_DECL(virAccessPermNetwork); VIR_ENUM_DECL(virAccessPermNodeDevice); VIR_ENUM_DECL(virAccessPermNWFilter); +VIR_ENUM_DECL(virAccessPermNWFilterBinding); VIR_ENUM_DECL(virAccessPermSecret); VIR_ENUM_DECL(virAccessPermStoragePool); VIR_ENUM_DECL(virAccessPermStorageVol); diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index b8b83b6b40..480ebe7b00 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -2033,7 +2033,8 @@ elsif ($mode eq "client") { "storage_conf.h", "nwfilter_conf.h", "node_device_conf.h", - "interface_conf.h" + "interface_conf.h", + "virnwfilterbindingdef.h", ); foreach my $hdr (@headers) { print "#include \"$hdr\"\n"; -- 2.17.0

Reviewed-by: John Ferlan <jferlan@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/remote/remote_daemon_dispatch.c | 15 +++++ src/remote/remote_driver.c | 20 +++++++ src/remote/remote_protocol.x | 90 ++++++++++++++++++++++++++++- src/remote_protocol-structs | 43 ++++++++++++++ src/rpc/gendispatch.pl | 12 ++-- 5 files changed, 173 insertions(+), 7 deletions(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index f1a5ba2590..4a93f09a7d 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -90,6 +90,7 @@ static virStoragePoolPtr get_nonnull_storage_pool(virConnectPtr conn, remote_non static virStorageVolPtr get_nonnull_storage_vol(virConnectPtr conn, remote_nonnull_storage_vol vol); static virSecretPtr get_nonnull_secret(virConnectPtr conn, remote_nonnull_secret secret); static virNWFilterPtr get_nonnull_nwfilter(virConnectPtr conn, remote_nonnull_nwfilter nwfilter); +static virNWFilterBindingPtr get_nonnull_nwfilter_binding(virConnectPtr conn, remote_nonnull_nwfilter_binding binding); static virDomainSnapshotPtr get_nonnull_domain_snapshot(virDomainPtr dom, remote_nonnull_domain_snapshot snapshot); static virNodeDevicePtr get_nonnull_node_device(virConnectPtr conn, remote_nonnull_node_device dev); static void make_nonnull_domain(remote_nonnull_domain *dom_dst, virDomainPtr dom_src); @@ -100,6 +101,7 @@ static void make_nonnull_storage_vol(remote_nonnull_storage_vol *vol_dst, virSto static void make_nonnull_node_device(remote_nonnull_node_device *dev_dst, virNodeDevicePtr dev_src); static void make_nonnull_secret(remote_nonnull_secret *secret_dst, virSecretPtr secret_src); static void make_nonnull_nwfilter(remote_nonnull_nwfilter *net_dst, virNWFilterPtr nwfilter_src); +static void make_nonnull_nwfilter_binding(remote_nonnull_nwfilter_binding *binding_dst, virNWFilterBindingPtr binding_src); static void make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapshot_dst, virDomainSnapshotPtr snapshot_src); static int @@ -7087,6 +7089,12 @@ get_nonnull_nwfilter(virConnectPtr conn, remote_nonnull_nwfilter nwfilter) return virGetNWFilter(conn, nwfilter.name, BAD_CAST nwfilter.uuid); } +static virNWFilterBindingPtr +get_nonnull_nwfilter_binding(virConnectPtr conn, remote_nonnull_nwfilter_binding binding) +{ + return virGetNWFilterBinding(conn, binding.portdev, binding.filtername); +} + static virDomainSnapshotPtr get_nonnull_domain_snapshot(virDomainPtr dom, remote_nonnull_domain_snapshot snapshot) { @@ -7159,6 +7167,13 @@ make_nonnull_nwfilter(remote_nonnull_nwfilter *nwfilter_dst, virNWFilterPtr nwfi memcpy(nwfilter_dst->uuid, nwfilter_src->uuid, VIR_UUID_BUFLEN); } +static void +make_nonnull_nwfilter_binding(remote_nonnull_nwfilter_binding *binding_dst, virNWFilterBindingPtr binding_src) +{ + ignore_value(VIR_STRDUP_QUIET(binding_dst->portdev, binding_src->portdev)); + ignore_value(VIR_STRDUP_QUIET(binding_dst->filtername, binding_src->filtername)); +} + static void make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapshot_dst, virDomainSnapshotPtr snapshot_src) { diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 1328f910b0..1d94c2e42d 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -141,6 +141,7 @@ static int remoteAuthPolkit(virConnectPtr conn, struct private_data *priv, static virDomainPtr get_nonnull_domain(virConnectPtr conn, remote_nonnull_domain domain); static virNetworkPtr get_nonnull_network(virConnectPtr conn, remote_nonnull_network network); static virNWFilterPtr get_nonnull_nwfilter(virConnectPtr conn, remote_nonnull_nwfilter nwfilter); +static virNWFilterBindingPtr get_nonnull_nwfilter_binding(virConnectPtr conn, remote_nonnull_nwfilter_binding binding); static virInterfacePtr get_nonnull_interface(virConnectPtr conn, remote_nonnull_interface iface); static virStoragePoolPtr get_nonnull_storage_pool(virConnectPtr conn, remote_nonnull_storage_pool pool); static virStorageVolPtr get_nonnull_storage_vol(virConnectPtr conn, remote_nonnull_storage_vol vol); @@ -156,6 +157,7 @@ static void make_nonnull_node_device(remote_nonnull_node_device *dev_dst, virNodeDevicePtr dev_src); static void make_nonnull_secret(remote_nonnull_secret *secret_dst, virSecretPtr secret_src); static void make_nonnull_nwfilter(remote_nonnull_nwfilter *nwfilter_dst, virNWFilterPtr nwfilter_src); +static void make_nonnull_nwfilter_binding(remote_nonnull_nwfilter_binding *binding_dst, virNWFilterBindingPtr binding_src); static void make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapshot_dst, virDomainSnapshotPtr snapshot_src); /*----------------------------------------------------------------------*/ @@ -8206,6 +8208,12 @@ get_nonnull_nwfilter(virConnectPtr conn, remote_nonnull_nwfilter nwfilter) return virGetNWFilter(conn, nwfilter.name, BAD_CAST nwfilter.uuid); } +static virNWFilterBindingPtr +get_nonnull_nwfilter_binding(virConnectPtr conn, remote_nonnull_nwfilter_binding binding) +{ + return virGetNWFilterBinding(conn, binding.portdev, binding.filtername); +} + static virDomainSnapshotPtr get_nonnull_domain_snapshot(virDomainPtr domain, remote_nonnull_domain_snapshot snapshot) { @@ -8273,6 +8281,13 @@ make_nonnull_nwfilter(remote_nonnull_nwfilter *nwfilter_dst, virNWFilterPtr nwfi memcpy(nwfilter_dst->uuid, nwfilter_src->uuid, VIR_UUID_BUFLEN); } +static void +make_nonnull_nwfilter_binding(remote_nonnull_nwfilter_binding *binding_dst, virNWFilterBindingPtr binding_src) +{ + binding_dst->portdev = binding_src->portdev; + binding_dst->filtername = binding_src->filtername; +} + static void make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapshot_dst, virDomainSnapshotPtr snapshot_src) { @@ -8656,6 +8671,11 @@ static virNWFilterDriver nwfilter_driver = { .connectNumOfNWFilters = remoteConnectNumOfNWFilters, /* 0.8.0 */ .connectListNWFilters = remoteConnectListNWFilters, /* 0.8.0 */ .connectListAllNWFilters = remoteConnectListAllNWFilters, /* 0.10.2 */ + .connectListAllNWFilterBindings = remoteConnectListAllNWFilterBindings, /* 4.5.0 */ + .nwfilterBindingLookupByPortDev = remoteNWFilterBindingLookupByPortDev, /* 4.5.0 */ + .nwfilterBindingCreateXML = remoteNWFilterBindingCreateXML, /* 4.5.0 */ + .nwfilterBindingDelete = remoteNWFilterBindingDelete, /* 4.5.0 */ + .nwfilterBindingGetXMLDesc = remoteNWFilterBindingGetXMLDesc, /* 4.5.0 */ }; static virConnectDriver connect_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 162cf5e61b..28c8febabd 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -94,6 +94,9 @@ const REMOTE_NODE_DEVICE_CAPS_LIST_MAX = 65536; /* Upper limit on lists of network filters. */ const REMOTE_NWFILTER_LIST_MAX = 16384; +/* Upper limit on lists of network filter bindings. */ +const REMOTE_NWFILTER_BINDING_LIST_MAX = 16384; + /* Upper limit on list of scheduler parameters. */ const REMOTE_DOMAIN_SCHEDULER_PARAMETERS_MAX = 16; @@ -281,6 +284,12 @@ struct remote_nonnull_nwfilter { remote_uuid uuid; }; +/* A network filter binding which may not be NULL. */ +struct remote_nonnull_nwfilter_binding { + remote_nonnull_string portdev; + remote_nonnull_string filtername; +}; + /* An interface which may not be NULL. */ struct remote_nonnull_interface { remote_nonnull_string name; @@ -322,6 +331,7 @@ struct remote_nonnull_domain_snapshot { typedef remote_nonnull_domain *remote_domain; typedef remote_nonnull_network *remote_network; typedef remote_nonnull_nwfilter *remote_nwfilter; +typedef remote_nonnull_nwfilter_binding *remote_nwfilter_binding; typedef remote_nonnull_storage_pool *remote_storage_pool; typedef remote_nonnull_storage_vol *remote_storage_vol; typedef remote_nonnull_node_device *remote_node_device; @@ -3505,6 +3515,48 @@ struct remote_domain_get_launch_security_info_ret { remote_typed_param params<REMOTE_DOMAIN_LAUNCH_SECURITY_INFO_PARAMS_MAX>; }; +/* nwfilter binding */ + +struct remote_nwfilter_binding_lookup_by_port_dev_args { + remote_nonnull_string name; +}; + +struct remote_nwfilter_binding_lookup_by_port_dev_ret { + remote_nonnull_nwfilter_binding nwfilter; +}; + +struct remote_nwfilter_binding_create_xml_args { + remote_nonnull_string xml; + unsigned int flags; +}; + +struct remote_nwfilter_binding_create_xml_ret { + remote_nonnull_nwfilter_binding nwfilter; +}; + +struct remote_nwfilter_binding_delete_args { + remote_nonnull_nwfilter_binding nwfilter; +}; + +struct remote_nwfilter_binding_get_xml_desc_args { + remote_nonnull_nwfilter_binding nwfilter; + unsigned int flags; +}; + +struct remote_nwfilter_binding_get_xml_desc_ret { + remote_nonnull_string xml; +}; + +struct remote_connect_list_all_nwfilter_bindings_args { + int need_results; + unsigned int flags; +}; + +struct remote_connect_list_all_nwfilter_bindings_ret { /* insert@1 */ + remote_nonnull_nwfilter_binding bindings<REMOTE_NWFILTER_BINDING_LIST_MAX>; + unsigned int ret; +}; + /*----- Protocol. -----*/ /* Define the program number, protocol version and procedure numbers here. */ @@ -6224,5 +6276,41 @@ enum remote_procedure { * @generate: none * @acl: domain:read */ - REMOTE_PROC_DOMAIN_GET_LAUNCH_SECURITY_INFO = 396 + REMOTE_PROC_DOMAIN_GET_LAUNCH_SECURITY_INFO = 396, + + /** + * @generate: both + * @priority: high + * @acl: nwfilter_binding:getattr + */ + REMOTE_PROC_NWFILTER_BINDING_LOOKUP_BY_PORT_DEV = 397, + + /** + * @generate: both + * @priority: high + * @acl: nwfilter_binding:read + */ + REMOTE_PROC_NWFILTER_BINDING_GET_XML_DESC = 398, + + /** + * @generate: both + * @priority: high + * @acl: nwfilter_binding:create + */ + REMOTE_PROC_NWFILTER_BINDING_CREATE_XML = 399, + + /** + * @generate: both + * @priority: high + * @acl: nwfilter_binding:delete + */ + REMOTE_PROC_NWFILTER_BINDING_DELETE = 400, + + /** + * @generate: both + * @priority: high + * @acl: connect:search_nwfilter_bindings + * @aclfilter: nwfilter_binding:getattr + */ + REMOTE_PROC_CONNECT_LIST_ALL_NWFILTER_BINDINGS = 401 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 0c75ad2305..6343e14638 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -21,6 +21,10 @@ struct remote_nonnull_nwfilter { remote_nonnull_string name; remote_uuid uuid; }; +struct remote_nonnull_nwfilter_binding { + remote_nonnull_string portdev; + remote_nonnull_string filtername; +}; struct remote_nonnull_interface { remote_nonnull_string name; remote_nonnull_string mac; @@ -2928,6 +2932,40 @@ struct remote_domain_get_launch_security_info_ret { remote_typed_param * params_val; } params; }; +struct remote_nwfilter_binding_lookup_by_port_dev_args { + remote_nonnull_string name; +}; +struct remote_nwfilter_binding_lookup_by_port_dev_ret { + remote_nonnull_nwfilter_binding nwfilter; +}; +struct remote_nwfilter_binding_create_xml_args { + remote_nonnull_string xml; + u_int flags; +}; +struct remote_nwfilter_binding_create_xml_ret { + remote_nonnull_nwfilter_binding nwfilter; +}; +struct remote_nwfilter_binding_delete_args { + remote_nonnull_nwfilter_binding nwfilter; +}; +struct remote_nwfilter_binding_get_xml_desc_args { + remote_nonnull_nwfilter_binding nwfilter; + u_int flags; +}; +struct remote_nwfilter_binding_get_xml_desc_ret { + remote_nonnull_string xml; +}; +struct remote_connect_list_all_nwfilter_bindings_args { + int need_results; + u_int flags; +}; +struct remote_connect_list_all_nwfilter_bindings_ret { + struct { + u_int bindings_len; + remote_nonnull_nwfilter_binding * bindings_val; + } bindings; + u_int ret; +}; enum remote_procedure { REMOTE_PROC_CONNECT_OPEN = 1, REMOTE_PROC_CONNECT_CLOSE = 2, @@ -3325,4 +3363,9 @@ enum remote_procedure { REMOTE_PROC_CONNECT_BASELINE_HYPERVISOR_CPU = 394, REMOTE_PROC_NODE_GET_SEV_INFO = 395, REMOTE_PROC_DOMAIN_GET_LAUNCH_SECURITY_INFO = 396, + REMOTE_PROC_NWFILTER_BINDING_LOOKUP_BY_PORT_DEV = 397, + REMOTE_PROC_NWFILTER_BINDING_GET_XML_DESC = 398, + REMOTE_PROC_NWFILTER_BINDING_CREATE_XML = 399, + REMOTE_PROC_NWFILTER_BINDING_DELETE = 400, + REMOTE_PROC_CONNECT_LIST_ALL_NWFILTER_BINDINGS = 401, }; diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index 480ebe7b00..0c4648c0fb 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -557,7 +557,7 @@ elsif ($mode eq "server") { if ($args_member =~ m/^remote_nonnull_string name;/ and $has_node_device) { # ignore the name arg for node devices next - } elsif ($args_member =~ m/^remote_nonnull_(domain|network|storage_pool|storage_vol|interface|secret|nwfilter) (\S+);/) { + } elsif ($args_member =~ m/^remote_nonnull_(domain|network|storage_pool|storage_vol|interface|secret|nwfilter|nwfilter_binding) (\S+);/) { my $type_name = name_to_TypeName($1); push(@vars_list, "vir${type_name}Ptr $2 = NULL"); @@ -722,7 +722,7 @@ elsif ($mode eq "server") { if (!$modern_ret_as_list) { push(@ret_list, "ret->$3 = tmp.$3;"); } - } elsif ($ret_member =~ m/(?:admin|remote)_nonnull_(secret|nwfilter|node_device|interface|network|storage_vol|storage_pool|domain_snapshot|domain|server|client) (\S+)<(\S+)>;/) { + } elsif ($ret_member =~ m/(?:admin|remote)_nonnull_(secret|nwfilter|nwfilter_binding|node_device|interface|network|storage_vol|storage_pool|domain_snapshot|domain|server|client) (\S+)<(\S+)>;/) { $modern_ret_struct_name = $1; $single_ret_list_error_msg_type = $1; $single_ret_list_name = $2; @@ -780,7 +780,7 @@ elsif ($mode eq "server") { $single_ret_var = $1; $single_ret_by_ref = 0; $single_ret_check = " == NULL"; - } elsif ($ret_member =~ m/^remote_nonnull_(domain|network|storage_pool|storage_vol|interface|node_device|secret|nwfilter|domain_snapshot) (\S+);/) { + } elsif ($ret_member =~ m/^remote_nonnull_(domain|network|storage_pool|storage_vol|interface|node_device|secret|nwfilter|nwfilter_binding|domain_snapshot) (\S+);/) { my $type_name = name_to_TypeName($1); if ($call->{ProcName} eq "DomainCreateWithFlags") { @@ -1325,7 +1325,7 @@ elsif ($mode eq "client") { $priv_src = "dev->conn"; push(@args_list, "virNodeDevicePtr dev"); push(@setters_list, "args.name = dev->name;"); - } elsif ($args_member =~ m/^remote_nonnull_(domain|network|storage_pool|storage_vol|interface|secret|nwfilter|domain_snapshot) (\S+);/) { + } elsif ($args_member =~ m/^remote_nonnull_(domain|network|storage_pool|storage_vol|interface|secret|nwfilter|nwfilter_binding|domain_snapshot) (\S+);/) { my $name = $1; my $arg_name = $2; my $type_name = name_to_TypeName($name); @@ -1518,7 +1518,7 @@ elsif ($mode eq "client") { } push(@ret_list, "memcpy(result->$3, ret.$3, sizeof(result->$3));"); - } elsif ($ret_member =~ m/(?:admin|remote)_nonnull_(secret|nwfilter|node_device|interface|network|storage_vol|storage_pool|domain_snapshot|domain|server|client) (\S+)<(\S+)>;/) { + } elsif ($ret_member =~ m/(?:admin|remote)_nonnull_(secret|nwfilter|nwfilter_binding|node_device|interface|network|storage_vol|storage_pool|domain_snapshot|domain|server|client) (\S+)<(\S+)>;/) { my $proc_name = name_to_TypeName($1); if ($structprefix eq "admin") { @@ -1571,7 +1571,7 @@ elsif ($mode eq "client") { push(@ret_list, "VIR_FREE(ret.$1);"); $single_ret_var = "char *rv = NULL"; $single_ret_type = "char *"; - } elsif ($ret_member =~ m/^remote_nonnull_(domain|network|storage_pool|storage_vol|node_device|interface|secret|nwfilter|domain_snapshot) (\S+);/) { + } elsif ($ret_member =~ m/^remote_nonnull_(domain|network|storage_pool|storage_vol|node_device|interface|secret|nwfilter|nwfilter_binding|domain_snapshot) (\S+);/) { my $name = $1; my $arg_name = $2; my $type_name = name_to_TypeName($name); -- 2.17.0

$ virsh nwfilter-binding-list Port Dev Filter ------------------------------------------------------------------ vnet0 clean-traffic vnet1 clean-traffic $ virsh nwfilter-binding-dumpxml vnet1 <filterbinding> <owner> <name>f25arm7</name> <uuid>12ac8b8c-4f23-4248-ae42-fdcd50c400fd</uuid> </owner> <portdev name='vnet1'/> <mac address='52:54:00:9d:81:b1'/> <filterref filter='clean-traffic'> <parameter name='MAC' value='52:54:00:9d:81:b1'/> </filterref> </filterbinding> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tools/virsh-completer.c | 45 ++++++ tools/virsh-completer.h | 4 + tools/virsh-nwfilter.c | 317 ++++++++++++++++++++++++++++++++++++++++ tools/virsh-nwfilter.h | 8 + 4 files changed, 374 insertions(+) diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c index d3effe59ea..0dc6ae82e1 100644 --- a/tools/virsh-completer.c +++ b/tools/virsh-completer.c @@ -473,6 +473,51 @@ virshNWFilterNameCompleter(vshControl *ctl, } +char ** +virshNWFilterBindingNameCompleter(vshControl *ctl, + const vshCmd *cmd ATTRIBUTE_UNUSED, + unsigned int flags) +{ + virshControlPtr priv = ctl->privData; + virNWFilterBindingPtr *bindings = NULL; + int nbindings = 0; + size_t i = 0; + char **ret = NULL; + + virCheckFlags(0, NULL); + + if (!priv->conn || virConnectIsAlive(priv->conn) <= 0) + return NULL; + + if ((nbindings = virConnectListAllNWFilterBindings(priv->conn, &bindings, flags)) < 0) + return NULL; + + if (VIR_ALLOC_N(ret, nbindings + 1) < 0) + goto error; + + for (i = 0; i < nbindings; i++) { + const char *name = virNWFilterBindingGetPortDev(bindings[i]); + + if (VIR_STRDUP(ret[i], name) < 0) + goto error; + + virNWFilterBindingFree(bindings[i]); + } + VIR_FREE(bindings); + + return ret; + + error: + for (; i < nbindings; i++) + virNWFilterBindingFree(bindings[i]); + VIR_FREE(bindings); + for (i = 0; i < nbindings; i++) + VIR_FREE(ret[i]); + VIR_FREE(ret); + return NULL; +} + + char ** virshSecretUUIDCompleter(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED, diff --git a/tools/virsh-completer.h b/tools/virsh-completer.h index ee7eec68c5..fcff040135 100644 --- a/tools/virsh-completer.h +++ b/tools/virsh-completer.h @@ -66,6 +66,10 @@ char ** virshNWFilterNameCompleter(vshControl *ctl, const vshCmd *cmd, unsigned int flags); +char ** virshNWFilterBindingNameCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags); + char ** virshSecretUUIDCompleter(vshControl *ctl, const vshCmd *cmd, unsigned int flags); diff --git a/tools/virsh-nwfilter.c b/tools/virsh-nwfilter.c index 06a002dffd..1cdbe5053a 100644 --- a/tools/virsh-nwfilter.c +++ b/tools/virsh-nwfilter.c @@ -443,6 +443,299 @@ cmdNWFilterEdit(vshControl *ctl, const vshCmd *cmd) return ret; } + +virNWFilterBindingPtr +virshCommandOptNWFilterBindingBy(vshControl *ctl, + const vshCmd *cmd, + const char **name, + unsigned int flags) +{ + virNWFilterBindingPtr binding = NULL; + const char *n = NULL; + const char *optname = "binding"; + virshControlPtr priv = ctl->privData; + + virCheckFlags(0, NULL); + + if (vshCommandOptStringReq(ctl, cmd, optname, &n) < 0) + return NULL; + + vshDebug(ctl, VSH_ERR_INFO, "%s: found option <%s>: %s\n", + cmd->def->name, optname, n); + + if (name) + *name = n; + + vshDebug(ctl, VSH_ERR_DEBUG, "%s: <%s> trying as nwfilter binding port dev\n", + cmd->def->name, optname); + binding = virNWFilterBindingLookupByPortDev(priv->conn, n); + + if (!binding) + vshError(ctl, _("failed to get nwfilter binding '%s'"), n); + + return binding; +} + + +/* + * "nwfilter-binding-create" command + */ +static const vshCmdInfo info_nwfilter_binding_create[] = { + {.name = "help", + .data = N_("create a network filter binding from an XML file") + }, + {.name = "desc", + .data = N_("Create a new network filter binding.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_nwfilter_binding_create[] = { + VIRSH_COMMON_OPT_FILE(N_("file containing an XML network " + "filter binding description")), + {.name = NULL} +}; + +static bool +cmdNWFilterBindingCreate(vshControl *ctl, const vshCmd *cmd) +{ + virNWFilterBindingPtr binding; + const char *from = NULL; + bool ret = true; + char *buffer; + virshControlPtr priv = ctl->privData; + + if (vshCommandOptStringReq(ctl, cmd, "file", &from) < 0) + return false; + + if (virFileReadAll(from, VSH_MAX_XML_FILE, &buffer) < 0) + return false; + + binding = virNWFilterBindingCreateXML(priv->conn, buffer, 0); + VIR_FREE(buffer); + + if (binding != NULL) { + vshPrintExtra(ctl, _("Network filter binding on %s created from %s\n"), + virNWFilterBindingGetPortDev(binding), from); + virNWFilterBindingFree(binding); + } else { + vshError(ctl, _("Failed to create network filter from %s"), from); + ret = false; + } + return ret; +} + + +/* + * "nwfilter-binding-delete" command + */ +static const vshCmdInfo info_nwfilter_binding_delete[] = { + {.name = "help", + .data = N_("delete a network filter binding") + }, + {.name = "desc", + .data = N_("Delete a given network filter binding.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_nwfilter_binding_delete[] = { + {.name = "binding", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("network filter binding port dev"), + .completer = virshNWFilterBindingNameCompleter, + }, + {.name = NULL} +}; + +static bool +cmdNWFilterBindingDelete(vshControl *ctl, const vshCmd *cmd) +{ + virNWFilterBindingPtr binding; + bool ret = true; + const char *portdev; + + if (!(binding = virshCommandOptNWFilterBinding(ctl, cmd, &portdev))) + return false; + + if (virNWFilterBindingDelete(binding) == 0) { + vshPrintExtra(ctl, _("Network filter binding on %s deleted\n"), portdev); + } else { + vshError(ctl, _("Failed to delete network filter binding on %s"), portdev); + ret = false; + } + + virNWFilterBindingFree(binding); + return ret; +} + + +/* + * "nwfilter-binding-dumpxml" command + */ +static const vshCmdInfo info_nwfilter_binding_dumpxml[] = { + {.name = "help", + .data = N_("network filter information in XML") + }, + {.name = "desc", + .data = N_("Output the network filter information as an XML dump to stdout.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_nwfilter_binding_dumpxml[] = { + {.name = "binding", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("network filter binding portdev"), + .completer = virshNWFilterBindingNameCompleter, + }, + {.name = NULL} +}; + +static bool +cmdNWFilterBindingDumpXML(vshControl *ctl, const vshCmd *cmd) +{ + virNWFilterBindingPtr binding; + bool ret = true; + char *dump; + + if (!(binding = virshCommandOptNWFilterBinding(ctl, cmd, NULL))) + return false; + + dump = virNWFilterBindingGetXMLDesc(binding, 0); + if (dump != NULL) { + vshPrint(ctl, "%s", dump); + VIR_FREE(dump); + } else { + ret = false; + } + + virNWFilterBindingFree(binding); + return ret; +} + + +static int +virshNWFilterBindingSorter(const void *a, const void *b) +{ + virNWFilterBindingPtr *fa = (virNWFilterBindingPtr *) a; + virNWFilterBindingPtr *fb = (virNWFilterBindingPtr *) b; + + if (*fa && !*fb) + return -1; + + if (!*fa) + return *fb != NULL; + + return vshStrcasecmp(virNWFilterBindingGetPortDev(*fa), + virNWFilterBindingGetPortDev(*fb)); +} + + +struct virshNWFilterBindingList { + virNWFilterBindingPtr *bindings; + size_t nbindings; +}; +typedef struct virshNWFilterBindingList *virshNWFilterBindingListPtr; + + +static void +virshNWFilterBindingListFree(virshNWFilterBindingListPtr list) +{ + size_t i; + + if (list && list->bindings) { + for (i = 0; i < list->nbindings; i++) { + if (list->bindings[i]) + virNWFilterBindingFree(list->bindings[i]); + } + VIR_FREE(list->bindings); + } + VIR_FREE(list); +} + + +static virshNWFilterBindingListPtr +virshNWFilterBindingListCollect(vshControl *ctl, + unsigned int flags) +{ + virshNWFilterBindingListPtr list = vshMalloc(ctl, sizeof(*list)); + int ret; + bool success = false; + virshControlPtr priv = ctl->privData; + + if ((ret = virConnectListAllNWFilterBindings(priv->conn, + &list->bindings, + flags)) < 0) { + /* there was an error during the call */ + vshError(ctl, "%s", _("Failed to list network filter bindings")); + goto cleanup; + } + + list->nbindings = ret; + + /* sort the list */ + if (list->bindings && list->nbindings > 1) + qsort(list->bindings, list->nbindings, + sizeof(*list->bindings), virshNWFilterBindingSorter); + + success = true; + + cleanup: + if (!success) { + virshNWFilterBindingListFree(list); + list = NULL; + } + + return list; +} + + +/* + * "nwfilter-binding-list" command + */ +static const vshCmdInfo info_nwfilter_binding_list[] = { + {.name = "help", + .data = N_("list network filter bindings") + }, + {.name = "desc", + .data = N_("Returns list of network filter bindings.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_nwfilter_binding_list[] = { + {.name = NULL} +}; + +static bool +cmdNWFilterBindingList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) +{ + size_t i; + virshNWFilterBindingListPtr list = NULL; + + if (!(list = virshNWFilterBindingListCollect(ctl, 0))) + return false; + + vshPrintExtra(ctl, " %-20s %-20s \n", _("Port Dev"), _("Filter")); + vshPrintExtra(ctl, "---------------------------------" + "---------------------------------\n"); + + for (i = 0; i < list->nbindings; i++) { + virNWFilterBindingPtr binding = list->bindings[i]; + + vshPrint(ctl, " %-20s %-20s\n", + virNWFilterBindingGetPortDev(binding), + virNWFilterBindingGetFilterName(binding)); + } + + virshNWFilterBindingListFree(list); + return true; +} + + const vshCmdDef nwfilterCmds[] = { {.name = "nwfilter-define", .handler = cmdNWFilterDefine, @@ -474,5 +767,29 @@ const vshCmdDef nwfilterCmds[] = { .info = info_nwfilter_undefine, .flags = 0 }, + {.name = "nwfilter-binding-create", + .handler = cmdNWFilterBindingCreate, + .opts = opts_nwfilter_binding_create, + .info = info_nwfilter_binding_create, + .flags = 0 + }, + {.name = "nwfilter-binding-delete", + .handler = cmdNWFilterBindingDelete, + .opts = opts_nwfilter_binding_delete, + .info = info_nwfilter_binding_delete, + .flags = 0 + }, + {.name = "nwfilter-binding-dumpxml", + .handler = cmdNWFilterBindingDumpXML, + .opts = opts_nwfilter_binding_dumpxml, + .info = info_nwfilter_binding_dumpxml, + .flags = 0 + }, + {.name = "nwfilter-binding-list", + .handler = cmdNWFilterBindingList, + .opts = opts_nwfilter_binding_list, + .info = info_nwfilter_binding_list, + .flags = 0 + }, {.name = NULL} }; diff --git a/tools/virsh-nwfilter.h b/tools/virsh-nwfilter.h index 2b76a7c849..d8ca0e3960 100644 --- a/tools/virsh-nwfilter.h +++ b/tools/virsh-nwfilter.h @@ -32,11 +32,19 @@ virNWFilterPtr virshCommandOptNWFilterBy(vshControl *ctl, const vshCmd *cmd, const char **name, unsigned int flags); +virNWFilterBindingPtr +virshCommandOptNWFilterBindingBy(vshControl *ctl, const vshCmd *cmd, + const char **name, unsigned int flags); + /* default is lookup by Name and UUID */ # define virshCommandOptNWFilter(_ctl, _cmd, _name) \ virshCommandOptNWFilterBy(_ctl, _cmd, _name, \ VIRSH_BYUUID | VIRSH_BYNAME) +/* default is lookup by port dev */ +# define virshCommandOptNWFilterBinding(_ctl, _cmd, _name) \ + virshCommandOptNWFilterBindingBy(_ctl, _cmd, _name, 0) + extern const vshCmdDef nwfilterCmds[]; #endif /* VIRSH_NWFILTER_H */ -- 2.17.0

On 06/14/2018 08:32 AM, Daniel P. Berrangé wrote:
$ virsh nwfilter-binding-list Port Dev Filter ------------------------------------------------------------------ vnet0 clean-traffic vnet1 clean-traffic
$ virsh nwfilter-binding-dumpxml vnet1 <filterbinding> <owner> <name>f25arm7</name> <uuid>12ac8b8c-4f23-4248-ae42-fdcd50c400fd</uuid> </owner> <portdev name='vnet1'/> <mac address='52:54:00:9d:81:b1'/> <filterref filter='clean-traffic'> <parameter name='MAC' value='52:54:00:9d:81:b1'/> </filterref> </filterbinding>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tools/virsh-completer.c | 45 ++++++ tools/virsh-completer.h | 4 + tools/virsh-nwfilter.c | 317 ++++++++++++++++++++++++++++++++++++++++ tools/virsh-nwfilter.h | 8 + 4 files changed, 374 insertions(+)
Will need virsh.pod changes to describe the new commands. I'm OK if you just post an update as a reply. The rest looked fine - although a few more usage comments in the new API's may be nice. John Would probably be a very interesting Avocado test to write - something that continue creates/deletes the binding while perhaps the guest is doing the start/destroy loop

On Sun, Jun 17, 2018 at 08:27:27AM -0400, John Ferlan wrote:
On 06/14/2018 08:32 AM, Daniel P. Berrangé wrote:
$ virsh nwfilter-binding-list Port Dev Filter ------------------------------------------------------------------ vnet0 clean-traffic vnet1 clean-traffic
$ virsh nwfilter-binding-dumpxml vnet1 <filterbinding> <owner> <name>f25arm7</name> <uuid>12ac8b8c-4f23-4248-ae42-fdcd50c400fd</uuid> </owner> <portdev name='vnet1'/> <mac address='52:54:00:9d:81:b1'/> <filterref filter='clean-traffic'> <parameter name='MAC' value='52:54:00:9d:81:b1'/> </filterref> </filterbinding>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tools/virsh-completer.c | 45 ++++++ tools/virsh-completer.h | 4 + tools/virsh-nwfilter.c | 317 ++++++++++++++++++++++++++++++++++++++++ tools/virsh-nwfilter.h | 8 + 4 files changed, 374 insertions(+)
Will need virsh.pod changes to describe the new commands. I'm OK if you just post an update as a reply.
I'll send a further patch with POD docs Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Use the virNWFilterBindingDefPtr struct in the gentech driver code directly. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/nwfilter/nwfilter_dhcpsnoop.c | 39 ++-- src/nwfilter/nwfilter_driver.c | 24 ++- src/nwfilter/nwfilter_gentech_driver.c | 236 ++++++++++++------------- src/nwfilter/nwfilter_gentech_driver.h | 22 ++- src/nwfilter/nwfilter_learnipaddr.c | 18 +- 5 files changed, 175 insertions(+), 164 deletions(-) diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index aff062ca7c..7274b03c2a 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -497,15 +497,20 @@ virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLeasePtr ipl, /* instantiate the filters */ - if (req->ifname) + if (req->ifname) { + virNWFilterBindingDef binding = { + .portdevname = req->ifname, + .linkdevname = req->linkdev, + .mac = req->macaddr, + .filter = req->filtername, + .filterparams = req->vars, + .ownername = NULL, + .owneruuid = {0}, + }; rc = virNWFilterInstantiateFilterLate(req->driver, - NULL, - req->ifname, - req->ifindex, - req->linkdev, - &req->macaddr, - req->filtername, - req->vars); + &binding, + req->ifindex); + } exit_snooprequnlock: virNWFilterSnoopReqUnlock(req); @@ -884,14 +889,18 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReqPtr req, goto skip_instantiate; if (ipAddrLeft) { + virNWFilterBindingDef binding = { + .portdevname = req->ifname, + .linkdevname = req->linkdev, + .mac = req->macaddr, + .filter = req->filtername, + .filterparams = req->vars, + .ownername = NULL, + .owneruuid = {0}, + }; ret = virNWFilterInstantiateFilterLate(req->driver, - NULL, - req->ifname, - req->ifindex, - req->linkdev, - &req->macaddr, - req->filtername, - req->vars); + &binding, + req->ifindex); } else { virNWFilterVarValuePtr dhcpsrvrs = virHashLookup(req->vars, NWFILTER_VARNAME_DHCPSERVER); diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index d17a8ec00b..7202691646 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -38,6 +38,7 @@ #include "domain_conf.h" #include "domain_nwfilter.h" #include "nwfilter_driver.h" +#include "virnwfilterbindingdef.h" #include "nwfilter_gentech_driver.h" #include "configmake.h" #include "virfile.h" @@ -642,19 +643,36 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter, static int -nwfilterInstantiateFilter(const char *vmname ATTRIBUTE_UNUSED, +nwfilterInstantiateFilter(const char *vmname, const unsigned char *vmuuid, virDomainNetDefPtr net) { - return virNWFilterInstantiateFilter(driver, vmuuid, net); + virNWFilterBindingDefPtr binding; + int ret; + + if (!(binding = virNWFilterBindingDefForNet(vmname, vmuuid, net))) + return -1; + ret = virNWFilterInstantiateFilter(driver, binding); + virNWFilterBindingDefFree(binding); + return ret; } static void nwfilterTeardownFilter(virDomainNetDefPtr net) { + virNWFilterBindingDef binding = { + .portdevname = net->ifname, + .linkdevname = (net->type == VIR_DOMAIN_NET_TYPE_DIRECT ? + net->data.direct.linkdev : NULL), + .mac = net->mac, + .filter = net->filter, + .filterparams = net->filterparams, + .ownername = NULL, + .owneruuid = {0}, + }; if ((net->ifname) && (net->filter)) - virNWFilterTeardownFilter(net); + virNWFilterTeardownFilter(&binding); } diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index af4411d4db..23e9998f53 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -182,33 +182,6 @@ virNWFilterVarHashmapAddStdValues(virHashTablePtr table, } -/** - * virNWFilterCreateVarHashmap: - * @macaddr: pointer to string containing formatted MAC address of interface - * @ipaddr: pointer to string containing formatted IP address used by - * VM on this interface; may be NULL - * - * Create a hashmap used for evaluating the firewall rules. Initializes - * it with the standard variable 'MAC' and 'IP' if provided. - * - * Returns pointer to hashmap, NULL if an error occurred. - */ -virHashTablePtr -virNWFilterCreateVarHashmap(const char *macaddr, - const virNWFilterVarValue *ipaddr) -{ - virHashTablePtr table = virNWFilterHashTableCreate(0); - if (!table) - return NULL; - - if (virNWFilterVarHashmapAddStdValues(table, macaddr, ipaddr) < 0) { - virHashFree(table); - return NULL; - } - return table; -} - - /** * Convert a virHashTable into a string of comma-separated * variable names. @@ -577,12 +550,9 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, /** * virNWFilterDoInstantiate: - * @vmuuid: The UUID of the VM * @techdriver: The driver to use for instantiation + * @binding: description of port to bind the filter to * @filter: The filter to instantiate - * @ifname: The name of the interface to apply the rules to - * @vars: A map holding variable names and values used for instantiating - * the filter and its subfilters. * @forceWithPendingReq: Ignore the check whether a pending learn request * is active; 'true' only when the rules are applied late * @@ -596,17 +566,13 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, * Call this function while holding the NWFilter filter update lock */ static int -virNWFilterDoInstantiate(const unsigned char *vmuuid, - virNWFilterTechDriverPtr techdriver, +virNWFilterDoInstantiate(virNWFilterTechDriverPtr techdriver, + virNWFilterBindingDefPtr binding, virNWFilterDefPtr filter, - const char *ifname, int ifindex, - const char *linkdev, - virHashTablePtr vars, enum instCase useNewFilter, bool *foundNewFilter, bool teardownOld, - const virMacAddr *macaddr, virNWFilterDriverStatePtr driver, bool forceWithPendingReq) { @@ -628,14 +594,14 @@ virNWFilterDoInstantiate(const unsigned char *vmuuid, } rc = virNWFilterDetermineMissingVarsRec(filter, - vars, + binding->filterparams, missing_vars, useNewFilter, driver); if (rc < 0) goto err_exit; - lv = virHashLookup(vars, NWFILTER_VARNAME_CTRL_IP_LEARNING); + lv = virHashLookup(binding->filterparams, NWFILTER_VARNAME_CTRL_IP_LEARNING); if (lv) learning = virNWFilterVarValueGetNthValue(lv, 0); else @@ -652,19 +618,20 @@ virNWFilterDoInstantiate(const unsigned char *vmuuid, goto err_unresolvable_vars; } if (STRCASEEQ(learning, "dhcp")) { - rc = virNWFilterDHCPSnoopReq(techdriver, ifname, linkdev, - vmuuid, macaddr, - filter->name, vars, driver); + rc = virNWFilterDHCPSnoopReq(techdriver, binding->portdevname, + binding->linkdevname, + binding->owneruuid, &binding->mac, + filter->name, binding->filterparams, driver); goto err_exit; } else if (STRCASEEQ(learning, "any")) { if (!virNWFilterHasLearnReq(ifindex)) { rc = virNWFilterLearnIPAddress(techdriver, - ifname, + binding->portdevname, ifindex, - linkdev, - macaddr, + binding->linkdevname, + &binding->mac, filter->name, - vars, driver, + binding->filterparams, driver, DETECT_DHCP|DETECT_STATIC); } goto err_exit; @@ -688,7 +655,7 @@ virNWFilterDoInstantiate(const unsigned char *vmuuid, rc = virNWFilterDefToInst(driver, filter, - vars, + binding->filterparams, useNewFilter, foundNewFilter, &inst); @@ -705,22 +672,22 @@ virNWFilterDoInstantiate(const unsigned char *vmuuid, } if (instantiate) { - if (virNWFilterLockIface(ifname) < 0) + if (virNWFilterLockIface(binding->portdevname) < 0) goto err_exit; - rc = techdriver->applyNewRules(ifname, inst.rules, inst.nrules); + rc = techdriver->applyNewRules(binding->portdevname, inst.rules, inst.nrules); if (teardownOld && rc == 0) - techdriver->tearOldRules(ifname); + techdriver->tearOldRules(binding->portdevname); - if (rc == 0 && (virNetDevValidateConfig(ifname, NULL, ifindex) <= 0)) { + if (rc == 0 && (virNetDevValidateConfig(binding->portdevname, NULL, ifindex) <= 0)) { virResetLastError(); /* interface changed/disppeared */ - techdriver->allTeardown(ifname); + techdriver->allTeardown(binding->portdevname); rc = -1; } - virNWFilterUnlockIface(ifname); + virNWFilterUnlockIface(binding->portdevname); } err_exit: @@ -749,14 +716,9 @@ virNWFilterDoInstantiate(const unsigned char *vmuuid, */ static int virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, bool teardownOld, - const char *ifname, + virNWFilterBindingDefPtr binding, int ifindex, - const char *linkdev, - const virMacAddr *macaddr, - const char *filtername, - virHashTablePtr filterparams, enum instCase useNewFilter, bool forceWithPendingReq, bool *foundNewFilter) @@ -765,7 +727,6 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, const char *drvname = EBIPTABLES_DRIVER_ID; virNWFilterTechDriverPtr techdriver; virNWFilterObjPtr obj; - virHashTablePtr vars, vars1; virNWFilterDefPtr filter; virNWFilterDefPtr newFilter; char vmmacaddr[VIR_MAC_STRING_BUFLEN] = {0}; @@ -781,29 +742,22 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, return -1; } - VIR_DEBUG("filter name: %s", filtername); + VIR_DEBUG("filter name: %s", binding->filter); if (!(obj = virNWFilterObjListFindInstantiateFilter(driver->nwfilters, - filtername))) + binding->filter))) return -1; - virMacAddrFormat(macaddr, vmmacaddr); + virMacAddrFormat(&binding->mac, vmmacaddr); - ipaddr = virNWFilterIPAddrMapGetIPAddr(ifname); + ipaddr = virNWFilterIPAddrMapGetIPAddr(binding->portdevname); - vars1 = virNWFilterCreateVarHashmap(vmmacaddr, ipaddr); - if (!vars1) { + if (virNWFilterVarHashmapAddStdValues(binding->filterparams, + vmmacaddr, ipaddr) < 0) { rc = -1; goto err_exit; } - vars = virNWFilterCreateVarsFrom(vars1, - filterparams); - if (!vars) { - rc = -1; - goto err_exit_vars1; - } - filter = virNWFilterObjGetDef(obj); switch (useNewFilter) { @@ -819,17 +773,11 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, break; } - rc = virNWFilterDoInstantiate(vmuuid, techdriver, filter, - ifname, ifindex, linkdev, - vars, useNewFilter, foundNewFilter, - teardownOld, macaddr, driver, + rc = virNWFilterDoInstantiate(techdriver, binding, filter, + ifindex, useNewFilter, foundNewFilter, + teardownOld, driver, forceWithPendingReq); - virHashFree(vars); - - err_exit_vars1: - virHashFree(vars1); - err_exit: virNWFilterObjUnlock(obj); @@ -839,15 +787,11 @@ virNWFilterInstantiateFilterUpdate(virNWFilterDriverStatePtr driver, static int virNWFilterInstantiateFilterInternal(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, - const virDomainNetDef *net, + virNWFilterBindingDefPtr binding, bool teardownOld, enum instCase useNewFilter, bool *foundNewFilter) { - const char *linkdev = (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) - ? net->data.direct.linkdev - : NULL; int ifindex; int rc; @@ -856,8 +800,8 @@ virNWFilterInstantiateFilterInternal(virNWFilterDriverStatePtr driver, /* after grabbing the filter update lock check for the interface; if it's not there anymore its filters will be or are being removed (while holding the lock) and we don't want to build new ones */ - if (virNetDevExists(net->ifname) != 1 || - virNetDevGetIndex(net->ifname, &ifindex) < 0) { + if (virNetDevExists(binding->portdevname) != 1 || + virNetDevGetIndex(binding->portdevname, &ifindex) < 0) { /* interfaces / VMs can disappear during filter instantiation; don't mark it as an error */ virResetLastError(); @@ -865,10 +809,10 @@ virNWFilterInstantiateFilterInternal(virNWFilterDriverStatePtr driver, goto cleanup; } - rc = virNWFilterInstantiateFilterUpdate(driver, vmuuid, teardownOld, - net->ifname, ifindex, linkdev, - &net->mac, net->filter, - net->filterparams, useNewFilter, + rc = virNWFilterInstantiateFilterUpdate(driver, teardownOld, + binding, + ifindex, + useNewFilter, false, foundNewFilter); cleanup: @@ -880,13 +824,8 @@ virNWFilterInstantiateFilterInternal(virNWFilterDriverStatePtr driver, int virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, - const char *ifname, - int ifindex, - const char *linkdev, - const virMacAddr *macaddr, - const char *filtername, - virHashTablePtr filterparams) + virNWFilterBindingDefPtr binding, + int ifindex) { int rc; bool foundNewFilter = false; @@ -894,18 +833,17 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, virNWFilterReadLockFilterUpdates(); virMutexLock(&updateMutex); - rc = virNWFilterInstantiateFilterUpdate(driver, vmuuid, true, - ifname, ifindex, linkdev, - macaddr, filtername, filterparams, + rc = virNWFilterInstantiateFilterUpdate(driver, true, + binding, ifindex, INSTANTIATE_ALWAYS, true, &foundNewFilter); if (rc < 0) { /* something went wrong... 'DOWN' the interface */ - if ((virNetDevValidateConfig(ifname, NULL, ifindex) <= 0) || - (virNetDevSetOnline(ifname, false) < 0)) { + if ((virNetDevValidateConfig(binding->portdevname, NULL, ifindex) <= 0) || + (virNetDevSetOnline(binding->portdevname, false) < 0)) { virResetLastError(); /* assuming interface disappeared... */ - _virNWFilterTeardownFilter(ifname); + _virNWFilterTeardownFilter(binding->portdevname); } } @@ -918,12 +856,11 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, int virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, - const virDomainNetDef *net) + virNWFilterBindingDefPtr binding) { bool foundNewFilter = false; - return virNWFilterInstantiateFilterInternal(driver, vmuuid, net, + return virNWFilterInstantiateFilterInternal(driver, binding, 1, INSTANTIATE_ALWAYS, &foundNewFilter); @@ -932,13 +869,12 @@ virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, int virNWFilterUpdateInstantiateFilter(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, - const virDomainNetDef *net, + virNWFilterBindingDefPtr binding, bool *skipIface) { bool foundNewFilter = false; - int rc = virNWFilterInstantiateFilterInternal(driver, vmuuid, net, + int rc = virNWFilterInstantiateFilterInternal(driver, binding, 0, INSTANTIATE_FOLLOW_NEWFILTER, &foundNewFilter); @@ -948,7 +884,7 @@ virNWFilterUpdateInstantiateFilter(virNWFilterDriverStatePtr driver, } static int -virNWFilterRollbackUpdateFilter(const virDomainNetDef *net) +virNWFilterRollbackUpdateFilter(virNWFilterBindingDefPtr binding) { const char *drvname = EBIPTABLES_DRIVER_ID; int ifindex; @@ -964,17 +900,17 @@ virNWFilterRollbackUpdateFilter(const virDomainNetDef *net) } /* don't tear anything while the address is being learned */ - if (virNetDevGetIndex(net->ifname, &ifindex) < 0) + if (virNetDevGetIndex(binding->portdevname, &ifindex) < 0) virResetLastError(); else if (virNWFilterHasLearnReq(ifindex)) return 0; - return techdriver->tearNewRules(net->ifname); + return techdriver->tearNewRules(binding->portdevname); } static int -virNWFilterTearOldFilter(virDomainNetDefPtr net) +virNWFilterTearOldFilter(virNWFilterBindingDefPtr binding) { const char *drvname = EBIPTABLES_DRIVER_ID; int ifindex; @@ -990,12 +926,12 @@ virNWFilterTearOldFilter(virDomainNetDefPtr net) } /* don't tear anything while the address is being learned */ - if (virNetDevGetIndex(net->ifname, &ifindex) < 0) + if (virNetDevGetIndex(binding->portdevname, &ifindex) < 0) virResetLastError(); else if (virNWFilterHasLearnReq(ifindex)) return 0; - return techdriver->tearOldRules(net->ifname); + return techdriver->tearOldRules(binding->portdevname); } @@ -1032,11 +968,11 @@ _virNWFilterTeardownFilter(const char *ifname) int -virNWFilterTeardownFilter(const virDomainNetDef *net) +virNWFilterTeardownFilter(virNWFilterBindingDefPtr binding) { int ret; virMutexLock(&updateMutex); - ret = _virNWFilterTeardownFilter(net->ifname); + ret = _virNWFilterTeardownFilter(binding->portdevname); virMutexUnlock(&updateMutex); return ret; } @@ -1057,12 +993,16 @@ virNWFilterDomainFWUpdateCB(virDomainObjPtr obj, if (virDomainObjIsActive(obj)) { for (i = 0; i < vm->nnets; i++) { virDomainNetDefPtr net = vm->nets[i]; - if ((net->filter) && (net->ifname)) { + virNWFilterBindingDefPtr binding; + + if ((net->filter) && (net->ifname) && + (binding = virNWFilterBindingDefForNet( + vm->name, vm->uuid, net))) { + switch (cb->step) { case STEP_APPLY_NEW: ret = virNWFilterUpdateInstantiateFilter(cb->opaque, - vm->uuid, - net, + binding, &skipIface); if (ret == 0 && skipIface) { /* filter tree unchanged -- no update needed */ @@ -1074,24 +1014,24 @@ virNWFilterDomainFWUpdateCB(virDomainObjPtr obj, case STEP_TEAR_NEW: if (!virHashLookup(cb->skipInterfaces, net->ifname)) - ret = virNWFilterRollbackUpdateFilter(net); + ret = virNWFilterRollbackUpdateFilter(binding); break; case STEP_TEAR_OLD: if (!virHashLookup(cb->skipInterfaces, net->ifname)) - ret = virNWFilterTearOldFilter(net); + ret = virNWFilterTearOldFilter(binding); break; case STEP_APPLY_CURRENT: ret = virNWFilterInstantiateFilter(cb->opaque, - vm->uuid, - net); + binding); if (ret) virReportError(VIR_ERR_INTERNAL_ERROR, _("Failure while applying current filter on " "VM %s"), vm->name); break; } + virNWFilterBindingDefFree(binding); if (ret) break; } @@ -1101,3 +1041,45 @@ virNWFilterDomainFWUpdateCB(virDomainObjPtr obj, virObjectUnlock(obj); return ret; } + + +virNWFilterBindingDefPtr +virNWFilterBindingDefForNet(const char *vmname, + const unsigned char *vmuuid, + virDomainNetDefPtr net) +{ + virNWFilterBindingDefPtr ret; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + if (VIR_STRDUP(ret->ownername, vmname) < 0) + goto error; + + memcpy(ret->owneruuid, vmuuid, sizeof(ret->owneruuid)); + + if (VIR_STRDUP(ret->portdevname, net->ifname) < 0) + goto error; + + if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT && + VIR_STRDUP(ret->linkdevname, net->data.direct.linkdev) < 0) + goto error; + + ret->mac = net->mac; + + if (VIR_STRDUP(ret->filter, net->filter) < 0) + goto error; + + if (!(ret->filterparams = virNWFilterHashTableCreate(0))) + goto error; + + if (net->filterparams && + virNWFilterHashTablePutAll(net->filterparams, ret->filterparams) < 0) + goto error; + + return ret; + + error: + virNWFilterBindingDefFree(ret); + return NULL; +} diff --git a/src/nwfilter/nwfilter_gentech_driver.h b/src/nwfilter/nwfilter_gentech_driver.h index 9e43a159c3..6b51096a0d 100644 --- a/src/nwfilter/nwfilter_gentech_driver.h +++ b/src/nwfilter/nwfilter_gentech_driver.h @@ -25,6 +25,7 @@ # define __NWFILTER_GENTECH_DRIVER_H # include "virnwfilterobj.h" +# include "virnwfilterbindingdef.h" # include "nwfilter_tech_driver.h" virNWFilterTechDriverPtr virNWFilterTechDriverForName(const char *name); @@ -39,23 +40,16 @@ enum instCase { int virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, - const virDomainNetDef *net); + virNWFilterBindingDefPtr binding); int virNWFilterUpdateInstantiateFilter(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, - const virDomainNetDef *net, + virNWFilterBindingDefPtr binding, bool *skipIface); int virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver, - const unsigned char *vmuuid, - const char *ifname, - int ifindex, - const char *linkdev, - const virMacAddr *macaddr, - const char *filtername, - virHashTablePtr filterparams); + virNWFilterBindingDefPtr binding, + int ifindex); -int virNWFilterTeardownFilter(const virDomainNetDef *net); +int virNWFilterTeardownFilter(virNWFilterBindingDefPtr binding); virHashTablePtr virNWFilterCreateVarHashmap(const char *macaddr, const virNWFilterVarValue *value); @@ -63,4 +57,8 @@ virHashTablePtr virNWFilterCreateVarHashmap(const char *macaddr, int virNWFilterDomainFWUpdateCB(virDomainObjPtr vm, void *data); +virNWFilterBindingDefPtr virNWFilterBindingDefForNet(const char *vmname, + const unsigned char *vmuuid, + virDomainNetDefPtr net); + #endif diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index ce58f66f6d..d76d13d8d4 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -672,19 +672,23 @@ learnIPAddressThread(void *arg) virNWFilterUnlockIface(req->ifname); if ((inetaddr = virSocketAddrFormat(&sa)) != NULL) { + virNWFilterBindingDef binding = { + .portdevname = req->ifname, + .linkdevname = req->linkdev, + .mac = req->macaddr, + .filter = req->filtername, + .filterparams = req->filterparams, + .ownername = NULL, + .owneruuid = {0}, + }; if (virNWFilterIPAddrMapAddIPAddr(req->ifname, inetaddr) < 0) { VIR_ERROR(_("Failed to add IP address %s to IP address " "cache for interface %s"), inetaddr, req->ifname); } ret = virNWFilterInstantiateFilterLate(req->driver, - NULL, - req->ifname, - req->ifindex, - req->linkdev, - &req->macaddr, - req->filtername, - req->filterparams); + &binding, + req->ifindex); VIR_DEBUG("Result from applying firewall rules on " "%s with IP addr %s : %d", req->ifname, inetaddr, ret); VIR_FREE(inetaddr); -- 2.17.0

On 06/14/2018 08:32 AM, Daniel P. Berrangé wrote:
Use the virNWFilterBindingDefPtr struct in the gentech driver code directly.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/nwfilter/nwfilter_dhcpsnoop.c | 39 ++-- src/nwfilter/nwfilter_driver.c | 24 ++- src/nwfilter/nwfilter_gentech_driver.c | 236 ++++++++++++------------- src/nwfilter/nwfilter_gentech_driver.h | 22 ++- src/nwfilter/nwfilter_learnipaddr.c | 18 +- 5 files changed, 175 insertions(+), 164 deletions(-)
FWIW: existing, but virNWFilterDoInstantiate doesn't have a complete list of parameters in the method doc... Missing ifindex, useNewFilter, foundNewFilter, teardownOld, and driver. Reviewed-by: John Ferlan <jferlan@redhat.com> John

Use the virNWFilterBindingDefPTr struct in the IP address learning code directly. Reviewed-by: John Ferlan <jferlan@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/nwfilter/nwfilter_gentech_driver.c | 7 +- src/nwfilter/nwfilter_learnipaddr.c | 106 +++++++------------------ src/nwfilter/nwfilter_learnipaddr.h | 7 +- 3 files changed, 32 insertions(+), 88 deletions(-) diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 23e9998f53..ce45587a44 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -626,12 +626,9 @@ virNWFilterDoInstantiate(virNWFilterTechDriverPtr techdriver, } else if (STRCASEEQ(learning, "any")) { if (!virNWFilterHasLearnReq(ifindex)) { rc = virNWFilterLearnIPAddress(techdriver, - binding->portdevname, + binding, ifindex, - binding->linkdevname, - &binding->mac, - filter->name, - binding->filterparams, driver, + driver, DETECT_DHCP|DETECT_STATIC); } goto err_exit; diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index d76d13d8d4..55ed0bfc09 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -138,12 +138,8 @@ typedef struct _virNWFilterIPAddrLearnReq virNWFilterIPAddrLearnReq; typedef virNWFilterIPAddrLearnReq *virNWFilterIPAddrLearnReqPtr; struct _virNWFilterIPAddrLearnReq { virNWFilterTechDriverPtr techdriver; - char ifname[IF_NAMESIZE]; int ifindex; - char linkdev[IF_NAMESIZE]; - virMacAddr macaddr; - char *filtername; - virHashTablePtr filterparams; + virNWFilterBindingDefPtr binding; virNWFilterDriverStatePtr driver; int howDetect; /* bitmask of enum howDetect */ @@ -233,8 +229,7 @@ virNWFilterIPAddrLearnReqFree(virNWFilterIPAddrLearnReqPtr req) if (!req) return; - VIR_FREE(req->filtername); - virHashFree(req->filterparams); + virNWFilterBindingDefFree(req->binding); VIR_FREE(req); } @@ -405,8 +400,9 @@ learnIPAddressThread(void *arg) virNWFilterIPAddrLearnReqPtr req = arg; uint32_t vmaddr = 0, bcastaddr = 0; unsigned int ethHdrSize; - char *listen_if = (strlen(req->linkdev) != 0) ? req->linkdev - : req->ifname; + char *listen_if = (req->binding->linkdevname ? + req->binding->linkdevname : + req->binding->portdevname); int dhcp_opts_len; char macaddr[VIR_MAC_STRING_BUFLEN]; virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -417,13 +413,13 @@ learnIPAddressThread(void *arg) virNWFilterTechDriverPtr techdriver = req->techdriver; struct pollfd fds[1]; - if (virNWFilterLockIface(req->ifname) < 0) + if (virNWFilterLockIface(req->binding->portdevname) < 0) goto err_no_lock; req->status = 0; /* anything change to the VM's interface -- check at least once */ - if (virNetDevValidateConfig(req->ifname, NULL, req->ifindex) <= 0) { + if (virNetDevValidateConfig(req->binding->portdevname, NULL, req->ifindex) <= 0) { virResetLastError(); req->status = ENODEV; goto done; @@ -440,11 +436,11 @@ learnIPAddressThread(void *arg) fds[0].fd = pcap_fileno(handle); fds[0].events = POLLIN | POLLERR; - virMacAddrFormat(&req->macaddr, macaddr); + virMacAddrFormat(&req->binding->mac, macaddr); if (req->howDetect == DETECT_DHCP) { - if (techdriver->applyDHCPOnlyRules(req->ifname, - &req->macaddr, + if (techdriver->applyDHCPOnlyRules(req->binding->portdevname, + &req->binding->mac, NULL, false) < 0) { VIR_DEBUG("Unable to apply DHCP only rules"); req->status = EINVAL; @@ -452,8 +448,8 @@ learnIPAddressThread(void *arg) } virBufferAddLit(&buf, "src port 67 and dst port 68"); } else { - if (techdriver->applyBasicRules(req->ifname, - &req->macaddr) < 0) { + if (techdriver->applyBasicRules(req->binding->portdevname, + &req->binding->mac) < 0) { VIR_DEBUG("Unable to apply basic rules"); req->status = EINVAL; goto done; @@ -524,7 +520,7 @@ learnIPAddressThread(void *arg) } /* Again, already handled above, but lets be sure */ - if (virNetDevValidateConfig(req->ifname, NULL, req->ifindex) <= 0) { + if (virNetDevValidateConfig(req->binding->portdevname, NULL, req->ifindex) <= 0) { virResetLastError(); req->status = ENODEV; showError = false; @@ -556,7 +552,7 @@ learnIPAddressThread(void *arg) continue; } - if (virMacAddrCmpRaw(&req->macaddr, ether_hdr->ether_shost) == 0) { + if (virMacAddrCmpRaw(&req->binding->mac, ether_hdr->ether_shost) == 0) { /* packets from the VM */ if (etherType == ETHERTYPE_IP && @@ -595,7 +591,7 @@ learnIPAddressThread(void *arg) break; } } - } else if (virMacAddrCmpRaw(&req->macaddr, + } else if (virMacAddrCmpRaw(&req->binding->mac, ether_hdr->ether_dhost) == 0 || /* allow Broadcast replies from DHCP server */ virMacAddrIsBroadcastRaw(ether_hdr->ether_dhost)) { @@ -625,7 +621,7 @@ learnIPAddressThread(void *arg) ((char *)udphdr + sizeof(udphdr)); if (dhcp->op == 2 /* BOOTREPLY */ && virMacAddrCmpRaw( - &req->macaddr, + &req->binding->mac, &dhcp->chaddr[0]) == 0) { dhcp_opts_len = header.len - (ethHdrSize + iphdr->ihl * 4 + @@ -669,28 +665,19 @@ learnIPAddressThread(void *arg) * Also it is safe to unlock interface here because we stopped * capturing and applied necessary rules on the interface, while * instantiating a new filter doesn't require a locked interface.*/ - virNWFilterUnlockIface(req->ifname); + virNWFilterUnlockIface(req->binding->portdevname); if ((inetaddr = virSocketAddrFormat(&sa)) != NULL) { - virNWFilterBindingDef binding = { - .portdevname = req->ifname, - .linkdevname = req->linkdev, - .mac = req->macaddr, - .filter = req->filtername, - .filterparams = req->filterparams, - .ownername = NULL, - .owneruuid = {0}, - }; - if (virNWFilterIPAddrMapAddIPAddr(req->ifname, inetaddr) < 0) { + if (virNWFilterIPAddrMapAddIPAddr(req->binding->portdevname, inetaddr) < 0) { VIR_ERROR(_("Failed to add IP address %s to IP address " - "cache for interface %s"), inetaddr, req->ifname); + "cache for interface %s"), inetaddr, req->binding->portdevname); } ret = virNWFilterInstantiateFilterLate(req->driver, - &binding, + req->binding, req->ifindex); VIR_DEBUG("Result from applying firewall rules on " - "%s with IP addr %s : %d", req->ifname, inetaddr, ret); + "%s with IP addr %s : %d", req->binding->portdevname, inetaddr, ret); VIR_FREE(inetaddr); } } else { @@ -698,13 +685,13 @@ learnIPAddressThread(void *arg) virReportSystemError(req->status, _("encountered an error on interface %s " "index %d"), - req->ifname, req->ifindex); + req->binding->portdevname, req->ifindex); - techdriver->applyDropAllRules(req->ifname); - virNWFilterUnlockIface(req->ifname); + techdriver->applyDropAllRules(req->binding->portdevname); + virNWFilterUnlockIface(req->binding->portdevname); } - VIR_DEBUG("pcap thread terminating for interface %s", req->ifname); + VIR_DEBUG("pcap thread terminating for interface %s", req->binding->portdevname); err_no_lock: @@ -737,19 +724,14 @@ learnIPAddressThread(void *arg) */ int virNWFilterLearnIPAddress(virNWFilterTechDriverPtr techdriver, - const char *ifname, + virNWFilterBindingDefPtr binding, int ifindex, - const char *linkdev, - const virMacAddr *macaddr, - const char *filtername, - virHashTablePtr filterparams, virNWFilterDriverStatePtr driver, int howDetect) { int rc; virThread thread; virNWFilterIPAddrLearnReqPtr req = NULL; - virHashTablePtr ht = NULL; if (howDetect == 0) return -1; @@ -765,37 +747,11 @@ virNWFilterLearnIPAddress(virNWFilterTechDriverPtr techdriver, if (VIR_ALLOC(req) < 0) goto err_no_req; - ht = virNWFilterHashTableCreate(0); - if (ht == NULL) + if (!(req->binding = virNWFilterBindingDefCopy(binding))) goto err_free_req; - if (virNWFilterHashTablePutAll(filterparams, ht) < 0) - goto err_free_ht; - - if (VIR_STRDUP(req->filtername, filtername) < 0) - goto err_free_ht; - - if (virStrcpyStatic(req->ifname, ifname) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Destination buffer for ifname ('%s') " - "not large enough"), ifname); - goto err_free_ht; - } - - if (linkdev) { - if (virStrcpyStatic(req->linkdev, linkdev) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Destination buffer for linkdev ('%s') " - "not large enough"), linkdev); - goto err_free_ht; - } - } - req->ifindex = ifindex; - virMacAddrSet(&req->macaddr, macaddr); req->driver = driver; - req->filterparams = ht; - ht = NULL; req->howDetect = howDetect; req->techdriver = techdriver; @@ -814,8 +770,6 @@ virNWFilterLearnIPAddress(virNWFilterTechDriverPtr techdriver, err_dereg_req: virNWFilterDeregisterLearnReq(ifindex); - err_free_ht: - virHashFree(ht); err_free_req: virNWFilterIPAddrLearnReqFree(req); err_no_req: @@ -826,12 +780,8 @@ virNWFilterLearnIPAddress(virNWFilterTechDriverPtr techdriver, int virNWFilterLearnIPAddress(virNWFilterTechDriverPtr techdriver ATTRIBUTE_UNUSED, - const char *ifname ATTRIBUTE_UNUSED, + virNWFilterBindingDefPtr binding ATTRIBUTE_UNUSED, int ifindex ATTRIBUTE_UNUSED, - const char *linkdev ATTRIBUTE_UNUSED, - const virMacAddr *macaddr ATTRIBUTE_UNUSED, - const char *filtername ATTRIBUTE_UNUSED, - virHashTablePtr filterparams ATTRIBUTE_UNUSED, virNWFilterDriverStatePtr driver ATTRIBUTE_UNUSED, int howDetect ATTRIBUTE_UNUSED) { diff --git a/src/nwfilter/nwfilter_learnipaddr.h b/src/nwfilter/nwfilter_learnipaddr.h index 753aabc594..7f17244100 100644 --- a/src/nwfilter/nwfilter_learnipaddr.h +++ b/src/nwfilter/nwfilter_learnipaddr.h @@ -28,6 +28,7 @@ # include "conf/nwfilter_params.h" # include "nwfilter_tech_driver.h" +# include "virnwfilterbindingdef.h" # include <net/if.h> enum howDetect { @@ -36,12 +37,8 @@ enum howDetect { }; int virNWFilterLearnIPAddress(virNWFilterTechDriverPtr techdriver, - const char *ifname, + virNWFilterBindingDefPtr binding, int ifindex, - const char *linkdev, - const virMacAddr *macaddr, - const char *filtername, - virHashTablePtr filterparams, virNWFilterDriverStatePtr driver, int howDetect); -- 2.17.0

On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
Use the virNWFilterBindingDefPTr struct in the IP address learning code directly.
Reviewed-by: John Ferlan <jferlan@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/nwfilter/nwfilter_gentech_driver.c | 7 +- src/nwfilter/nwfilter_learnipaddr.c | 106 +++++++------------------ src/nwfilter/nwfilter_learnipaddr.h | 7 +- 3 files changed, 32 insertions(+), 88 deletions(-)
R-By still applies, but please let's...
--- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c
[...]
@@ -737,19 +724,14 @@ learnIPAddressThread(void *arg) */
Adjust the comments above here to replace the ifname, linkdev, macaddr, filtername, and filterparams with @binding
int virNWFilterLearnIPAddress(virNWFilterTechDriverPtr techdriver, - const char *ifname, + virNWFilterBindingDefPtr binding, int ifindex, - const char *linkdev, - const virMacAddr *macaddr, - const char *filtername, - virHashTablePtr filterparams, virNWFilterDriverStatePtr driver, int howDetect)
[...]

On Sun, Jun 17, 2018 at 08:28:11AM -0400, John Ferlan wrote:
On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
Use the virNWFilterBindingDefPTr struct in the IP address learning code directly.
Reviewed-by: John Ferlan <jferlan@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/nwfilter/nwfilter_gentech_driver.c | 7 +- src/nwfilter/nwfilter_learnipaddr.c | 106 +++++++------------------ src/nwfilter/nwfilter_learnipaddr.h | 7 +- 3 files changed, 32 insertions(+), 88 deletions(-)
R-By still applies, but please let's...
--- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c
[...]
@@ -737,19 +724,14 @@ learnIPAddressThread(void *arg) */
Adjust the comments above here to replace the ifname, linkdev, macaddr, filtername, and filterparams with @binding
Ok will do.
int virNWFilterLearnIPAddress(virNWFilterTechDriverPtr techdriver, - const char *ifname, + virNWFilterBindingDefPtr binding, int ifindex, - const char *linkdev, - const virMacAddr *macaddr, - const char *filtername, - virHashTablePtr filterparams, virNWFilterDriverStatePtr driver, int howDetect)
[...]
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Use the virNWFilterBindingDefPtr struct in the DHCP address snooping code directly. Reviewed-by: John Ferlan <jferlan@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/nwfilter/nwfilter_dhcpsnoop.c | 161 +++++++++---------------- src/nwfilter/nwfilter_dhcpsnoop.h | 7 +- src/nwfilter/nwfilter_gentech_driver.c | 7 +- 3 files changed, 62 insertions(+), 113 deletions(-) diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index 7274b03c2a..533c45f080 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -135,13 +135,9 @@ struct _virNWFilterSnoopReq { int refctr; virNWFilterTechDriverPtr techdriver; - char *ifname; + virNWFilterBindingDefPtr binding; int ifindex; - char *linkdev; char ifkey[VIR_IFKEY_LEN]; - virMacAddr macaddr; - char *filtername; - virHashTablePtr vars; virNWFilterDriverStatePtr driver; /* start and end of lease list, ordered by lease time */ virNWFilterSnoopIPLeasePtr start; @@ -484,10 +480,10 @@ virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLeasePtr ipl, req = ipl->snoopReq; - /* protect req->ifname */ + /* protect req->binding->portdevname */ virNWFilterSnoopReqLock(req); - if (virNWFilterIPAddrMapAddIPAddr(req->ifname, ipaddr) < 0) + if (virNWFilterIPAddrMapAddIPAddr(req->binding->portdevname, ipaddr) < 0) goto exit_snooprequnlock; if (!instantiate) { @@ -497,18 +493,9 @@ virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLeasePtr ipl, /* instantiate the filters */ - if (req->ifname) { - virNWFilterBindingDef binding = { - .portdevname = req->ifname, - .linkdevname = req->linkdev, - .mac = req->macaddr, - .filter = req->filtername, - .filterparams = req->vars, - .ownername = NULL, - .owneruuid = {0}, - }; + if (req->binding->portdevname) { rc = virNWFilterInstantiateFilterLate(req->driver, - &binding, + req->binding, req->ifindex); } @@ -649,10 +636,7 @@ virNWFilterSnoopReqFree(virNWFilterSnoopReqPtr req) virNWFilterSnoopReqLeaseDel(req, &ipl->ipAddress, false, false); /* free all req data */ - VIR_FREE(req->ifname); - VIR_FREE(req->linkdev); - VIR_FREE(req->filtername); - virHashFree(req->vars); + virNWFilterBindingDefFree(req->binding); virMutexDestroy(&req->lock); virCondDestroy(&req->threadStatusCond); @@ -883,30 +867,23 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReqPtr req, if (update_leasefile) virNWFilterSnoopLeaseFileSave(ipl); - ipAddrLeft = virNWFilterIPAddrMapDelIPAddr(req->ifname, ipstr); + ipAddrLeft = virNWFilterIPAddrMapDelIPAddr(req->binding->portdevname, ipstr); if (!req->threadkey || !instantiate) goto skip_instantiate; if (ipAddrLeft) { - virNWFilterBindingDef binding = { - .portdevname = req->ifname, - .linkdevname = req->linkdev, - .mac = req->macaddr, - .filter = req->filtername, - .filterparams = req->vars, - .ownername = NULL, - .owneruuid = {0}, - }; ret = virNWFilterInstantiateFilterLate(req->driver, - &binding, + req->binding, req->ifindex); } else { virNWFilterVarValuePtr dhcpsrvrs = - virHashLookup(req->vars, NWFILTER_VARNAME_DHCPSERVER); + virHashLookup(req->binding->filterparams, + NWFILTER_VARNAME_DHCPSERVER); if (req->techdriver && - req->techdriver->applyDHCPOnlyRules(req->ifname, &req->macaddr, + req->techdriver->applyDHCPOnlyRules(req->binding->portdevname, + &req->binding->mac, dhcpsrvrs, false) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("virNWFilterSnoopListDel failed")); @@ -1036,7 +1013,7 @@ virNWFilterSnoopDHCPDecode(virNWFilterSnoopReqPtr req, * inside the DHCP response */ if (!fromVM) { - if (virMacAddrCmpRaw(&req->macaddr, + if (virMacAddrCmpRaw(&req->binding->mac, (unsigned char *)&pd->d_chaddr) != 0) return -2; } @@ -1198,7 +1175,7 @@ static void virNWFilterDHCPDecodeWorker(void *jobdata, void *opaque) virReportError(VIR_ERR_INTERNAL_ERROR, _("Instantiation of rules failed on " - "interface '%s'"), req->ifname); + "interface '%s'"), req->binding->portdevname); } virAtomicIntDecAndTest(job->qCtr); VIR_FREE(job); @@ -1407,13 +1384,14 @@ virNWFilterDHCPSnoopThread(void *req0) /* whoever started us increased the reference counter for the req for us */ - /* protect req->ifname & req->threadkey */ + /* protect req->binding->portdevname & req->threadkey */ virNWFilterSnoopReqLock(req); - if (req->ifname && req->threadkey) { + if (req->binding->portdevname && req->threadkey) { for (i = 0; i < ARRAY_CARDINALITY(pcapConf); i++) { pcapConf[i].handle = - virNWFilterSnoopDHCPOpen(req->ifname, &req->macaddr, + virNWFilterSnoopDHCPOpen(req->binding->portdevname, + &req->binding->mac, pcapConf[i].filter, pcapConf[i].dir); if (!pcapConf[i].handle) { @@ -1422,7 +1400,7 @@ virNWFilterDHCPSnoopThread(void *req0) } fds[i].fd = pcap_fileno(pcapConf[i].handle); } - tmp = virNetDevGetIndex(req->ifname, &ifindex); + tmp = virNetDevGetIndex(req->binding->portdevname, &ifindex); ignore_value(VIR_STRDUP(threadkey, req->threadkey)); worker = virThreadPoolNew(1, 1, 0, virNWFilterDHCPDecodeWorker, @@ -1487,11 +1465,11 @@ virNWFilterDHCPSnoopThread(void *req0) /* error reading from socket */ tmp = -1; - /* protect req->ifname */ + /* protect req->binding->portdevname */ virNWFilterSnoopReqLock(req); - if (req->ifname) - tmp = virNetDevValidateConfig(req->ifname, NULL, ifindex); + if (req->binding->portdevname) + tmp = virNetDevValidateConfig(req->binding->portdevname, NULL, ifindex); virNWFilterSnoopReqUnlock(req); @@ -1504,16 +1482,17 @@ virNWFilterDHCPSnoopThread(void *req0) pcap_close(pcapConf[i].handle); pcapConf[i].handle = NULL; - /* protect req->ifname */ + /* protect req->binding->portdevname */ virNWFilterSnoopReqLock(req); virReportError(VIR_ERR_INTERNAL_ERROR, _("interface '%s' failing; " "reopening"), - req->ifname); - if (req->ifname) + req->binding->portdevname); + if (req->binding->portdevname) pcapConf[i].handle = - virNWFilterSnoopDHCPOpen(req->ifname, &req->macaddr, + virNWFilterSnoopDHCPOpen(req->binding->portdevname, + &req->binding->mac, pcapConf[i].filter, pcapConf[i].dir); @@ -1539,7 +1518,7 @@ virNWFilterDHCPSnoopThread(void *req0) last_displayed_queue = time(0); VIR_WARN("Worker thread for interface '%s' has a " "job queue that is too long", - req->ifname); + req->binding->portdevname); } continue; } @@ -1552,7 +1531,7 @@ virNWFilterDHCPSnoopThread(void *req0) if (time(0) - last_displayed > 10) { last_displayed = time(0); VIR_WARN("Too many DHCP packets on interface '%s'", - req->ifname); + req->binding->portdevname); } continue; } @@ -1563,7 +1542,7 @@ virNWFilterDHCPSnoopThread(void *req0) &pcapConf[i].qCtr) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Job submission failed on " - "interface '%s'"), req->ifname); + "interface '%s'"), req->binding->portdevname); error = true; break; } @@ -1574,15 +1553,15 @@ virNWFilterDHCPSnoopThread(void *req0) /* protect IfNameToKey */ virNWFilterSnoopLock(); - /* protect req->ifname & req->threadkey */ + /* protect req->binding->portdevname & req->threadkey */ virNWFilterSnoopReqLock(req); virNWFilterSnoopCancel(&req->threadkey); ignore_value(virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey, - req->ifname)); + req->binding->portdevname)); - VIR_FREE(req->ifname); + VIR_FREE(req->binding->portdevname); virNWFilterSnoopReqUnlock(req); virNWFilterSnoopUnlock(); @@ -1615,12 +1594,7 @@ virNWFilterSnoopIFKeyFMT(char *ifkey, const unsigned char *vmuuid, int virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver, - const char *ifname, - const char *linkdev, - const unsigned char *vmuuid, - const virMacAddr *macaddr, - const char *filtername, - virHashTablePtr filterparams, + virNWFilterBindingDefPtr binding, virNWFilterDriverStatePtr driver) { virNWFilterSnoopReqPtr req; @@ -1631,7 +1605,7 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver, virNWFilterVarValuePtr dhcpsrvrs; bool threadPuts = false; - virNWFilterSnoopIFKeyFMT(ifkey, vmuuid, macaddr); + virNWFilterSnoopIFKeyFMT(ifkey, binding->owneruuid, &binding->mac); req = virNWFilterSnoopReqGetByIFKey(ifkey); isnewreq = (req == NULL); @@ -1640,9 +1614,8 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver, virNWFilterSnoopReqPut(req); return 0; } - /* a recycled req may still have filtername and vars */ - VIR_FREE(req->filtername); - virHashFree(req->vars); + virNWFilterBindingDefFree(req->binding); + req->binding = NULL; } else { req = virNWFilterSnoopReqNew(ifkey); if (!req) @@ -1651,17 +1624,9 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver, req->driver = driver; req->techdriver = techdriver; - tmp = virNetDevGetIndex(ifname, &req->ifindex); - virMacAddrSet(&req->macaddr, macaddr); - req->vars = virNWFilterHashTableCreate(0); - req->linkdev = NULL; - - if (VIR_STRDUP(req->ifname, ifname) < 0 || - VIR_STRDUP(req->filtername, filtername) < 0 || - VIR_STRDUP(req->linkdev, linkdev) < 0) + if ((tmp = virNetDevGetIndex(binding->portdevname, &req->ifindex)) < 0) goto exit_snoopreqput; - - if (!req->vars || tmp < 0) + if (!(req->binding = virNWFilterBindingDefCopy(binding))) goto exit_snoopreqput; /* check that all tools are available for applying the filters (late) */ @@ -1673,10 +1638,11 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver, goto exit_snoopreqput; } - dhcpsrvrs = virHashLookup(filterparams, + dhcpsrvrs = virHashLookup(binding->filterparams, NWFILTER_VARNAME_DHCPSERVER); - if (techdriver->applyDHCPOnlyRules(req->ifname, &req->macaddr, + if (techdriver->applyDHCPOnlyRules(req->binding->portdevname, + &req->binding->mac, dhcpsrvrs, false) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("applyDHCPOnlyRules " @@ -1684,20 +1650,14 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver, goto exit_snoopreqput; } - if (virNWFilterHashTablePutAll(filterparams, req->vars) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("virNWFilterDHCPSnoopReq: can't copy variables" - " on if %s"), ifkey); - goto exit_snoopreqput; - } - virNWFilterSnoopLock(); - if (virHashAddEntry(virNWFilterSnoopState.ifnameToKey, ifname, + if (virHashAddEntry(virNWFilterSnoopState.ifnameToKey, + req->binding->portdevname, req->ifkey) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("virNWFilterDHCPSnoopReq ifname map failed" - " on interface \"%s\" key \"%s\""), ifname, + " on interface \"%s\" key \"%s\""), binding->portdevname, ifkey); goto exit_snoopunlock; } @@ -1706,7 +1666,7 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver, virHashAddEntry(virNWFilterSnoopState.snoopReqs, ifkey, req) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("virNWFilterDHCPSnoopReq req add failed on" - " interface \"%s\" ifkey \"%s\""), ifname, + " interface \"%s\" ifkey \"%s\""), binding->portdevname, ifkey); goto exit_rem_ifnametokey; } @@ -1718,7 +1678,7 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver, req) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("virNWFilterDHCPSnoopReq virThreadCreate " - "failed on interface '%s'"), ifname); + "failed on interface '%s'"), binding->portdevname); goto exit_snoopreq_unlock; } @@ -1730,14 +1690,14 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver, if (!req->threadkey) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Activation of snoop request failed on " - "interface '%s'"), req->ifname); + "interface '%s'"), req->binding->portdevname); goto exit_snoopreq_unlock; } if (virNWFilterSnoopReqRestore(req) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Restoring of leases failed on " - "interface '%s'"), req->ifname); + "interface '%s'"), req->binding->portdevname); goto exit_snoop_cancel; } @@ -1766,7 +1726,7 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver, exit_snoopreq_unlock: virNWFilterSnoopReqUnlock(req); exit_rem_ifnametokey: - virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey, ifname); + virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey, binding->portdevname); exit_snoopunlock: virNWFilterSnoopUnlock(); exit_snoopreqput: @@ -2074,21 +2034,21 @@ virNWFilterSnoopRemAllReqIter(const void *payload, { virNWFilterSnoopReqPtr req = (virNWFilterSnoopReqPtr)payload; - /* protect req->ifname */ + /* protect req->binding->portdevname */ virNWFilterSnoopReqLock(req); - if (req->ifname) { + if (req->binding->portdevname) { ignore_value(virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey, - req->ifname)); + req->binding->portdevname)); /* * Remove all IP addresses known to be associated with this * interface so that a new thread will be started on this * interface */ - virNWFilterIPAddrMapDelIPAddr(req->ifname, NULL); + virNWFilterIPAddrMapDelIPAddr(req->binding->portdevname, NULL); - VIR_FREE(req->ifname); + VIR_FREE(req->binding->portdevname); } virNWFilterSnoopReqUnlock(req); @@ -2191,13 +2151,13 @@ virNWFilterDHCPSnoopEnd(const char *ifname) goto cleanup; } - /* protect req->ifname & req->threadkey */ + /* protect req->binding->portdevname & req->threadkey */ virNWFilterSnoopReqLock(req); /* keep valid lease req; drop interface association */ virNWFilterSnoopCancel(&req->threadkey); - VIR_FREE(req->ifname); + VIR_FREE(req->binding->portdevname); virNWFilterSnoopReqUnlock(req); @@ -2259,12 +2219,7 @@ virNWFilterDHCPSnoopShutdown(void) int virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver ATTRIBUTE_UNUSED, - const char *ifname ATTRIBUTE_UNUSED, - const char *linkdev ATTRIBUTE_UNUSED, - const unsigned char *vmuuid ATTRIBUTE_UNUSED, - const virMacAddr *macaddr ATTRIBUTE_UNUSED, - const char *filtername ATTRIBUTE_UNUSED, - virHashTablePtr filterparams ATTRIBUTE_UNUSED, + virNWFilterBindingDefPtr binding ATTRIBUTE_UNUSED, virNWFilterDriverStatePtr driver ATTRIBUTE_UNUSED) { virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/nwfilter/nwfilter_dhcpsnoop.h b/src/nwfilter/nwfilter_dhcpsnoop.h index a5925de40a..c693e1adbd 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.h +++ b/src/nwfilter/nwfilter_dhcpsnoop.h @@ -30,12 +30,7 @@ int virNWFilterDHCPSnoopInit(void); void virNWFilterDHCPSnoopShutdown(void); int virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver, - const char *ifname, - const char *linkdev, - const unsigned char *vmuuid, - const virMacAddr *macaddr, - const char *filtername, - virHashTablePtr filterparams, + virNWFilterBindingDefPtr binding, virNWFilterDriverStatePtr driver); void virNWFilterDHCPSnoopEnd(const char *ifname); #endif /* __NWFILTER_DHCPSNOOP_H */ diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index ce45587a44..4b55bd6ca4 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -618,10 +618,9 @@ virNWFilterDoInstantiate(virNWFilterTechDriverPtr techdriver, goto err_unresolvable_vars; } if (STRCASEEQ(learning, "dhcp")) { - rc = virNWFilterDHCPSnoopReq(techdriver, binding->portdevname, - binding->linkdevname, - binding->owneruuid, &binding->mac, - filter->name, binding->filterparams, driver); + rc = virNWFilterDHCPSnoopReq(techdriver, + binding, + driver); goto err_exit; } else if (STRCASEEQ(learning, "any")) { if (!virNWFilterHasLearnReq(ifindex)) { -- 2.17.0

If a <interface> includes a filter name but the nwfilter driver is not present we silently do nothing. This is very bad, because an application that thinks it is protected by malicious guest traffic will in fact be vulnerable. Reporting an error gives the administrator the ability to know there is a problem and fix it. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/domain_nwfilter.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_nwfilter.c b/src/conf/domain_nwfilter.c index e360aceeba..7570e0ae83 100644 --- a/src/conf/domain_nwfilter.c +++ b/src/conf/domain_nwfilter.c @@ -28,6 +28,9 @@ #include "datatypes.h" #include "domain_conf.h" #include "domain_nwfilter.h" +#include "virerror.h" + +#define VIR_FROM_THIS VIR_FROM_NWFILTER static virDomainConfNWFilterDriverPtr nwfilterDriver; @@ -44,8 +47,10 @@ virDomainConfNWFilterInstantiate(const char *vmname, { if (nwfilterDriver != NULL) return nwfilterDriver->instantiateFilter(vmname, vmuuid, net); - /* driver module not available -- don't indicate failure */ - return 0; + + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("No network filter driver available")); + return -1; } void -- 2.17.0

$SUBJ: s/by/but On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
If a <interface> includes a filter name but the nwfilter driver is not present we silently do nothing. This is very bad, because an application that thinks it is protected by malicious guest traffic will in fact be vulnerable. Reporting an error gives the administrator the ability to know there is a problem and fix it.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/domain_nwfilter.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John However/But/FWIW: I know this method changes again in patch 20; however, it's easier to describe now... All callers except 1 will only call here if net->filter is true. The one outlier is the error path in qemuDomainChangeNetFilter which will call with olddev before checking if olddev->filter was true. Looking at qemuDomainChangeNet the STRNEQ_NULLABLE is used on olddev->filter and newdev->filter, so I think we could currently end up in a fairly strange place - although there will be errors eventually from virNWFilterInstantiateFilterUpdate when @filtername is not found. I'm tempted to suggest moving the net->filter check into here, but I know that by patch 20 all that changes again. It's fine to do things later, but figured while it was fresh in my mind - I'd note it.
diff --git a/src/conf/domain_nwfilter.c b/src/conf/domain_nwfilter.c index e360aceeba..7570e0ae83 100644 --- a/src/conf/domain_nwfilter.c +++ b/src/conf/domain_nwfilter.c @@ -28,6 +28,9 @@ #include "datatypes.h" #include "domain_conf.h" #include "domain_nwfilter.h" +#include "virerror.h" + +#define VIR_FROM_THIS VIR_FROM_NWFILTER
static virDomainConfNWFilterDriverPtr nwfilterDriver;
@@ -44,8 +47,10 @@ virDomainConfNWFilterInstantiate(const char *vmname, { if (nwfilterDriver != NULL) return nwfilterDriver->instantiateFilter(vmname, vmuuid, net); - /* driver module not available -- don't indicate failure */ - return 0; + + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("No network filter driver available")); + return -1; }
void

Introduce a new struct to act as the stateful owner of the virNWFilterBindingDefPtr objects. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/Makefile.inc.am | 2 + src/conf/virnwfilterbindingobj.c | 299 +++++++++++++++++++++++++++++++ src/conf/virnwfilterbindingobj.h | 69 +++++++ src/libvirt_private.syms | 14 ++ 4 files changed, 384 insertions(+) create mode 100644 src/conf/virnwfilterbindingobj.c create mode 100644 src/conf/virnwfilterbindingobj.h diff --git a/src/conf/Makefile.inc.am b/src/conf/Makefile.inc.am index f5fb323233..3d55ba688d 100644 --- a/src/conf/Makefile.inc.am +++ b/src/conf/Makefile.inc.am @@ -87,6 +87,8 @@ NWFILTER_CONF_SOURCES = \ conf/virnwfilterobj.h \ conf/virnwfilterbindingdef.c \ conf/virnwfilterbindingdef.h \ + conf/virnwfilterbindingobj.c \ + conf/virnwfilterbindingobj.h \ $(NULL) STORAGE_CONF_SOURCES = \ diff --git a/src/conf/virnwfilterbindingobj.c b/src/conf/virnwfilterbindingobj.c new file mode 100644 index 0000000000..b6c05daf09 --- /dev/null +++ b/src/conf/virnwfilterbindingobj.c @@ -0,0 +1,299 @@ +/* + * virnwfilterbindingobj.c: network filter binding object processing + * + * Copyright (C) 2018 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#include "viralloc.h" +#include "virerror.h" +#include "virstring.h" +#include "nwfilter_params.h" +#include "virnwfilterbindingobj.h" +#include "viruuid.h" +#include "virfile.h" + + +#define VIR_FROM_THIS VIR_FROM_NWFILTER + +struct _virNWFilterBindingObj { + virObjectLockable parent; + + bool removing; + virNWFilterBindingDefPtr def; +}; + + +static virClassPtr virNWFilterBindingObjClass; +static void virNWFilterBindingObjDispose(void *obj); + +static int +virNWFilterBindingObjOnceInit(void) +{ + if (!VIR_CLASS_NEW(virNWFilterBindingObj, virClassForObjectLockable())) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virNWFilterBindingObj) + +virNWFilterBindingObjPtr +virNWFilterBindingObjNew(void) +{ + if (virNWFilterBindingObjInitialize() < 0) + return NULL; + + return virObjectNew(virNWFilterBindingObjClass); +} + + +static void +virNWFilterBindingObjDispose(void *obj) +{ + virNWFilterBindingObjPtr bobj = obj; + + virNWFilterBindingDefFree(bobj->def); +} + + +virNWFilterBindingDefPtr +virNWFilterBindingObjGetDef(virNWFilterBindingObjPtr obj) +{ + return obj->def; +} + + +void +virNWFilterBindingObjSetDef(virNWFilterBindingObjPtr obj, + virNWFilterBindingDefPtr def) +{ + virNWFilterBindingDefFree(obj->def); + obj->def = def; +} + + +bool +virNWFilterBindingObjGetRemoving(virNWFilterBindingObjPtr obj) +{ + return obj->removing; +} + + +void +virNWFilterBindingObjSetRemoving(virNWFilterBindingObjPtr obj, + bool removing) +{ + obj->removing = removing; +} + + +/** + * virNWFilterBindingObjEndAPI: + * @obj: binding object + * + * Finish working with a binding object in an API. This function + * clears whatever was left of a domain that was gathered using + * virNWFilterBindingObjListFindByPortDev(). Currently that means + * only unlocking and decrementing the reference counter of that + * object. And in order to make sure the caller does not access + * the object, the pointer is cleared. + */ +void +virNWFilterBindingObjEndAPI(virNWFilterBindingObjPtr *obj) +{ + if (!*obj) + return; + + virObjectUnlock(*obj); + virObjectUnref(*obj); + *obj = NULL; +} + + +char * +virNWFilterBindingObjConfigFile(const char *dir, + const char *name) +{ + char *ret; + + ignore_value(virAsprintf(&ret, "%s/%s.xml", dir, name)); + return ret; +} + + +int +virNWFilterBindingObjSave(const virNWFilterBindingObj *obj, + const char *statusDir) +{ + char *filename; + char *xml = NULL; + int ret = -1; + + if (!(filename = virNWFilterBindingObjConfigFile(statusDir, + obj->def->portdevname))) + return -1; + + if (!(xml = virNWFilterBindingObjFormat(obj))) + goto cleanup; + + if (virFileMakePath(statusDir) < 0) { + virReportSystemError(errno, + _("cannot create config directory '%s'"), + statusDir); + goto cleanup; + } + + ret = virXMLSaveFile(filename, + obj->def->portdevname, "nwfilter-binding-create", + xml); + + cleanup: + VIR_FREE(xml); + VIR_FREE(filename); + return ret; +} + + +int +virNWFilterBindingObjDelete(const virNWFilterBindingObj *obj, + const char *statusDir) +{ + char *filename; + int ret = -1; + + if (!(filename = virNWFilterBindingObjConfigFile(statusDir, + obj->def->portdevname))) + return -1; + + if (unlink(filename) < 0 && + errno != ENOENT) { + virReportSystemError(errno, + _("Unable to remove status '%s' for nwfilter binding %s'"), + filename, obj->def->portdevname); + goto cleanup; + } + + ret = 0; + + cleanup: + VIR_FREE(filename); + return ret; +} + + +static virNWFilterBindingObjPtr +virNWFilterBindingObjParseXML(xmlDocPtr doc, + xmlXPathContextPtr ctxt) +{ + virNWFilterBindingObjPtr ret; + xmlNodePtr node; + + if (!(ret = virNWFilterBindingObjNew())) + return NULL; + + if (!(node = virXPathNode("./filterbinding", ctxt))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("filter binding status missing content")); + goto cleanup; + } + + if (!(ret->def = virNWFilterBindingDefParseNode(doc, node))) + goto cleanup; + + return ret; + + cleanup: + virObjectUnref(ret); + return NULL; +} + + +static virNWFilterBindingObjPtr +virNWFilterBindingObjParseNode(xmlDocPtr doc, + xmlNodePtr root) +{ + xmlXPathContextPtr ctxt = NULL; + virNWFilterBindingObjPtr obj = NULL; + + if (STRNEQ((const char *)root->name, "filterbinding")) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("unknown root element for filter binding")); + goto cleanup; + } + + ctxt = xmlXPathNewContext(doc); + if (ctxt == NULL) { + virReportOOMError(); + goto cleanup; + } + + ctxt->node = root; + obj = virNWFilterBindingObjParseXML(doc, ctxt); + + cleanup: + xmlXPathFreeContext(ctxt); + return obj; +} + + +static virNWFilterBindingObjPtr +virNWFilterBindingObjParse(const char *xmlStr, + const char *filename) +{ + virNWFilterBindingObjPtr obj = NULL; + xmlDocPtr xml; + + if ((xml = virXMLParse(filename, xmlStr, _("(nwfilterbinding_status)")))) { + obj = virNWFilterBindingObjParseNode(xml, xmlDocGetRootElement(xml)); + xmlFreeDoc(xml); + } + + return obj; +} + + +virNWFilterBindingObjPtr +virNWFilterBindingObjParseFile(const char *filename) +{ + return virNWFilterBindingObjParse(NULL, filename); +} + + +char * +virNWFilterBindingObjFormat(const virNWFilterBindingObj *obj) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&buf, "<filterbindingstatus>\n"); + + virBufferAdjustIndent(&buf, 2); + + if (virNWFilterBindingDefFormatBuf(&buf, obj->def) < 0) { + virBufferFreeAndReset(&buf); + return NULL; + } + + virBufferAdjustIndent(&buf, -2); + virBufferAddLit(&buf, "</filterbindingstatus>\n"); + + if (virBufferCheckError(&buf) < 0) + return NULL; + + return virBufferContentAndReset(&buf); +} diff --git a/src/conf/virnwfilterbindingobj.h b/src/conf/virnwfilterbindingobj.h new file mode 100644 index 0000000000..21ae85b064 --- /dev/null +++ b/src/conf/virnwfilterbindingobj.h @@ -0,0 +1,69 @@ +/* + * virnwfilterbindingobj.h: network filter binding object processing + * + * Copyright (C) 2018 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ +#ifndef VIR_NWFILTER_BINDING_OBJ_H +# define VIR_NWFILTER_BINDING_OBJ_H + +# include "internal.h" +# include "virnwfilterbindingdef.h" +# include "virobject.h" + +typedef struct _virNWFilterBindingObj virNWFilterBindingObj; +typedef virNWFilterBindingObj *virNWFilterBindingObjPtr; + +virNWFilterBindingObjPtr +virNWFilterBindingObjNew(void); + +virNWFilterBindingDefPtr +virNWFilterBindingObjGetDef(virNWFilterBindingObjPtr obj); + +void +virNWFilterBindingObjSetDef(virNWFilterBindingObjPtr obj, + virNWFilterBindingDefPtr def); + +bool +virNWFilterBindingObjGetRemoving(virNWFilterBindingObjPtr obj); + +void +virNWFilterBindingObjSetRemoving(virNWFilterBindingObjPtr obj, + bool removing); + +void +virNWFilterBindingObjEndAPI(virNWFilterBindingObjPtr *obj); + +char * +virNWFilterBindingObjConfigFile(const char *dir, + const char *name); + +int +virNWFilterBindingObjSave(const virNWFilterBindingObj *obj, + const char *statusDir); + +int +virNWFilterBindingObjDelete(const virNWFilterBindingObj *obj, + const char *statusDir); + +virNWFilterBindingObjPtr +virNWFilterBindingObjParseFile(const char *filename); + +char * +virNWFilterBindingObjFormat(const virNWFilterBindingObj *obj); + +#endif /* VIR_NWFILTER_BINDING_OBJ_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a323316607..4ad2116238 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1058,6 +1058,20 @@ virNWFilterBindingDefParseNode; virNWFilterBindingDefParseString; +# conf/virnwfilterbindingobj.h +virNWFilterBindingObjConfigFile; +virNWFilterBindingObjDelete; +virNWFilterBindingObjEndAPI; +virNWFilterBindingObjFormat; +virNWFilterBindingObjGetDef; +virNWFilterBindingObjGetRemoving; +virNWFilterBindingObjNew; +virNWFilterBindingObjParseFile; +virNWFilterBindingObjSave; +virNWFilterBindingObjSetDef; +virNWFilterBindingObjSetRemoving; + + # conf/virnwfilterobj.h virNWFilterObjGetDef; virNWFilterObjGetNewDef; -- 2.17.0

On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
Introduce a new struct to act as the stateful owner of the virNWFilterBindingDefPtr objects.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/Makefile.inc.am | 2 + src/conf/virnwfilterbindingobj.c | 299 +++++++++++++++++++++++++++++++ src/conf/virnwfilterbindingobj.h | 69 +++++++ src/libvirt_private.syms | 14 ++ 4 files changed, 384 insertions(+) create mode 100644 src/conf/virnwfilterbindingobj.c create mode 100644 src/conf/virnwfilterbindingobj.h
Reviewed-by: John Ferlan <jferlan@redhat.com> John

On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
Introduce a new struct to act as the stateful owner of the virNWFilterBindingDefPtr objects.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/Makefile.inc.am | 2 + src/conf/virnwfilterbindingobj.c | 299 +++++++++++++++++++++++++++++++ src/conf/virnwfilterbindingobj.h | 69 +++++++ src/libvirt_private.syms | 14 ++ 4 files changed, 384 insertions(+) create mode 100644 src/conf/virnwfilterbindingobj.c create mode 100644 src/conf/virnwfilterbindingobj.h
While continuing, I tripped across this:
+ +static virNWFilterBindingObjPtr +virNWFilterBindingObjParseNode(xmlDocPtr doc, + xmlNodePtr root) +{ + xmlXPathContextPtr ctxt = NULL; + virNWFilterBindingObjPtr obj = NULL; + + if (STRNEQ((const char *)root->name, "filterbinding")) {
"filterbindingstatus"
+ virReportError(VIR_ERR_XML_ERROR, "%s", + _("unknown root element for filter binding"));
Found by adding the '%s' here to print the root->name...
+ goto cleanup; + } + + ctxt = xmlXPathNewContext(doc); + if (ctxt == NULL) { + virReportOOMError(); + goto cleanup; + } + + ctxt->node = root; + obj = virNWFilterBindingObjParseXML(doc, ctxt); + + cleanup: + xmlXPathFreeContext(ctxt); + return obj; +} + +
John

On Mon, Jun 18, 2018 at 10:08:00AM -0400, John Ferlan wrote:
On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
Introduce a new struct to act as the stateful owner of the virNWFilterBindingDefPtr objects.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/Makefile.inc.am | 2 + src/conf/virnwfilterbindingobj.c | 299 +++++++++++++++++++++++++++++++ src/conf/virnwfilterbindingobj.h | 69 +++++++ src/libvirt_private.syms | 14 ++ 4 files changed, 384 insertions(+) create mode 100644 src/conf/virnwfilterbindingobj.c create mode 100644 src/conf/virnwfilterbindingobj.h
While continuing, I tripped across this:
+ +static virNWFilterBindingObjPtr +virNWFilterBindingObjParseNode(xmlDocPtr doc, + xmlNodePtr root) +{ + xmlXPathContextPtr ctxt = NULL; + virNWFilterBindingObjPtr obj = NULL; + + if (STRNEQ((const char *)root->name, "filterbinding")) {
"filterbindingstatus"
Oh fun, will fix that. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Introduce a new struct to act as the manager of a collection of virNWFilterBindingObjPtr objects. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/Makefile.inc.am | 2 + src/conf/virnwfilterbindingobjlist.c | 487 +++++++++++++++++++++++++++ src/conf/virnwfilterbindingobjlist.h | 69 ++++ src/libvirt_private.syms | 10 + 4 files changed, 568 insertions(+) create mode 100644 src/conf/virnwfilterbindingobjlist.c create mode 100644 src/conf/virnwfilterbindingobjlist.h diff --git a/src/conf/Makefile.inc.am b/src/conf/Makefile.inc.am index 3d55ba688d..af23810640 100644 --- a/src/conf/Makefile.inc.am +++ b/src/conf/Makefile.inc.am @@ -89,6 +89,8 @@ NWFILTER_CONF_SOURCES = \ conf/virnwfilterbindingdef.h \ conf/virnwfilterbindingobj.c \ conf/virnwfilterbindingobj.h \ + conf/virnwfilterbindingobjlist.c \ + conf/virnwfilterbindingobjlist.h \ $(NULL) STORAGE_CONF_SOURCES = \ diff --git a/src/conf/virnwfilterbindingobjlist.c b/src/conf/virnwfilterbindingobjlist.c new file mode 100644 index 0000000000..7ce59f7c6e --- /dev/null +++ b/src/conf/virnwfilterbindingobjlist.c @@ -0,0 +1,487 @@ +/* + * virnwfilterbindingobjlist.c: nwfilter binding object list utilities + * + * Copyright (C) 2018 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Daniel P. Berrange <berrange@redhat.com> + */ + +#include <config.h> + +#include "internal.h" +#include "datatypes.h" +#include "virnwfilterbindingobjlist.h" +#include "viralloc.h" +#include "virfile.h" +#include "virlog.h" +#include "virstring.h" + +#define VIR_FROM_THIS VIR_FROM_NWFILTER + +VIR_LOG_INIT("conf.virnwfilterbindingobjlist"); + +static virClassPtr virNWFilterBindingObjListClass; +static void virNWFilterBindingObjListDispose(void *obj); + +struct _virNWFilterBindingObjList { + virObjectRWLockable parent; + + /* port dev name -> virNWFilterBindingObj mapping + * for O(1), lockless lookup-by-port dev */ + virHashTable *objs; +}; + + +static int virNWFilterBindingObjListOnceInit(void) +{ + if (!VIR_CLASS_NEW(virNWFilterBindingObjList, virClassForObjectRWLockable())) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virNWFilterBindingObjList) + + +virNWFilterBindingObjListPtr +virNWFilterBindingObjListNew(void) +{ + virNWFilterBindingObjListPtr bindings; + + if (virNWFilterBindingObjListInitialize() < 0) + return NULL; + + if (!(bindings = virObjectRWLockableNew(virNWFilterBindingObjListClass))) + return NULL; + + if (!(bindings->objs = virHashCreate(50, virObjectFreeHashData))) { + virObjectUnref(bindings); + return NULL; + } + + return bindings; +} + + +static void +virNWFilterBindingObjListDispose(void *obj) +{ + virNWFilterBindingObjListPtr bindings = obj; + + virHashFree(bindings->objs); +} + + +static virNWFilterBindingObjPtr +virNWFilterBindingObjListFindByPortDevLocked(virNWFilterBindingObjListPtr bindings, + const char *name) +{ + virNWFilterBindingObjPtr obj; + + obj = virHashLookup(bindings->objs, name); + virObjectRef(obj); + if (obj) { + virObjectLock(obj); + if (virNWFilterBindingObjGetRemoving(obj)) { + virObjectUnlock(obj); + virObjectUnref(obj); + obj = NULL; + } + } + return obj; +} + + +/** + * @bindings: NWFilterBinding object list + * @name: Name to search the bindings->objs table + * + * Lookup the @name in the bindings->objs hash table and return a + * locked and ref counted binding object if found. Caller is expected + * to use the virNWFilterBindingObjEndAPI when done with the object. + */ +virNWFilterBindingObjPtr +virNWFilterBindingObjListFindByPortDev(virNWFilterBindingObjListPtr bindings, + const char *name) +{ + virNWFilterBindingObjPtr obj; + + virObjectRWLockRead(bindings); + obj = virNWFilterBindingObjListFindByPortDevLocked(bindings, name); + virObjectRWUnlock(bindings); + + return obj; +} + + +/** + * @bindings: NWFilterBinding object list pointer + * @binding: NWFilterBinding object to be added + * + * Upon entry @binding should have at least 1 ref and be locked. + * + * Add the @binding into the @bindings->objs hash + * tables. Once successfully added into a table, increase the + * reference count since upon removal in virHashRemoveEntry + * the virObjectUnref will be called since the hash tables were + * configured to call virObjectFreeHashData when the object is + * removed from the hash table. + * + * Returns 0 on success with 2 references and locked + * -1 on failure with 1 reference and locked + */ +static int +virNWFilterBindingObjListAddObjLocked(virNWFilterBindingObjListPtr bindings, + virNWFilterBindingObjPtr binding) +{ + virNWFilterBindingDefPtr def = virNWFilterBindingObjGetDef(binding); + if (virHashAddEntry(bindings->objs, def->portdevname, binding) < 0) + return -1; + virObjectRef(binding); + + return 0; +} + + +/* + * virNWFilterBindingObjListAddLocked: + * + * The returned @binding from this function will be locked and ref + * counted. The caller is expected to use virNWFilterBindingObjEndAPI + * when it completes usage. + */ +static virNWFilterBindingObjPtr +virNWFilterBindingObjListAddLocked(virNWFilterBindingObjListPtr bindings, + virNWFilterBindingDefPtr def) +{ + virNWFilterBindingObjPtr binding; + + /* See if a binding with matching portdev already exists */ + if ((binding = virNWFilterBindingObjListFindByPortDevLocked( + bindings, def->portdevname))) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("binding '%s' already exists"), + def->portdevname); + goto error; + } + + if (!(binding = virNWFilterBindingObjNew())) + goto error; + + virNWFilterBindingObjSetDef(binding, def); + + if (virNWFilterBindingObjListAddObjLocked(bindings, binding) < 0) + goto error; + + return binding; + + error: + virNWFilterBindingObjEndAPI(&binding); + return NULL; +} + + +virNWFilterBindingObjPtr +virNWFilterBindingObjListAdd(virNWFilterBindingObjListPtr bindings, + virNWFilterBindingDefPtr def) +{ + virNWFilterBindingObjPtr ret; + + virObjectRWLockWrite(bindings); + ret = virNWFilterBindingObjListAddLocked(bindings, def); + virObjectRWUnlock(bindings); + return ret; +} + + +/* The caller must hold lock on 'bindings' in addition to 'virNWFilterBindingObjListRemove' + * requirements + * + * Can be used to remove current element while iterating with + * virNWFilterBindingObjListForEach + */ +static void +virNWFilterBindingObjListRemoveLocked(virNWFilterBindingObjListPtr bindings, + virNWFilterBindingObjPtr binding) +{ + virNWFilterBindingDefPtr def = virNWFilterBindingObjGetDef(binding); + virHashRemoveEntry(bindings->objs, def->portdevname); +} + + +/** + * @bindings: Pointer to the binding object list + * @binding: NWFilterBinding pointer from either after Add or FindBy* API where the + * @binding was successfully added to the bindings->objs + * hash tables that now would need to be removed. + * + * The caller must hold a lock on the driver owning 'bindings', + * and must also have locked and ref counted 'binding', to ensure + * no one else is either waiting for 'binding' or still using it. + * + * When this function returns, @binding will be removed from the hash + * tables and returned with lock and refcnt that was present upon entry. + */ +void +virNWFilterBindingObjListRemove(virNWFilterBindingObjListPtr bindings, + virNWFilterBindingObjPtr binding) +{ + virNWFilterBindingObjSetRemoving(binding, true); + virObjectRef(binding); + virObjectUnlock(binding); + virObjectRWLockWrite(bindings); + virObjectLock(binding); + virNWFilterBindingObjListRemoveLocked(bindings, binding); + virObjectUnref(binding); + virObjectRWUnlock(bindings); +} + + +static virNWFilterBindingObjPtr +virNWFilterBindingObjListLoadStatus(virNWFilterBindingObjListPtr bindings, + const char *statusDir, + const char *name) +{ + char *statusFile = NULL; + virNWFilterBindingObjPtr obj = NULL; + virNWFilterBindingDefPtr def; + + if ((statusFile = virNWFilterBindingObjConfigFile(statusDir, name)) == NULL) + goto error; + + if (!(obj = virNWFilterBindingObjParseFile(statusFile))) + goto error; + + def = virNWFilterBindingObjGetDef(obj); + if (virHashLookup(bindings->objs, def->portdevname) != NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected binding %s already exists"), + def->portdevname); + goto error; + } + + if (virNWFilterBindingObjListAddObjLocked(bindings, obj) < 0) + goto error; + + VIR_FREE(statusFile); + return obj; + + error: + virNWFilterBindingObjEndAPI(&obj); + VIR_FREE(statusFile); + return NULL; +} + + +int +virNWFilterBindingObjListLoadAllConfigs(virNWFilterBindingObjListPtr bindings, + const char *configDir) +{ + DIR *dir; + struct dirent *entry; + int ret = -1; + int rc; + + VIR_INFO("Scanning for configs in %s", configDir); + + if ((rc = virDirOpenIfExists(&dir, configDir)) <= 0) + return rc; + + virObjectRWLockWrite(bindings); + + while ((ret = virDirRead(dir, &entry, configDir)) > 0) { + virNWFilterBindingObjPtr binding; + + if (!virFileStripSuffix(entry->d_name, ".xml")) + continue; + + /* NB: ignoring errors, so one malformed config doesn't + kill the whole process */ + VIR_INFO("Loading config file '%s.xml'", entry->d_name); + binding = virNWFilterBindingObjListLoadStatus(bindings, + configDir, + entry->d_name); + if (binding) + virNWFilterBindingObjEndAPI(&binding); + else + VIR_ERROR(_("Failed to load config for binding '%s'"), entry->d_name); + } + + VIR_DIR_CLOSE(dir); + virObjectRWUnlock(bindings); + return ret; +} + + +struct virNWFilterBindingListIterData { + virNWFilterBindingObjListIterator callback; + void *opaque; + int ret; +}; + + +static int +virNWFilterBindingObjListHelper(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + struct virNWFilterBindingListIterData *data = opaque; + + if (data->callback(payload, data->opaque) < 0) + data->ret = -1; + return 0; +} + + +int +virNWFilterBindingObjListForEach(virNWFilterBindingObjListPtr bindings, + virNWFilterBindingObjListIterator callback, + void *opaque) +{ + struct virNWFilterBindingListIterData data = { + callback, opaque, 0, + }; + virObjectRWLockRead(bindings); + virHashForEach(bindings->objs, virNWFilterBindingObjListHelper, &data); + virObjectRWUnlock(bindings); + return data.ret; +} + + +struct virNWFilterBindingListData { + virNWFilterBindingObjPtr *bindings; + size_t nbindings; +}; + + +static int +virNWFilterBindingObjListCollectIterator(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + struct virNWFilterBindingListData *data = opaque; + + data->bindings[data->nbindings++] = virObjectRef(payload); + return 0; +} + + +static void +virNWFilterBindingObjListFilter(virNWFilterBindingObjPtr **list, + size_t *nbindings, + virConnectPtr conn, + virNWFilterBindingObjListACLFilter filter) +{ + size_t i = 0; + + while (i < *nbindings) { + virNWFilterBindingObjPtr binding = (*list)[i]; + virNWFilterBindingDefPtr def; + + virObjectLock(binding); + + def = virNWFilterBindingObjGetDef(binding); + /* do not list the object if: + * 1) it's being removed. + * 2) connection does not have ACL to see it + * 3) it doesn't match the filter + */ + if (virNWFilterBindingObjGetRemoving(binding) || + (filter && !filter(conn, def))) { + virObjectUnlock(binding); + virObjectUnref(binding); + VIR_DELETE_ELEMENT(*list, i, *nbindings); + continue; + } + + virObjectUnlock(binding); + i++; + } +} + + +static int +virNWFilterBindingObjListCollect(virNWFilterBindingObjListPtr domlist, + virConnectPtr conn, + virNWFilterBindingObjPtr **bindings, + size_t *nbindings, + virNWFilterBindingObjListACLFilter filter) +{ + struct virNWFilterBindingListData data = { NULL, 0 }; + + virObjectRWLockRead(domlist); + sa_assert(domlist->objs); + if (VIR_ALLOC_N(data.bindings, virHashSize(domlist->objs)) < 0) { + virObjectRWUnlock(domlist); + return -1; + } + + virHashForEach(domlist->objs, virNWFilterBindingObjListCollectIterator, &data); + virObjectRWUnlock(domlist); + + virNWFilterBindingObjListFilter(&data.bindings, &data.nbindings, conn, filter); + + *nbindings = data.nbindings; + *bindings = data.bindings; + + return 0; +} + + +int +virNWFilterBindingObjListExport(virNWFilterBindingObjListPtr bindings, + virConnectPtr conn, + virNWFilterBindingPtr **bindinglist, + virNWFilterBindingObjListACLFilter filter) +{ + virNWFilterBindingObjPtr *bindingobjs = NULL; + size_t nbindings = 0; + size_t i; + int ret = -1; + + if (virNWFilterBindingObjListCollect(bindings, conn, &bindingobjs, + &nbindings, filter) < 0) + return -1; + + if (bindinglist) { + if (VIR_ALLOC_N(*bindinglist, nbindings + 1) < 0) + goto cleanup; + + for (i = 0; i < nbindings; i++) { + virNWFilterBindingObjPtr binding = bindingobjs[i]; + virNWFilterBindingDefPtr def = virNWFilterBindingObjGetDef(binding); + + virObjectLock(binding); + (*bindinglist)[i] = virGetNWFilterBinding(conn, def->portdevname, + def->filter); + virObjectUnlock(binding); + + if (!(*bindinglist)[i]) + goto cleanup; + } + } + + ret = nbindings; + + cleanup: + virObjectListFreeCount(bindingobjs, nbindings); + if (ret < 0) { + virObjectListFreeCount(*bindinglist, nbindings); + *bindinglist = NULL; + } + return ret; +} diff --git a/src/conf/virnwfilterbindingobjlist.h b/src/conf/virnwfilterbindingobjlist.h new file mode 100644 index 0000000000..dfda2bea85 --- /dev/null +++ b/src/conf/virnwfilterbindingobjlist.h @@ -0,0 +1,69 @@ +/* + * virnwfilterbindingobjlist.h: nwfilter binding object list utilities + * + * Copyright (C) 2018 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Daniel P. Berrange <berrange@redhat.com> + */ + +#ifndef __VIR_NWFILTER_BINDING_OBJ_LIST_H__ +# define __VIR_NWFILTER_BINDING_OBJ_LIST_H__ + +# include "virnwfilterbindingobj.h" + +typedef struct _virNWFilterBindingObjList virNWFilterBindingObjList; +typedef virNWFilterBindingObjList *virNWFilterBindingObjListPtr; + +virNWFilterBindingObjListPtr +virNWFilterBindingObjListNew(void); + +virNWFilterBindingObjPtr +virNWFilterBindingObjListFindByPortDev(virNWFilterBindingObjListPtr bindings, + const char *name); + +virNWFilterBindingObjPtr +virNWFilterBindingObjListAdd(virNWFilterBindingObjListPtr bindings, + virNWFilterBindingDefPtr def); + +void +virNWFilterBindingObjListRemove(virNWFilterBindingObjListPtr bindings, + virNWFilterBindingObjPtr binding); + +int +virNWFilterBindingObjListLoadAllConfigs(virNWFilterBindingObjListPtr bindings, + const char *configDir); + + +typedef int (*virNWFilterBindingObjListIterator)(virNWFilterBindingObjPtr binding, + void *opaque); + +int +virNWFilterBindingObjListForEach(virNWFilterBindingObjListPtr bindings, + virNWFilterBindingObjListIterator callback, + void *opaque); + +typedef bool (*virNWFilterBindingObjListACLFilter)(virConnectPtr conn, + virNWFilterBindingDefPtr def); + +int +virNWFilterBindingObjListExport(virNWFilterBindingObjListPtr bindings, + virConnectPtr conn, + virNWFilterBindingPtr **bindinglist, + virNWFilterBindingObjListACLFilter filter); + + +#endif /* __VIR_NWFILTER_BINDING_OBJ_LIST_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4ad2116238..43c0ee75a4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1072,6 +1072,16 @@ virNWFilterBindingObjSetDef; virNWFilterBindingObjSetRemoving; +# conf/virnwfilterbindingobjlist.h +virNWFilterBindingObjListAdd; +virNWFilterBindingObjListExport; +virNWFilterBindingObjListFindByPortDev; +virNWFilterBindingObjListForEach; +virNWFilterBindingObjListLoadAllConfigs; +virNWFilterBindingObjListNew; +virNWFilterBindingObjListRemove; + + # conf/virnwfilterobj.h virNWFilterObjGetDef; virNWFilterObjGetNewDef; -- 2.17.0

On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
Introduce a new struct to act as the manager of a collection of virNWFilterBindingObjPtr objects.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/Makefile.inc.am | 2 + src/conf/virnwfilterbindingobjlist.c | 487 +++++++++++++++++++++++++++ src/conf/virnwfilterbindingobjlist.h | 69 ++++ src/libvirt_private.syms | 10 + 4 files changed, 568 insertions(+) create mode 100644 src/conf/virnwfilterbindingobjlist.c create mode 100644 src/conf/virnwfilterbindingobjlist.h
Reviewed-by: John Ferlan <jferlan@redhat.com> John [I'll get to 16-20 ... I had started this on Friday figuring I'd get through everything, but couldn't... So rather than have things lie around for another day or two I figured I'd get through what I'd done so far...]

Currently the nwfilter driver does not keep any record of what filter bindings it has active. This means that when it needs to recreate filters, it has to rely on triggering callbacks provided by the virt drivers. This introduces a hash table recording the virNWFilterBinding objects so the driver has a record of all active filters. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/virnwfilterobj.h | 4 ++ src/nwfilter/nwfilter_driver.c | 86 ++++++++++++++++++++++++---------- 2 files changed, 65 insertions(+), 25 deletions(-) diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index 433b0402d0..4a54dd50da 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -22,6 +22,7 @@ # include "internal.h" # include "nwfilter_conf.h" +# include "virnwfilterbindingobjlist.h" typedef struct _virNWFilterObj virNWFilterObj; typedef virNWFilterObj *virNWFilterObjPtr; @@ -37,7 +38,10 @@ struct _virNWFilterDriverState { virNWFilterObjListPtr nwfilters; + virNWFilterBindingObjListPtr bindings; + char *configDir; + char *bindingDir; }; virNWFilterDefPtr diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 7202691646..2388e925fc 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -38,7 +38,6 @@ #include "domain_conf.h" #include "domain_nwfilter.h" #include "nwfilter_driver.h" -#include "virnwfilterbindingdef.h" #include "nwfilter_gentech_driver.h" #include "configmake.h" #include "virfile.h" @@ -174,7 +173,6 @@ nwfilterStateInitialize(bool privileged, virStateInhibitCallback callback ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { - char *base = NULL; DBusConnection *sysbus = NULL; if (virDBusHasSystemBus() && @@ -191,6 +189,9 @@ nwfilterStateInitialize(bool privileged, if (!(driver->nwfilters = virNWFilterObjListNew())) goto error; + if (!(driver->bindings = virNWFilterBindingObjListNew())) + goto error; + if (!privileged) return 0; @@ -230,30 +231,35 @@ nwfilterStateInitialize(bool privileged, goto error; } - if (VIR_STRDUP(base, SYSCONFDIR "/libvirt") < 0) + if (VIR_STRDUP(driver->configDir, SYSCONFDIR "/libvirt/nwfilter") < 0) goto error; - if (virAsprintf(&driver->configDir, - "%s/nwfilter", base) == -1) + if (virFileMakePathWithMode(driver->configDir, S_IRWXU) < 0) { + virReportSystemError(errno, _("cannot create config directory '%s'"), + driver->configDir); goto error; + } - VIR_FREE(base); + if (VIR_STRDUP(driver->bindingDir, LOCALSTATEDIR "/run/libvirt/nwfilter-binding") < 0) + goto error; - if (virFileMakePathWithMode(driver->configDir, S_IRWXU) < 0) { + if (virFileMakePathWithMode(driver->bindingDir, S_IRWXU) < 0) { virReportSystemError(errno, _("cannot create config directory '%s'"), - driver->configDir); + driver->bindingDir); goto error; } if (virNWFilterObjListLoadAllConfigs(driver->nwfilters, driver->configDir) < 0) goto error; + if (virNWFilterBindingObjListLoadAllConfigs(driver->bindings, driver->bindingDir) < 0) + goto error; + nwfilterDriverUnlock(); return 0; error: - VIR_FREE(base); nwfilterDriverUnlock(); nwfilterStateCleanup(); @@ -333,9 +339,12 @@ nwfilterStateCleanup(void) nwfilterDriverRemoveDBusMatches(); VIR_FREE(driver->configDir); + VIR_FREE(driver->bindingDir); nwfilterDriverUnlock(); } + virObjectUnref(driver->bindings); + /* free inactive nwfilters */ virNWFilterObjListFree(driver->nwfilters); @@ -647,13 +656,37 @@ nwfilterInstantiateFilter(const char *vmname, const unsigned char *vmuuid, virDomainNetDefPtr net) { - virNWFilterBindingDefPtr binding; + virNWFilterBindingObjPtr obj; + virNWFilterBindingDefPtr def; int ret; - if (!(binding = virNWFilterBindingDefForNet(vmname, vmuuid, net))) + obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, net->ifname); + if (obj) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Filter already present for NIC %s"), net->ifname); + virNWFilterBindingObjEndAPI(&obj); + return -1; + } + + if (!(def = virNWFilterBindingDefForNet(vmname, vmuuid, net))) return -1; - ret = virNWFilterInstantiateFilter(driver, binding); - virNWFilterBindingDefFree(binding); + + obj = virNWFilterBindingObjListAdd(driver->bindings, + def); + if (!obj) { + virNWFilterBindingDefFree(def); + return -1; + } + + ret = virNWFilterInstantiateFilter(driver, def); + + if (ret >= 0) + virNWFilterBindingObjSave(obj, driver->bindingDir); + else + virNWFilterBindingObjListRemove(driver->bindings, obj); + + virNWFilterBindingObjEndAPI(&obj); + return ret; } @@ -661,18 +694,21 @@ nwfilterInstantiateFilter(const char *vmname, static void nwfilterTeardownFilter(virDomainNetDefPtr net) { - virNWFilterBindingDef binding = { - .portdevname = net->ifname, - .linkdevname = (net->type == VIR_DOMAIN_NET_TYPE_DIRECT ? - net->data.direct.linkdev : NULL), - .mac = net->mac, - .filter = net->filter, - .filterparams = net->filterparams, - .ownername = NULL, - .owneruuid = {0}, - }; - if ((net->ifname) && (net->filter)) - virNWFilterTeardownFilter(&binding); + virNWFilterBindingObjPtr obj; + virNWFilterBindingDefPtr def; + if (!net->ifname) + return; + + obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, net->ifname); + if (!obj) + return; + + def = virNWFilterBindingObjGetDef(obj); + virNWFilterTeardownFilter(def); + virNWFilterBindingObjDelete(obj, driver->bindingDir); + + virNWFilterBindingObjListRemove(driver->bindings, obj); + virNWFilterBindingObjEndAPI(&obj); } -- 2.17.0

On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
Currently the nwfilter driver does not keep any record of what filter bindings it has active. This means that when it needs to recreate filters, it has to rely on triggering callbacks provided by the virt drivers. This introduces a hash table recording the virNWFilterBinding objects so the driver has a record of all active filters.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/virnwfilterobj.h | 4 ++ src/nwfilter/nwfilter_driver.c | 86 ++++++++++++++++++++++++---------- 2 files changed, 65 insertions(+), 25 deletions(-)
[...]
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 7202691646..2388e925fc 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -38,7 +38,6 @@ #include "domain_conf.h" #include "domain_nwfilter.h" #include "nwfilter_driver.h" -#include "virnwfilterbindingdef.h" #include "nwfilter_gentech_driver.h" #include "configmake.h" #include "virfile.h" @@ -174,7 +173,6 @@ nwfilterStateInitialize(bool privileged, virStateInhibitCallback callback ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) {
[...]
if (virNWFilterObjListLoadAllConfigs(driver->nwfilters, driver->configDir) < 0) goto error;
+ if (virNWFilterBindingObjListLoadAllConfigs(driver->bindings, driver->bindingDir) < 0) + goto error; +
Because of this... [1]
nwfilterDriverUnlock();
return 0;
[...]
@@ -647,13 +656,37 @@ nwfilterInstantiateFilter(const char *vmname, const unsigned char *vmuuid, virDomainNetDefPtr net) { - virNWFilterBindingDefPtr binding; + virNWFilterBindingObjPtr obj; + virNWFilterBindingDefPtr def; int ret;
- if (!(binding = virNWFilterBindingDefForNet(vmname, vmuuid, net))) + obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, net->ifname); + if (obj) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Filter already present for NIC %s"), net->ifname);
[1] If I stop at this patch, start a domain with a filter, then restart libvirtd, then that will cause a guest running with a filter to exit and not just disappear - as it can be restarted. The error is: 2018-06-18 16:49:07.405+0000: 3978: error : nwfilterInstantiateFilter:666 : internal error: Filter already present for NIC vnet1 Because once I have this patch built/running the /var/run/libvirt/nwfilter-binding/vnet1.xml exists and get's loaded when virNWFilterBindingObjListLoadAllConfigs is called at StateInitialize. I only saw this because I found later in patch 20 the failure comes from nwfilterBindingCreateXML instead when virDomainConfNWFilterInstantiate is called. I worked my way back to this point. Not sure which would be the "best" solution at this point. Should we wait to do [1] until patch 20 or perhaps just not have an error here. NB: If the guest was running at a point up through patch 15 then it won't exit on the first libvirtd restart (since the obj dir doesn't exist), but the issue shows up on the 2nd restart. In general the code is fine to me, but just need to handle this one issue in some way. John
+ virNWFilterBindingObjEndAPI(&obj); + return -1; + } + + if (!(def = virNWFilterBindingDefForNet(vmname, vmuuid, net))) return -1; - ret = virNWFilterInstantiateFilter(driver, binding); - virNWFilterBindingDefFree(binding);
[...]

On Mon, Jun 18, 2018 at 04:59:12PM -0400, John Ferlan wrote:
On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
Currently the nwfilter driver does not keep any record of what filter bindings it has active. This means that when it needs to recreate filters, it has to rely on triggering callbacks provided by the virt drivers. This introduces a hash table recording the virNWFilterBinding objects so the driver has a record of all active filters.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/virnwfilterobj.h | 4 ++ src/nwfilter/nwfilter_driver.c | 86 ++++++++++++++++++++++++---------- 2 files changed, 65 insertions(+), 25 deletions(-)
[...]
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 7202691646..2388e925fc 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -38,7 +38,6 @@ #include "domain_conf.h" #include "domain_nwfilter.h" #include "nwfilter_driver.h" -#include "virnwfilterbindingdef.h" #include "nwfilter_gentech_driver.h" #include "configmake.h" #include "virfile.h" @@ -174,7 +173,6 @@ nwfilterStateInitialize(bool privileged, virStateInhibitCallback callback ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) {
[...]
if (virNWFilterObjListLoadAllConfigs(driver->nwfilters, driver->configDir) < 0) goto error;
+ if (virNWFilterBindingObjListLoadAllConfigs(driver->bindings, driver->bindingDir) < 0) + goto error; +
Because of this... [1]
nwfilterDriverUnlock();
return 0;
[...]
@@ -647,13 +656,37 @@ nwfilterInstantiateFilter(const char *vmname, const unsigned char *vmuuid, virDomainNetDefPtr net) { - virNWFilterBindingDefPtr binding; + virNWFilterBindingObjPtr obj; + virNWFilterBindingDefPtr def; int ret;
- if (!(binding = virNWFilterBindingDefForNet(vmname, vmuuid, net))) + obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, net->ifname); + if (obj) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Filter already present for NIC %s"), net->ifname);
[1] If I stop at this patch, start a domain with a filter, then restart libvirtd, then that will cause a guest running with a filter to exit and not just disappear - as it can be restarted. The error is:
2018-06-18 16:49:07.405+0000: 3978: error : nwfilterInstantiateFilter:666 : internal error: Filter already present for NIC vnet1
Because once I have this patch built/running the /var/run/libvirt/nwfilter-binding/vnet1.xml exists and get's loaded when virNWFilterBindingObjListLoadAllConfigs is called at StateInitialize.
I only saw this because I found later in patch 20 the failure comes from nwfilterBindingCreateXML instead when virDomainConfNWFilterInstantiate is called. I worked my way back to this point.
Not sure which would be the "best" solution at this point. Should we wait to do [1] until patch 20 or perhaps just not have an error here.
The current semantics are that nwfilterInstantiateFilter will not report an error if the filter already exists, so I think I'll just not report an error here. This method will go away anyway, so no great loss.
NB: If the guest was running at a point up through patch 15 then it won't exit on the first libvirtd restart (since the obj dir doesn't exist), but the issue shows up on the 2nd restart.
In general the code is fine to me, but just need to handle this one issue in some way.
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Now that the nwfilter driver keeps a list of bindings that it has created, there is no need for the complex virt driver callbacks. It is possible to simply iterate of the list of recorded filter bindings. This means that rebuilding filters no longer has to acquire any locks on the virDomainObj objects, as they're never touched. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/nwfilter_conf.c | 134 +++----------------- src/conf/nwfilter_conf.h | 51 +------- src/conf/virnwfilterobj.c | 4 +- src/libvirt_private.syms | 7 +- src/lxc/lxc_driver.c | 28 ----- src/nwfilter/nwfilter_driver.c | 21 ++-- src/nwfilter/nwfilter_gentech_driver.c | 167 ++++++++++++++++--------- src/nwfilter/nwfilter_gentech_driver.h | 4 +- src/qemu/qemu_driver.c | 25 ---- src/uml/uml_driver.c | 29 ----- 10 files changed, 141 insertions(+), 329 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index de26a6d034..5bb8a0c2e7 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2819,121 +2819,6 @@ virNWFilterSaveConfig(const char *configDir, } -int nCallbackDriver; -#define MAX_CALLBACK_DRIVER 10 -static virNWFilterCallbackDriverPtr callbackDrvArray[MAX_CALLBACK_DRIVER]; - -void -virNWFilterRegisterCallbackDriver(virNWFilterCallbackDriverPtr cbd) -{ - if (nCallbackDriver < MAX_CALLBACK_DRIVER) - callbackDrvArray[nCallbackDriver++] = cbd; -} - - -void -virNWFilterUnRegisterCallbackDriver(virNWFilterCallbackDriverPtr cbd) -{ - size_t i = 0; - - while (i < nCallbackDriver && callbackDrvArray[i] != cbd) - i++; - - if (i < nCallbackDriver) { - memmove(&callbackDrvArray[i], &callbackDrvArray[i+1], - (nCallbackDriver - i - 1) * sizeof(callbackDrvArray[i])); - callbackDrvArray[i] = 0; - nCallbackDriver--; - } -} - - -void -virNWFilterCallbackDriversLock(void) -{ - size_t i; - - for (i = 0; i < nCallbackDriver; i++) - callbackDrvArray[i]->vmDriverLock(); -} - - -void -virNWFilterCallbackDriversUnlock(void) -{ - size_t i; - - for (i = 0; i < nCallbackDriver; i++) - callbackDrvArray[i]->vmDriverUnlock(); -} - - -static virDomainObjListIterator virNWFilterDomainFWUpdateCB; -static void *virNWFilterDomainFWUpdateOpaque; - -/** - * virNWFilterInstFiltersOnAllVMs: - * Apply all filters on all running VMs. Don't terminate in case of an - * error. This should be called upon reloading of the driver. - */ -int -virNWFilterInstFiltersOnAllVMs(void) -{ - size_t i; - struct domUpdateCBStruct cb = { - .opaque = virNWFilterDomainFWUpdateOpaque, - .step = STEP_APPLY_CURRENT, - .skipInterfaces = NULL, /* not needed */ - }; - - for (i = 0; i < nCallbackDriver; i++) - callbackDrvArray[i]->vmFilterRebuild(virNWFilterDomainFWUpdateCB, - &cb); - - return 0; -} - - -int -virNWFilterTriggerVMFilterRebuild(void) -{ - size_t i; - int ret = 0; - struct domUpdateCBStruct cb = { - .opaque = virNWFilterDomainFWUpdateOpaque, - .step = STEP_APPLY_NEW, - .skipInterfaces = virHashCreate(0, NULL), - }; - - if (!cb.skipInterfaces) - return -1; - - for (i = 0; i < nCallbackDriver; i++) { - if (callbackDrvArray[i]->vmFilterRebuild(virNWFilterDomainFWUpdateCB, - &cb) < 0) - ret = -1; - } - - if (ret < 0) { - cb.step = STEP_TEAR_NEW; /* rollback */ - - for (i = 0; i < nCallbackDriver; i++) - callbackDrvArray[i]->vmFilterRebuild(virNWFilterDomainFWUpdateCB, - &cb); - } else { - cb.step = STEP_TEAR_OLD; /* switch over */ - - for (i = 0; i < nCallbackDriver; i++) - callbackDrvArray[i]->vmFilterRebuild(virNWFilterDomainFWUpdateCB, - &cb); - } - - virHashFree(cb.skipInterfaces); - - return ret; -} - - int virNWFilterDeleteDef(const char *configDir, virNWFilterDefPtr def) @@ -3204,16 +3089,18 @@ virNWFilterDefFormat(const virNWFilterDef *def) return NULL; } +static virNWFilterTriggerRebuildCallback rebuildCallback; +static void *rebuildOpaque; int -virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB, +virNWFilterConfLayerInit(virNWFilterTriggerRebuildCallback cb, void *opaque) { if (initialized) return -1; - virNWFilterDomainFWUpdateCB = domUpdateCB; - virNWFilterDomainFWUpdateOpaque = opaque; + rebuildCallback = cb; + rebuildOpaque = opaque; initialized = true; @@ -3233,8 +3120,15 @@ virNWFilterConfLayerShutdown(void) virRWLockDestroy(&updateLock); initialized = false; - virNWFilterDomainFWUpdateOpaque = NULL; - virNWFilterDomainFWUpdateCB = NULL; + rebuildCallback = NULL; + rebuildOpaque = NULL; +} + + +int +virNWFilterTriggerRebuild(void) +{ + return rebuildCallback(rebuildOpaque); } diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 08fc07c55c..9f8ad51bf2 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -546,20 +546,6 @@ struct _virNWFilterDef { }; -typedef enum { - STEP_APPLY_NEW, - STEP_TEAR_NEW, - STEP_TEAR_OLD, - STEP_APPLY_CURRENT, -} UpdateStep; - -struct domUpdateCBStruct { - void *opaque; - UpdateStep step; - virHashTablePtr skipInterfaces; -}; - - void virNWFilterRuleDefFree(virNWFilterRuleDefPtr def); @@ -567,7 +553,7 @@ void virNWFilterDefFree(virNWFilterDefPtr def); int -virNWFilterTriggerVMFilterRebuild(void); +virNWFilterTriggerRebuild(void); int virNWFilterDeleteDef(const char *configDir, @@ -599,44 +585,15 @@ virNWFilterReadLockFilterUpdates(void); void virNWFilterUnlockFilterUpdates(void); +typedef int (*virNWFilterTriggerRebuildCallback)(void *opaque); + int -virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB, +virNWFilterConfLayerInit(virNWFilterTriggerRebuildCallback cb, void *opaque); void virNWFilterConfLayerShutdown(void); -int -virNWFilterInstFiltersOnAllVMs(void); - -typedef int -(*virNWFilterRebuild)(virDomainObjListIterator domUpdateCB, - void *data); - -typedef void -(*virNWFilterVoidCall)(void); - -typedef struct _virNWFilterCallbackDriver virNWFilterCallbackDriver; -typedef virNWFilterCallbackDriver *virNWFilterCallbackDriverPtr; -struct _virNWFilterCallbackDriver { - const char *name; - - virNWFilterRebuild vmFilterRebuild; - virNWFilterVoidCall vmDriverLock; - virNWFilterVoidCall vmDriverUnlock; -}; - -void -virNWFilterRegisterCallbackDriver(virNWFilterCallbackDriverPtr); - -void -virNWFilterUnRegisterCallbackDriver(virNWFilterCallbackDriverPtr); - -void -virNWFilterCallbackDriversLock(void); - -void -virNWFilterCallbackDriversUnlock(void); char * virNWFilterPrintTCPFlags(uint8_t flags); diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index 87d7e72703..0136a0d56c 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -276,7 +276,7 @@ virNWFilterObjTestUnassignDef(virNWFilterObjPtr obj) obj->wantRemoved = true; /* trigger the update on VMs referencing the filter */ - if (virNWFilterTriggerVMFilterRebuild() < 0) + if (virNWFilterTriggerRebuild() < 0) rc = -1; obj->wantRemoved = false; @@ -358,7 +358,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters, obj->newDef = def; /* trigger the update on VMs referencing the filter */ - if (virNWFilterTriggerVMFilterRebuild() < 0) { + if (virNWFilterTriggerRebuild() < 0) { obj->newDef = NULL; virNWFilterObjUnlock(obj); return NULL; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 43c0ee75a4..ecc60dbe73 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -788,8 +788,6 @@ virDomainNumatuneSpecifiedMaxNode; # conf/nwfilter_conf.h -virNWFilterCallbackDriversLock; -virNWFilterCallbackDriversUnlock; virNWFilterChainSuffixTypeToString; virNWFilterConfLayerInit; virNWFilterConfLayerShutdown; @@ -798,12 +796,10 @@ virNWFilterDefFree; virNWFilterDefParseFile; virNWFilterDefParseString; virNWFilterDeleteDef; -virNWFilterInstFiltersOnAllVMs; virNWFilterJumpTargetTypeToString; virNWFilterPrintStateMatchFlags; virNWFilterPrintTCPFlags; virNWFilterReadLockFilterUpdates; -virNWFilterRegisterCallbackDriver; virNWFilterRuleActionTypeToString; virNWFilterRuleDirectionTypeToString; virNWFilterRuleIsProtocolEthernet; @@ -811,9 +807,8 @@ virNWFilterRuleIsProtocolIPv4; virNWFilterRuleIsProtocolIPv6; virNWFilterRuleProtocolTypeToString; virNWFilterSaveConfig; -virNWFilterTriggerVMFilterRebuild; +virNWFilterTriggerRebuild; virNWFilterUnlockFilterUpdates; -virNWFilterUnRegisterCallbackDriver; virNWFilterWriteLockFilterUpdates; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index cfb431488d..bde0ff6ad4 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -66,7 +66,6 @@ #include "virfdstream.h" #include "domain_audit.h" #include "domain_nwfilter.h" -#include "nwfilter_conf.h" #include "virinitctl.h" #include "virnetdev.h" #include "virnetdevtap.h" @@ -95,31 +94,6 @@ static int lxcStateInitialize(bool privileged, static int lxcStateCleanup(void); virLXCDriverPtr lxc_driver = NULL; -/* callbacks for nwfilter */ -static int -lxcVMFilterRebuild(virDomainObjListIterator iter, void *data) -{ - return virDomainObjListForEach(lxc_driver->domains, iter, data); -} - -static void -lxcVMDriverLock(void) -{ - lxcDriverLock(lxc_driver); -} - -static void -lxcVMDriverUnlock(void) -{ - lxcDriverUnlock(lxc_driver); -} - -static virNWFilterCallbackDriver lxcCallbackDriver = { - .name = "LXC", - .vmFilterRebuild = lxcVMFilterRebuild, - .vmDriverLock = lxcVMDriverLock, - .vmDriverUnlock = lxcVMDriverUnlock, -}; /** * lxcDomObjFromDomain: @@ -1672,7 +1646,6 @@ static int lxcStateInitialize(bool privileged, NULL, NULL) < 0) goto cleanup; - virNWFilterRegisterCallbackDriver(&lxcCallbackDriver); virObjectUnref(caps); return 0; @@ -1744,7 +1717,6 @@ static int lxcStateCleanup(void) if (lxc_driver == NULL) return -1; - virNWFilterUnRegisterCallbackDriver(&lxcCallbackDriver); virObjectUnref(lxc_driver->domains); virObjectUnref(lxc_driver->domainEventState); diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 2388e925fc..3b111e3dc7 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -163,6 +163,14 @@ nwfilterDriverInstallDBusMatches(DBusConnection *sysbus ATTRIBUTE_UNUSED) #endif /* HAVE_FIREWALLD */ +static int virNWFilterTriggerRebuildImpl(void *opaque) +{ + virNWFilterDriverStatePtr nwdriver = opaque; + + return virNWFilterBuildAll(nwdriver, true); +} + + /** * nwfilterStateInitialize: * @@ -207,7 +215,7 @@ nwfilterStateInitialize(bool privileged, if (virNWFilterTechDriversInit(privileged) < 0) goto err_dhcpsnoop_shutdown; - if (virNWFilterConfLayerInit(virNWFilterDomainFWUpdateCB, + if (virNWFilterConfLayerInit(virNWFilterTriggerRebuildImpl, driver) < 0) goto err_techdrivers_shutdown; @@ -302,15 +310,14 @@ nwfilterStateReload(void) nwfilterDriverLock(); virNWFilterWriteLockFilterUpdates(); - virNWFilterCallbackDriversLock(); virNWFilterObjListLoadAllConfigs(driver->nwfilters, driver->configDir); - virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); - nwfilterDriverUnlock(); - virNWFilterInstFiltersOnAllVMs(); + virNWFilterBuildAll(driver, false); + + nwfilterDriverUnlock(); return 0; } @@ -547,7 +554,6 @@ nwfilterDefineXML(virConnectPtr conn, nwfilterDriverLock(); virNWFilterWriteLockFilterUpdates(); - virNWFilterCallbackDriversLock(); if (!(def = virNWFilterDefParseString(xml))) goto cleanup; @@ -572,7 +578,6 @@ nwfilterDefineXML(virConnectPtr conn, if (obj) virNWFilterObjUnlock(obj); - virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); nwfilterDriverUnlock(); return nwfilter; @@ -588,7 +593,6 @@ nwfilterUndefine(virNWFilterPtr nwfilter) nwfilterDriverLock(); virNWFilterWriteLockFilterUpdates(); - virNWFilterCallbackDriversLock(); if (!(obj = nwfilterObjFromNWFilter(nwfilter->uuid))) goto cleanup; @@ -615,7 +619,6 @@ nwfilterUndefine(virNWFilterPtr nwfilter) if (obj) virNWFilterObjUnlock(obj); - virNWFilterCallbackDriversUnlock(); virNWFilterUnlockFilterUpdates(); nwfilterDriverUnlock(); return ret; diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 4b55bd6ca4..d208d0188e 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -153,9 +153,9 @@ virNWFilterVarHashmapAddStdValues(virHashTablePtr table, if (!val) return -1; - if (virHashAddEntry(table, - NWFILTER_STD_VAR_MAC, - val) < 0) { + if (virHashUpdateEntry(table, + NWFILTER_STD_VAR_MAC, + val) < 0) { virNWFilterVarValueFree(val); virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not add variable 'MAC' to hashmap")); @@ -168,9 +168,9 @@ virNWFilterVarHashmapAddStdValues(virHashTablePtr table, if (!val) return -1; - if (virHashAddEntry(table, - NWFILTER_STD_VAR_IP, - val) < 0) { + if (virHashUpdateEntry(table, + NWFILTER_STD_VAR_IP, + val) < 0) { virNWFilterVarValueFree(val); virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not add variable 'IP' to hashmap")); @@ -973,68 +973,113 @@ virNWFilterTeardownFilter(virNWFilterBindingDefPtr binding) return ret; } +enum { + STEP_APPLY_NEW, + STEP_ROLLBACK, + STEP_SWITCH, + STEP_APPLY_CURRENT, +}; -int -virNWFilterDomainFWUpdateCB(virDomainObjPtr obj, - void *data) +static int +virNWFilterBuildOne(virNWFilterDriverStatePtr driver, + virNWFilterBindingDefPtr binding, + virHashTablePtr skipInterfaces, + int step) { - virDomainDefPtr vm = obj->def; - struct domUpdateCBStruct *cb = data; - size_t i; bool skipIface; int ret = 0; - - virObjectLock(obj); - - if (virDomainObjIsActive(obj)) { - for (i = 0; i < vm->nnets; i++) { - virDomainNetDefPtr net = vm->nets[i]; - virNWFilterBindingDefPtr binding; - - if ((net->filter) && (net->ifname) && - (binding = virNWFilterBindingDefForNet( - vm->name, vm->uuid, net))) { - - switch (cb->step) { - case STEP_APPLY_NEW: - ret = virNWFilterUpdateInstantiateFilter(cb->opaque, - binding, - &skipIface); - if (ret == 0 && skipIface) { - /* filter tree unchanged -- no update needed */ - ret = virHashAddEntry(cb->skipInterfaces, - net->ifname, - (void *)~0); - } - break; - - case STEP_TEAR_NEW: - if (!virHashLookup(cb->skipInterfaces, net->ifname)) - ret = virNWFilterRollbackUpdateFilter(binding); - break; - - case STEP_TEAR_OLD: - if (!virHashLookup(cb->skipInterfaces, net->ifname)) - ret = virNWFilterTearOldFilter(binding); - break; - - case STEP_APPLY_CURRENT: - ret = virNWFilterInstantiateFilter(cb->opaque, - binding); - if (ret) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failure while applying current filter on " - "VM %s"), vm->name); - break; - } - virNWFilterBindingDefFree(binding); - if (ret) - break; - } + VIR_DEBUG("Building filter for portdev=%s step=%d", binding->portdevname, step); + + switch (step) { + case STEP_APPLY_NEW: + ret = virNWFilterUpdateInstantiateFilter(driver, + binding, + &skipIface); + if (ret == 0 && skipIface) { + /* filter tree unchanged -- no update needed */ + ret = virHashAddEntry(skipInterfaces, + binding->portdevname, + (void *)~0); } + break; + + case STEP_ROLLBACK: + if (!virHashLookup(skipInterfaces, binding->portdevname)) + ret = virNWFilterRollbackUpdateFilter(binding); + break; + + case STEP_SWITCH: + if (!virHashLookup(skipInterfaces, binding->portdevname)) + ret = virNWFilterTearOldFilter(binding); + break; + + case STEP_APPLY_CURRENT: + ret = virNWFilterInstantiateFilter(driver, + binding); + break; } - virObjectUnlock(obj); + return ret; +} + + +struct virNWFilterBuildData { + virNWFilterDriverStatePtr driver; + virHashTablePtr skipInterfaces; + int step; +}; + +static int +virNWFilterBuildIter(virNWFilterBindingObjPtr binding, void *opaque) +{ + struct virNWFilterBuildData *data = opaque; + virNWFilterBindingDefPtr def = virNWFilterBindingObjGetDef(binding); + + return virNWFilterBuildOne(data->driver, def, + data->skipInterfaces, data->step); +} + +int +virNWFilterBuildAll(virNWFilterDriverStatePtr driver, + bool newFilters) +{ + struct virNWFilterBuildData data = { + .driver = driver, + }; + int ret = 0; + + VIR_DEBUG("Build all filters newFilters=%d", newFilters); + + if (newFilters) { + if (!(data.skipInterfaces = virHashCreate(0, NULL))) + return -1; + + data.step = STEP_APPLY_NEW; + if (virNWFilterBindingObjListForEach(driver->bindings, + virNWFilterBuildIter, + &data) < 0) + ret = -1; + + if (ret == -1) { + data.step = STEP_ROLLBACK; + virNWFilterBindingObjListForEach(driver->bindings, + virNWFilterBuildIter, + &data); + } else { + data.step = STEP_SWITCH; + virNWFilterBindingObjListForEach(driver->bindings, + virNWFilterBuildIter, + &data); + } + + virHashFree(data.skipInterfaces); + } else { + data.step = STEP_APPLY_CURRENT; + if (virNWFilterBindingObjListForEach(driver->bindings, + virNWFilterBuildIter, + &data) < 0) + ret = -1; + } return ret; } diff --git a/src/nwfilter/nwfilter_gentech_driver.h b/src/nwfilter/nwfilter_gentech_driver.h index 6b51096a0d..481fdd2413 100644 --- a/src/nwfilter/nwfilter_gentech_driver.h +++ b/src/nwfilter/nwfilter_gentech_driver.h @@ -54,8 +54,8 @@ int virNWFilterTeardownFilter(virNWFilterBindingDefPtr binding); virHashTablePtr virNWFilterCreateVarHashmap(const char *macaddr, const virNWFilterVarValue *value); -int virNWFilterDomainFWUpdateCB(virDomainObjPtr vm, - void *data); +int virNWFilterBuildAll(virNWFilterDriverStatePtr driver, + bool newFilters); virNWFilterBindingDefPtr virNWFilterBindingDefForNet(const char *vmname, const unsigned char *vmuuid, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 42069ee617..2a70e442ac 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -84,7 +84,6 @@ #include "cpu/cpu.h" #include "virsysinfo.h" #include "domain_nwfilter.h" -#include "nwfilter_conf.h" #include "virhook.h" #include "virstoragefile.h" #include "virfile.h" @@ -164,28 +163,6 @@ static int qemuARPGetInterfaces(virDomainObjPtr vm, static virQEMUDriverPtr qemu_driver; - -static void -qemuVMDriverLock(void) -{} -static void -qemuVMDriverUnlock(void) -{} - -static int -qemuVMFilterRebuild(virDomainObjListIterator iter, void *data) -{ - return virDomainObjListForEach(qemu_driver->domains, iter, data); -} - -static virNWFilterCallbackDriver qemuCallbackDriver = { - .name = QEMU_DRIVER_NAME, - .vmFilterRebuild = qemuVMFilterRebuild, - .vmDriverLock = qemuVMDriverLock, - .vmDriverUnlock = qemuVMDriverUnlock, -}; - - /** * qemuDomObjFromDomain: * @domain: Domain pointer that has to be looked up @@ -941,7 +918,6 @@ qemuStateInitialize(bool privileged, qemuProcessReconnectAll(qemu_driver); - virNWFilterRegisterCallbackDriver(&qemuCallbackDriver); return 0; error: @@ -1081,7 +1057,6 @@ qemuStateCleanup(void) if (!qemu_driver) return -1; - virNWFilterUnRegisterCallbackDriver(&qemuCallbackDriver); virThreadPoolFree(qemu_driver->workerPool); virObjectUnref(qemu_driver->config); virObjectUnref(qemu_driver->hostdevMgr); diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 0c5b7fcda7..c77988f01e 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -55,7 +55,6 @@ #include "datatypes.h" #include "virlog.h" #include "domain_nwfilter.h" -#include "nwfilter_conf.h" #include "virfile.h" #include "virfdstream.h" #include "configmake.h" @@ -143,25 +142,6 @@ static int umlMonitorCommand(const struct uml_driver *driver, static struct uml_driver *uml_driver; -static int -umlVMFilterRebuild(virDomainObjListIterator iter, void *data) -{ - return virDomainObjListForEach(uml_driver->domains, iter, data); -} - -static void -umlVMDriverLock(void) -{ - umlDriverLock(uml_driver); -} - -static void -umlVMDriverUnlock(void) -{ - umlDriverUnlock(uml_driver); -} - - static virDomainObjPtr umlDomObjFromDomainLocked(struct uml_driver *driver, const unsigned char *uuid) @@ -194,13 +174,6 @@ umlDomObjFromDomain(struct uml_driver *driver, } -static virNWFilterCallbackDriver umlCallbackDriver = { - .name = "UML", - .vmFilterRebuild = umlVMFilterRebuild, - .vmDriverLock = umlVMDriverLock, - .vmDriverUnlock = umlVMDriverUnlock, -}; - struct umlAutostartData { struct uml_driver *driver; virConnectPtr conn; @@ -604,7 +577,6 @@ umlStateInitialize(bool privileged, VIR_FREE(userdir); - virNWFilterRegisterCallbackDriver(¨CallbackDriver); return 0; out_of_memory: @@ -697,7 +669,6 @@ umlStateCleanup(void) return -1; umlDriverLock(uml_driver); - virNWFilterRegisterCallbackDriver(¨CallbackDriver); if (uml_driver->inotifyWatch != -1) virEventRemoveHandle(uml_driver->inotifyWatch); VIR_FORCE_CLOSE(uml_driver->inotifyFD); -- 2.17.0

On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
Now that the nwfilter driver keeps a list of bindings that it has created, there is no need for the complex virt driver callbacks. It is possible to simply iterate of the list of recorded filter bindings.
This means that rebuilding filters no longer has to acquire any locks on the virDomainObj objects, as they're never touched.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/nwfilter_conf.c | 134 +++----------------- src/conf/nwfilter_conf.h | 51 +------- src/conf/virnwfilterobj.c | 4 +- src/libvirt_private.syms | 7 +- src/lxc/lxc_driver.c | 28 ----- src/nwfilter/nwfilter_driver.c | 21 ++-- src/nwfilter/nwfilter_gentech_driver.c | 167 ++++++++++++++++--------- src/nwfilter/nwfilter_gentech_driver.h | 4 +- src/qemu/qemu_driver.c | 25 ---- src/uml/uml_driver.c | 29 ----- 10 files changed, 141 insertions(+), 329 deletions(-)
A diffstat that Jano usually applauds! Miracles do happen when you close your eyes and say 3 times "I wish this code would just go away" ;-). Still this is some of the most "entertaining code" - that now used to exist! It seems I can dig up my nwfilter obj/hash code once this is in... There's a couple nits below... Reviewed-by: John Ferlan <jferlan@redhat.com> John
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index de26a6d034..5bb8a0c2e7 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c
[...]
+ + +int +virNWFilterTriggerRebuild(void) +{ + return rebuildCallback(rebuildOpaque);
In the better safe than sorry - should we gate on "if (rebuildCallback)"? I don't think there's a way into here with it being NULL, but those are always "famous last words".
}
[...]
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 2388e925fc..3b111e3dc7 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -163,6 +163,14 @@ nwfilterDriverInstallDBusMatches(DBusConnection *sysbus ATTRIBUTE_UNUSED)
#endif /* HAVE_FIREWALLD */
+static int virNWFilterTriggerRebuildImpl(void *opaque)
NIT: static int virNWFilterTriggerRebuildImpl(void *opaque)
+{ + virNWFilterDriverStatePtr nwdriver = opaque; + + return virNWFilterBuildAll(nwdriver, true); +} + +
[...]

On Mon, Jun 18, 2018 at 04:59:37PM -0400, John Ferlan wrote:
On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
Now that the nwfilter driver keeps a list of bindings that it has created, there is no need for the complex virt driver callbacks. It is possible to simply iterate of the list of recorded filter bindings.
This means that rebuilding filters no longer has to acquire any locks on the virDomainObj objects, as they're never touched.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/nwfilter_conf.c | 134 +++----------------- src/conf/nwfilter_conf.h | 51 +------- src/conf/virnwfilterobj.c | 4 +- src/libvirt_private.syms | 7 +- src/lxc/lxc_driver.c | 28 ----- src/nwfilter/nwfilter_driver.c | 21 ++-- src/nwfilter/nwfilter_gentech_driver.c | 167 ++++++++++++++++--------- src/nwfilter/nwfilter_gentech_driver.h | 4 +- src/qemu/qemu_driver.c | 25 ---- src/uml/uml_driver.c | 29 ----- 10 files changed, 141 insertions(+), 329 deletions(-)
A diffstat that Jano usually applauds! Miracles do happen when you close your eyes and say 3 times "I wish this code would just go away" ;-). Still this is some of the most "entertaining code" - that now used to exist! It seems I can dig up my nwfilter obj/hash code once this is in...
There's a couple nits below...
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index de26a6d034..5bb8a0c2e7 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c
[...]
+ + +int +virNWFilterTriggerRebuild(void) +{ + return rebuildCallback(rebuildOpaque);
In the better safe than sorry - should we gate on "if (rebuildCallback)"? I don't think there's a way into here with it being NULL, but those are always "famous last words".
Yeah ok. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Wire up the ListAll, LookupByPortDev and GetXMLDesc APIs to allow the virsh nwfilter-binding-list & nwfilter-binding-dumpxml commands to work. Reviewed-by: John Ferlan <jferlan@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/nwfilter/nwfilter_driver.c | 76 ++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 3b111e3dc7..6bfb584b09 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -715,6 +715,79 @@ nwfilterTeardownFilter(virDomainNetDefPtr net) } +static virNWFilterBindingPtr +nwfilterBindingLookupByPortDev(virConnectPtr conn, + const char *portdev) +{ + virNWFilterBindingPtr ret = NULL; + virNWFilterBindingObjPtr obj; + virNWFilterBindingDefPtr def; + + obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, + portdev); + if (!obj) + goto cleanup; + + def = virNWFilterBindingObjGetDef(obj); + if (virNWFilterBindingLookupByPortDevEnsureACL(conn, def) < 0) + goto cleanup; + + ret = virGetNWFilterBinding(conn, def->portdevname, def->filter); + + cleanup: + virNWFilterBindingObjEndAPI(&obj); + return ret; +} + + +static int +nwfilterConnectListAllNWFilterBindings(virConnectPtr conn, + virNWFilterBindingPtr **bindings, + unsigned int flags) +{ + int ret; + + virCheckFlags(0, -1); + + if (virConnectListAllNWFilterBindingsEnsureACL(conn) < 0) + return -1; + + ret = virNWFilterBindingObjListExport(driver->bindings, + conn, + bindings, + virConnectListAllNWFilterBindingsCheckACL); + + return ret; +} + + +static char * +nwfilterBindingGetXMLDesc(virNWFilterBindingPtr binding, + unsigned int flags) +{ + virNWFilterBindingObjPtr obj; + virNWFilterBindingDefPtr def; + char *ret = NULL; + + virCheckFlags(0, NULL); + + obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, + binding->portdev); + if (!obj) + goto cleanup; + + def = virNWFilterBindingObjGetDef(obj); + if (virNWFilterBindingGetXMLDescEnsureACL(binding->conn, def) < 0) + goto cleanup; + + ret = virNWFilterBindingDefFormat(def); + + cleanup: + virNWFilterBindingObjEndAPI(&obj); + return ret; +} + + static virNWFilterDriver nwfilterDriver = { .name = "nwfilter", .connectNumOfNWFilters = nwfilterConnectNumOfNWFilters, /* 0.8.0 */ @@ -725,6 +798,9 @@ static virNWFilterDriver nwfilterDriver = { .nwfilterDefineXML = nwfilterDefineXML, /* 0.8.0 */ .nwfilterUndefine = nwfilterUndefine, /* 0.8.0 */ .nwfilterGetXMLDesc = nwfilterGetXMLDesc, /* 0.8.0 */ + .nwfilterBindingLookupByPortDev = nwfilterBindingLookupByPortDev, /* 4.5.0 */ + .connectListAllNWFilterBindings = nwfilterConnectListAllNWFilterBindings, /* 4.5.0 */ + .nwfilterBindingGetXMLDesc = nwfilterBindingGetXMLDesc, /* 4.5.0 */ }; -- 2.17.0

This allows the virsh commands nwfilter-binding-create and nwfilter-binding-delete to be used. Note using these commands lets you delete filters that were previously created automatically by the virt drivers, or add filters for VM nics that were not there before. Generally it is expected these new APIs will only be used by virt drivers. It is the admin's responsibility to not shoot themselves in the foot. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/nwfilter/nwfilter_driver.c | 79 ++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 6bfb584b09..2b6856a36c 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -788,6 +788,83 @@ nwfilterBindingGetXMLDesc(virNWFilterBindingPtr binding, } +static virNWFilterBindingPtr +nwfilterBindingCreateXML(virConnectPtr conn, + const char *xml, + unsigned int flags) +{ + virNWFilterBindingObjPtr obj; + virNWFilterBindingDefPtr def; + virNWFilterBindingPtr ret = NULL; + + virCheckFlags(0, NULL); + + def = virNWFilterBindingDefParseString(xml); + if (!def) + return NULL; + + if (virNWFilterBindingCreateXMLEnsureACL(conn, def) < 0) + goto cleanup; + + obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, def->portdevname); + if (obj) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Filter already present for NIC %s"), def->portdevname); + goto cleanup; + } + + obj = virNWFilterBindingObjListAdd(driver->bindings, + def); + if (!obj) + goto cleanup; + + if (!(ret = virGetNWFilterBinding(conn, def->portdevname, def->filter))) + goto cleanup; + + if (virNWFilterInstantiateFilter(driver, def) < 0) { + virNWFilterBindingObjListRemove(driver->bindings, obj); + virObjectUnref(ret); + ret = NULL; + goto cleanup; + } + virNWFilterBindingObjSave(obj, driver->bindingDir); + + cleanup: + if (!obj) + virNWFilterBindingDefFree(def); + virNWFilterBindingObjEndAPI(&obj); + + return ret; +} + + +static int +nwfilterBindingDelete(virNWFilterBindingPtr binding) +{ + virNWFilterBindingObjPtr obj; + virNWFilterBindingDefPtr def; + int ret = -1; + + obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, binding->portdev); + if (!obj) + return -1; + + def = virNWFilterBindingObjGetDef(obj); + if (virNWFilterBindingDeleteEnsureACL(binding->conn, def) < 0) + goto cleanup; + + virNWFilterTeardownFilter(def); + virNWFilterBindingObjDelete(obj, driver->bindingDir); + virNWFilterBindingObjListRemove(driver->bindings, obj); + + ret = 0; + + cleanup: + virNWFilterBindingObjEndAPI(&obj); + return ret; +} + + static virNWFilterDriver nwfilterDriver = { .name = "nwfilter", .connectNumOfNWFilters = nwfilterConnectNumOfNWFilters, /* 0.8.0 */ @@ -801,6 +878,8 @@ static virNWFilterDriver nwfilterDriver = { .nwfilterBindingLookupByPortDev = nwfilterBindingLookupByPortDev, /* 4.5.0 */ .connectListAllNWFilterBindings = nwfilterConnectListAllNWFilterBindings, /* 4.5.0 */ .nwfilterBindingGetXMLDesc = nwfilterBindingGetXMLDesc, /* 4.5.0 */ + .nwfilterBindingCreateXML = nwfilterBindingCreateXML, /* 4.5.0 */ + .nwfilterBindingDelete = nwfilterBindingDelete, /* 4.5.0 */ }; -- 2.17.0

On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
This allows the virsh commands nwfilter-binding-create and nwfilter-binding-delete to be used.
Note using these commands lets you delete filters that were previously created automatically by the virt drivers, or add filters for VM nics that were not there before. Generally it is expected these new APIs will only be used by virt drivers. It is the admin's responsibility to not shoot themselves in the foot.
Can't wait to see QE get ahold of this ;-)
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/nwfilter/nwfilter_driver.c | 79 ++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+)
I think with a couple of extra comments as described below, then this looks fine. Not sure how the other point regarding calling CreateXML from the ConfNWFilterInstantiate path (and the reload of load all configs) will be handled, but I'm sure it'll be something handled in patch 16 and 20, so the comment below is just the "tracing" I did while reviewing... Reviewed-by: John Ferlan <jferlan@redhat.com> John
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 6bfb584b09..2b6856a36c 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -788,6 +788,83 @@ nwfilterBindingGetXMLDesc(virNWFilterBindingPtr binding, }
Because "not everyone" reads the commit message that added this, I think adding a few comments here and for BindingDelete to essentially indicate the same as the commit message would be good.
+static virNWFilterBindingPtr +nwfilterBindingCreateXML(virConnectPtr conn, + const char *xml, + unsigned int flags) +{ + virNWFilterBindingObjPtr obj; + virNWFilterBindingDefPtr def; + virNWFilterBindingPtr ret = NULL; + + virCheckFlags(0, NULL); + + def = virNWFilterBindingDefParseString(xml); + if (!def) + return NULL; + + if (virNWFilterBindingCreateXMLEnsureACL(conn, def) < 0) + goto cleanup; + + obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, def->portdevname); + if (obj) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Filter already present for NIC %s"), def->portdevname);
Recall my point from patch 16 regarding the existence of the filter.
From certain paths it's OK if it exists but once this becomes functional for the subsequent patch via virDomainConfNWFilterInstantiate, then the issue from patch 16 moves to patch 20 (e.g. guest not restarting).
+ goto cleanup; + } + + obj = virNWFilterBindingObjListAdd(driver->bindings, + def); + if (!obj) + goto cleanup; + + if (!(ret = virGetNWFilterBinding(conn, def->portdevname, def->filter))) + goto cleanup; + + if (virNWFilterInstantiateFilter(driver, def) < 0) { + virNWFilterBindingObjListRemove(driver->bindings, obj); + virObjectUnref(ret); + ret = NULL; + goto cleanup; + } + virNWFilterBindingObjSave(obj, driver->bindingDir); + + cleanup: + if (!obj) + virNWFilterBindingDefFree(def); + virNWFilterBindingObjEndAPI(&obj); + + return ret; +} + + +static int +nwfilterBindingDelete(virNWFilterBindingPtr binding) +{ + virNWFilterBindingObjPtr obj; + virNWFilterBindingDefPtr def; + int ret = -1; +
[...]

On Mon, Jun 18, 2018 at 04:59:50PM -0400, John Ferlan wrote:
On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
This allows the virsh commands nwfilter-binding-create and nwfilter-binding-delete to be used.
Note using these commands lets you delete filters that were previously created automatically by the virt drivers, or add filters for VM nics that were not there before. Generally it is expected these new APIs will only be used by virt drivers. It is the admin's responsibility to not shoot themselves in the foot.
Can't wait to see QE get ahold of this ;-)
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/nwfilter/nwfilter_driver.c | 79 ++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+)
I think with a couple of extra comments as described below, then this looks fine. Not sure how the other point regarding calling CreateXML from the ConfNWFilterInstantiate path (and the reload of load all configs) will be handled, but I'm sure it'll be something handled in patch 16 and 20, so the comment below is just the "tracing" I did while reviewing...
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 6bfb584b09..2b6856a36c 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -788,6 +788,83 @@ nwfilterBindingGetXMLDesc(virNWFilterBindingPtr binding, }
Because "not everyone" reads the commit message that added this, I think adding a few comments here and for BindingDelete to essentially indicate the same as the commit message would be good.
+static virNWFilterBindingPtr +nwfilterBindingCreateXML(virConnectPtr conn, + const char *xml, + unsigned int flags) +{ + virNWFilterBindingObjPtr obj; + virNWFilterBindingDefPtr def; + virNWFilterBindingPtr ret = NULL; + + virCheckFlags(0, NULL); + + def = virNWFilterBindingDefParseString(xml); + if (!def) + return NULL; + + if (virNWFilterBindingCreateXMLEnsureACL(conn, def) < 0) + goto cleanup; + + obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, def->portdevname); + if (obj) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Filter already present for NIC %s"), def->portdevname);
Recall my point from patch 16 regarding the existence of the filter. From certain paths it's OK if it exists but once this becomes functional for the subsequent patch via virDomainConfNWFilterInstantiate, then the issue from patch 16 moves to patch 20 (e.g. guest not restarting).
In this case, I think I'll change the calling code so that it first checks whether the filter exists or not, instead of unconditionally trying to recreate it. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Remove the callbacks that the nwfilter driver registers with the domain object config layer. Instead make the current helper methods call into the public API for creating/deleting nwfilter bindings. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/domain_nwfilter.c | 124 +++++++++++++++++++++---- src/conf/domain_nwfilter.h | 13 --- src/libvirt_private.syms | 1 - src/nwfilter/nwfilter_driver.c | 84 +++-------------- src/nwfilter/nwfilter_gentech_driver.c | 42 --------- src/nwfilter/nwfilter_gentech_driver.h | 4 - 6 files changed, 120 insertions(+), 148 deletions(-) diff --git a/src/conf/domain_nwfilter.c b/src/conf/domain_nwfilter.c index 7570e0ae83..ed45394918 100644 --- a/src/conf/domain_nwfilter.c +++ b/src/conf/domain_nwfilter.c @@ -28,45 +28,137 @@ #include "datatypes.h" #include "domain_conf.h" #include "domain_nwfilter.h" +#include "virnwfilterbindingdef.h" #include "virerror.h" +#include "viralloc.h" +#include "virstring.h" +#include "virlog.h" -#define VIR_FROM_THIS VIR_FROM_NWFILTER -static virDomainConfNWFilterDriverPtr nwfilterDriver; +VIR_LOG_INIT("conf.domain_nwfilter"); -void -virDomainConfNWFilterRegister(virDomainConfNWFilterDriverPtr driver) +#define VIR_FROM_THIS VIR_FROM_NWFILTER + +static virNWFilterBindingDefPtr +virNWFilterBindingDefForNet(const char *vmname, + const unsigned char *vmuuid, + virDomainNetDefPtr net) { - nwfilterDriver = driver; + virNWFilterBindingDefPtr ret; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + if (VIR_STRDUP(ret->ownername, vmname) < 0) + goto error; + + memcpy(ret->owneruuid, vmuuid, sizeof(ret->owneruuid)); + + if (VIR_STRDUP(ret->portdevname, net->ifname) < 0) + goto error; + + if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT && + VIR_STRDUP(ret->linkdevname, net->data.direct.linkdev) < 0) + goto error; + + ret->mac = net->mac; + + if (VIR_STRDUP(ret->filter, net->filter) < 0) + goto error; + + if (!(ret->filterparams = virNWFilterHashTableCreate(0))) + goto error; + + if (net->filterparams && + virNWFilterHashTablePutAll(net->filterparams, ret->filterparams) < 0) + goto error; + + return ret; + + error: + virNWFilterBindingDefFree(ret); + return NULL; } + int virDomainConfNWFilterInstantiate(const char *vmname, const unsigned char *vmuuid, virDomainNetDefPtr net) { - if (nwfilterDriver != NULL) - return nwfilterDriver->instantiateFilter(vmname, vmuuid, net); + virConnectPtr conn = virGetConnectNWFilter(); + virNWFilterBindingDefPtr def = NULL; + virNWFilterBindingPtr binding = NULL; + char *xml; + int ret = -1; + + VIR_DEBUG("vmname=%s portdev=%s filter=%s", + vmname, NULLSTR(net->ifname), NULLSTR(net->filter)); + + if (!conn) + goto cleanup; + + if (!(def = virNWFilterBindingDefForNet(vmname, vmuuid, net))) + goto cleanup; + + if (!(xml = virNWFilterBindingDefFormat(def))) + goto cleanup; + + if (!(binding = virNWFilterBindingCreateXML(conn, xml, 0))) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(xml); + virNWFilterBindingDefFree(def); + virObjectUnref(binding); + virObjectUnref(conn); + return ret; +} + + +static void +virDomainConfNWFilterTeardownImpl(virConnectPtr conn, + virDomainNetDefPtr net) +{ + virNWFilterBindingPtr binding; - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("No network filter driver available")); - return -1; + binding = virNWFilterBindingLookupByPortDev(conn, net->ifname); + if (!binding) + return; + + virNWFilterBindingDelete(binding); + + virObjectUnref(binding); } + void virDomainConfNWFilterTeardown(virDomainNetDefPtr net) { - if (nwfilterDriver != NULL) - nwfilterDriver->teardownFilter(net); + virConnectPtr conn = virGetConnectNWFilter(); + + if (!conn) + return; + + virDomainConfNWFilterTeardownImpl(conn, net); + + virObjectUnref(conn); } void virDomainConfVMNWFilterTeardown(virDomainObjPtr vm) { size_t i; + virConnectPtr conn = virGetConnectNWFilter(); + + if (!conn) + return; + + + for (i = 0; i < vm->def->nnets; i++) + virDomainConfNWFilterTeardownImpl(conn, vm->def->nets[i]); - if (nwfilterDriver != NULL) { - for (i = 0; i < vm->def->nnets; i++) - virDomainConfNWFilterTeardown(vm->def->nets[i]); - } + virObjectUnref(conn); } diff --git a/src/conf/domain_nwfilter.h b/src/conf/domain_nwfilter.h index 857cac6c2a..d2ebeff853 100644 --- a/src/conf/domain_nwfilter.h +++ b/src/conf/domain_nwfilter.h @@ -23,19 +23,6 @@ #ifndef DOMAIN_NWFILTER_H # define DOMAIN_NWFILTER_H -typedef int (*virDomainConfInstantiateNWFilter)(const char *vmname, - const unsigned char *vmuuid, - virDomainNetDefPtr net); -typedef void (*virDomainConfTeardownNWFilter)(virDomainNetDefPtr net); - -typedef struct { - virDomainConfInstantiateNWFilter instantiateFilter; - virDomainConfTeardownNWFilter teardownFilter; -} virDomainConfNWFilterDriver; -typedef virDomainConfNWFilterDriver *virDomainConfNWFilterDriverPtr; - -void virDomainConfNWFilterRegister(virDomainConfNWFilterDriverPtr driver); - int virDomainConfNWFilterInstantiate(const char *vmname, const unsigned char *vmuuid, virDomainNetDefPtr net); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ecc60dbe73..9676ad3d13 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -652,7 +652,6 @@ virDomainQemuMonitorEventStateRegisterID; # conf/domain_nwfilter.h virDomainConfNWFilterInstantiate; -virDomainConfNWFilterRegister; virDomainConfNWFilterTeardown; virDomainConfVMNWFilterTeardown; diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 2b6856a36c..26e6e76b3b 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -654,67 +654,6 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter, } -static int -nwfilterInstantiateFilter(const char *vmname, - const unsigned char *vmuuid, - virDomainNetDefPtr net) -{ - virNWFilterBindingObjPtr obj; - virNWFilterBindingDefPtr def; - int ret; - - obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, net->ifname); - if (obj) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Filter already present for NIC %s"), net->ifname); - virNWFilterBindingObjEndAPI(&obj); - return -1; - } - - if (!(def = virNWFilterBindingDefForNet(vmname, vmuuid, net))) - return -1; - - obj = virNWFilterBindingObjListAdd(driver->bindings, - def); - if (!obj) { - virNWFilterBindingDefFree(def); - return -1; - } - - ret = virNWFilterInstantiateFilter(driver, def); - - if (ret >= 0) - virNWFilterBindingObjSave(obj, driver->bindingDir); - else - virNWFilterBindingObjListRemove(driver->bindings, obj); - - virNWFilterBindingObjEndAPI(&obj); - - return ret; -} - - -static void -nwfilterTeardownFilter(virDomainNetDefPtr net) -{ - virNWFilterBindingObjPtr obj; - virNWFilterBindingDefPtr def; - if (!net->ifname) - return; - - obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, net->ifname); - if (!obj) - return; - - def = virNWFilterBindingObjGetDef(obj); - virNWFilterTeardownFilter(def); - virNWFilterBindingObjDelete(obj, driver->bindingDir); - - virNWFilterBindingObjListRemove(driver->bindings, obj); - virNWFilterBindingObjEndAPI(&obj); -} - - static virNWFilterBindingPtr nwfilterBindingLookupByPortDev(virConnectPtr conn, const char *portdev) @@ -725,8 +664,11 @@ nwfilterBindingLookupByPortDev(virConnectPtr conn, obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, portdev); - if (!obj) + if (!obj) { + virReportError(VIR_ERR_NO_NWFILTER_BINDING, + _("no nwfilter binding for port dev '%s'"), portdev); goto cleanup; + } def = virNWFilterBindingObjGetDef(obj); if (virNWFilterBindingLookupByPortDevEnsureACL(conn, def) < 0) @@ -773,8 +715,11 @@ nwfilterBindingGetXMLDesc(virNWFilterBindingPtr binding, obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, binding->portdev); - if (!obj) + if (!obj) { + virReportError(VIR_ERR_NO_NWFILTER_BINDING, + _("no nwfilter binding for port dev '%s'"), binding->portdev); goto cleanup; + } def = virNWFilterBindingObjGetDef(obj); if (virNWFilterBindingGetXMLDescEnsureACL(binding->conn, def) < 0) @@ -846,8 +791,11 @@ nwfilterBindingDelete(virNWFilterBindingPtr binding) int ret = -1; obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, binding->portdev); - if (!obj) + if (!obj) { + virReportError(VIR_ERR_NO_NWFILTER_BINDING, + _("no nwfilter binding for port dev '%s'"), binding->portdev); return -1; + } def = virNWFilterBindingObjGetDef(obj); if (virNWFilterBindingDeleteEnsureACL(binding->conn, def) < 0) @@ -908,13 +856,6 @@ static virStateDriver stateDriver = { .stateReload = nwfilterStateReload, }; - -static virDomainConfNWFilterDriver domainNWFilterDriver = { - .instantiateFilter = nwfilterInstantiateFilter, - .teardownFilter = nwfilterTeardownFilter, -}; - - int nwfilterRegister(void) { if (virRegisterConnectDriver(&nwfilterConnectDriver, false) < 0) @@ -923,6 +864,5 @@ int nwfilterRegister(void) return -1; if (virRegisterStateDriver(&stateDriver) < 0) return -1; - virDomainConfNWFilterRegister(&domainNWFilterDriver); return 0; } diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index d208d0188e..e5dea91f83 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -1082,45 +1082,3 @@ virNWFilterBuildAll(virNWFilterDriverStatePtr driver, } return ret; } - - -virNWFilterBindingDefPtr -virNWFilterBindingDefForNet(const char *vmname, - const unsigned char *vmuuid, - virDomainNetDefPtr net) -{ - virNWFilterBindingDefPtr ret; - - if (VIR_ALLOC(ret) < 0) - return NULL; - - if (VIR_STRDUP(ret->ownername, vmname) < 0) - goto error; - - memcpy(ret->owneruuid, vmuuid, sizeof(ret->owneruuid)); - - if (VIR_STRDUP(ret->portdevname, net->ifname) < 0) - goto error; - - if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT && - VIR_STRDUP(ret->linkdevname, net->data.direct.linkdev) < 0) - goto error; - - ret->mac = net->mac; - - if (VIR_STRDUP(ret->filter, net->filter) < 0) - goto error; - - if (!(ret->filterparams = virNWFilterHashTableCreate(0))) - goto error; - - if (net->filterparams && - virNWFilterHashTablePutAll(net->filterparams, ret->filterparams) < 0) - goto error; - - return ret; - - error: - virNWFilterBindingDefFree(ret); - return NULL; -} diff --git a/src/nwfilter/nwfilter_gentech_driver.h b/src/nwfilter/nwfilter_gentech_driver.h index 481fdd2413..2cd19c90fc 100644 --- a/src/nwfilter/nwfilter_gentech_driver.h +++ b/src/nwfilter/nwfilter_gentech_driver.h @@ -57,8 +57,4 @@ virHashTablePtr virNWFilterCreateVarHashmap(const char *macaddr, int virNWFilterBuildAll(virNWFilterDriverStatePtr driver, bool newFilters); -virNWFilterBindingDefPtr virNWFilterBindingDefForNet(const char *vmname, - const unsigned char *vmuuid, - virDomainNetDefPtr net); - #endif -- 2.17.0

On 06/14/2018 08:33 AM, Daniel P. Berrangé wrote:
Remove the callbacks that the nwfilter driver registers with the domain object config layer. Instead make the current helper methods call into the public API for creating/deleting nwfilter bindings.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/conf/domain_nwfilter.c | 124 +++++++++++++++++++++---- src/conf/domain_nwfilter.h | 13 --- src/libvirt_private.syms | 1 - src/nwfilter/nwfilter_driver.c | 84 +++-------------- src/nwfilter/nwfilter_gentech_driver.c | 42 --------- src/nwfilter/nwfilter_gentech_driver.h | 4 - 6 files changed, 120 insertions(+), 148 deletions(-)
I ran the more "aggressive" Avocado tests based on an old bz (https://bugzilla.redhat.com/show_bug.cgi?id=1034807) and got the following in my debug libvirtd output: 2018-06-15 15:46:18.683+0000: 18817: error : virNWFilterBindingLookupByPortDev:589 : portdev in virNWFilterBindingLookupByPortDev must not be NULL 2018-06-15 15:46:18.684+0000: 18817: error : virNWFilterBindingLookupByPortDev:589 : portdev in virNWFilterBindingLookupByPortDev must not be NULL The first thing the test does is remove the defined interface: <interface type='bridge'> <mac address='52:54:00:9a:9b:9c'/> <source bridge='virbr0'/> <model type='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> </interface> and then replaces/adds with a new interface: <interface type="bridge"> <mac address="52:54:00:9a:9b:9c" /> <source bridge="virbr0" /> <model type="virtio" /> <address bus="0x00" domain="0x0000" function="0x0" slot="0x03" type="pci" /> <filterref filter="allow-arp" /> </interface> for the test domain via device attach. Then 2 threads are started - 1 that continually starts/destroys the domain and 1 that continually removes/adds allow-arp. The actual logic in the test is busted because starting up a domain without a defined filter will fail and the thread doing the start/stop has no retry (try/except) logic. When I added the try/except logic and toned down the retry logic a bit I could get the test to pass with a few adjustments to the libvirt code here. Ironically, when the test goes too fast it caused my CPU's to get hot and generate a false positive failure because there were dmesg events related to core temperature above threshold. Anyway, I've noted a couple of places I think adjustments could/should be made - hopefully they make sense as I started looking "backwards" to see where things go introduced.
diff --git a/src/conf/domain_nwfilter.c b/src/conf/domain_nwfilter.c index 7570e0ae83..ed45394918 100644 --- a/src/conf/domain_nwfilter.c +++ b/src/conf/domain_nwfilter.c
[...]
+ int virDomainConfNWFilterInstantiate(const char *vmname, const unsigned char *vmuuid, virDomainNetDefPtr net)
Revisiting my comment from patch 13 - it is possible to enter here with net->filter being NULL. Is it worth returning 0 in that case under the assumption that there is no filter so that the callers then don't have to make that check? If portdev/ifname is NULL, not much is going to be found as well.
{ - if (nwfilterDriver != NULL) - return nwfilterDriver->instantiateFilter(vmname, vmuuid, net); + virConnectPtr conn = virGetConnectNWFilter(); + virNWFilterBindingDefPtr def = NULL; + virNWFilterBindingPtr binding = NULL; + char *xml; + int ret = -1; + + VIR_DEBUG("vmname=%s portdev=%s filter=%s", + vmname, NULLSTR(net->ifname), NULLSTR(net->filter));
Both being NULL probably is not a good thing - what filter for what portdev?
+ + if (!conn) + goto cleanup; +
Based on patch 16/19 comments and the thought above: if (!net->ifname || (binding = virNWFilterBindingLookupByPortDev(conn, net->ifname))) { ret = 0; goto cleanup; } where the !net->ifname may avoids the patch 13 comment issue and the ret = 0 when finding the binding handles the filter already loaded issue. The filter would be loaded for a running guest (after at least the second libvirtd restart) by virNWFilterBindingObjListLoadAllConfigs.
+ if (!(def = virNWFilterBindingDefForNet(vmname, vmuuid, net))) + goto cleanup; + + if (!(xml = virNWFilterBindingDefFormat(def))) + goto cleanup; + + if (!(binding = virNWFilterBindingCreateXML(conn, xml, 0))) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(xml); + virNWFilterBindingDefFree(def); + virObjectUnref(binding); + virObjectUnref(conn); + return ret; +} + + +static void +virDomainConfNWFilterTeardownImpl(virConnectPtr conn, + virDomainNetDefPtr net) +{ + virNWFilterBindingPtr binding;
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("No network filter driver available")); - return -1; + binding = virNWFilterBindingLookupByPortDev(conn, net->ifname); + if (!binding)
virNWFilterBindingLookupByPortDev will generate an error when there's no filter defined for @net (if you're running libvirtd in a debugger you see it). Shouldn't the call be guarded by a "if (!net->filter)"? if (!net->filter || !binding = vir...()) return; if not, then we probably should reset the last error since we're just returning (error as in [1]).
+ return; + + virNWFilterBindingDelete(binding); + + virObjectUnref(binding); }
+ void virDomainConfNWFilterTeardown(virDomainNetDefPtr net) { - if (nwfilterDriver != NULL) - nwfilterDriver->teardownFilter(net); + virConnectPtr conn = virGetConnectNWFilter(); + + if (!conn) + return; + + virDomainConfNWFilterTeardownImpl(conn, net); + + virObjectUnref(conn); }
may as well add the blank line here from ...
void virDomainConfVMNWFilterTeardown(virDomainObjPtr vm) { size_t i; + virConnectPtr conn = virGetConnectNWFilter(); + + if (!conn) + return; +
... here... or at least remove this extra blank line.
+ + for (i = 0; i < vm->def->nnets; i++) + virDomainConfNWFilterTeardownImpl(conn, vm->def->nets[i]);
- if (nwfilterDriver != NULL) { - for (i = 0; i < vm->def->nnets; i++) - virDomainConfNWFilterTeardown(vm->def->nets[i]); - } + virObjectUnref(conn); }
[...]
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 2b6856a36c..26e6e76b3b 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -654,67 +654,6 @@ nwfilterGetXMLDesc(virNWFilterPtr nwfilter, }
[...]
static virNWFilterBindingPtr nwfilterBindingLookupByPortDev(virConnectPtr conn, const char *portdev) @@ -725,8 +664,11 @@ nwfilterBindingLookupByPortDev(virConnectPtr conn,
obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, portdev); - if (!obj) + if (!obj) { + virReportError(VIR_ERR_NO_NWFILTER_BINDING, + _("no nwfilter binding for port dev '%s'"), portdev);454
[1] Adding this error here for someone running debug will cause those virDomainConfNWFilterTeardownImpl consumers w/o a filter to display this message. Of course removing it means the callers all have to add some sort of message. John
goto cleanup; + }
def = virNWFilterBindingObjGetDef(obj); if (virNWFilterBindingLookupByPortDevEnsureACL(conn, def) < 0)
[...]
participants (2)
-
Daniel P. Berrangé
-
John Ferlan