On 05/15/2018 01:43 PM, 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@xxxxxxxxxx>
> ---
> src/conf/virnwfilterobj.h | 4 ++
> src/nwfilter/nwfilter_driver.c | 83 ++++++++++++++++++++++++----------
> 2 files changed, 64 insertions(+), 23 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 b57e5dd00d..67e07d2dec 100644
> --- a/src/nwfilter/nwfilter_driver.c
> +++ b/src/nwfilter/nwfilter_driver.c
[...]
> @@ -647,13 +656,38 @@ 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;
> +
> + obj = virNWFilterBindingObjListAdd(driver->bindings,
> + def);
> + if (!obj) {
> + virNWFilterBindingDefFree(def);
> return -1;
> - ret = virNWFilterInstantiateFilter(driver, binding);
> - virNWFilterBindingDefFree(binding);
> + }
> + def = NULL;
> +
> + ret = virNWFilterInstantiateFilter(driver, obj->def);
If _virNWFilterBindingObj becomes private, then this is where that fetch
of @objdef from non nwfilter and domain drivers is done. So rather than
obj->def - s/b an accessor.
> +
> + if (ret < 0)
> + virNWFilterBindingObjListRemove(driver->bindings, obj);
> +
> + virNWFilterBindingObjSave(obj, driver->bindingDir);
if ret < 0, do we really want to Save?
> +
> + virNWFilterBindingObjEndAPI(&obj);
> +
> return ret;
> }
>
> @@ -661,16 +695,19 @@ 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,
> - };
> - if ((net->ifname) && (net->filter))
> - virNWFilterTeardownFilter(&binding);
> + virNWFilterBindingObjPtr obj;
> + if (!net->ifname)
> + return;
> +
> + obj = virNWFilterBindingObjListFindByPortDev(driver->bindings, net->ifname);
> + if (!obj)
> + return;
> +
> + virNWFilterTeardownFilter(obj->def);
Again use of accessor for @obj
> + virNWFilterBindingObjDelete(obj, driver->bindingDir);
> +
> + virNWFilterBindingObjListRemove(driver->bindings, obj);
> + virNWFilterBindingObjEndAPI(&obj);
> }
>
>
>
I think this is essentially clean - the use of accessors is an easy
change and I trust you'd to the right thing if instantiate fails w/r/t
whether to save the filter obj...
Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
