[libvirt] [PATCH v3 19/20] nwfilter: wire up new APIs for creating and deleting nwfilter bindings

[John Ferlan]

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 at 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 at 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;
> +

[...]