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
