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

[Daniel P. Berrangé]

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 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).

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 :|