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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux