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

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(a)redhat.com&gt; > --- > 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(a)redhat.com&gt; 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).