Re: [libvirt] [PATCH v3 19/20] nwfilter: wire up new APIs for creating and deleting nwfilter bindings
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é
> > ---
> > 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
>
> 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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é
> ---
> 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
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();
> +
> +return ret;
> +}
> +
> +
> +static int
> +nwfilterBindingDelete(virNWFilterBindingPtr binding)
> +{
> +virNWFilterBindingObjPtr obj;
> +virNWFilterBindingDefPtr def;
> +int ret = -1;
> +
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 19/20] nwfilter: wire up new APIs for creating and deleting nwfilter bindings
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.
Signed-off-by: Daniel P. Berrangé
---
src/nwfilter/nwfilter_driver.c | 79 ++
1 file changed, 79 insertions(+)
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,
}
+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);
+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();
+
+return ret;
+}
+
+
+static int
+nwfilterBindingDelete(virNWFilterBindingPtr binding)
+{
+virNWFilterBindingObjPtr obj;
+virNWFilterBindingDefPtr def;
+int ret = -1;
+
+obj = virNWFilterBindingObjListFindByPortDev(driver->bindings,
binding->portdev);
+if (!obj)
+return -1;
+
+def = virNWFilterBindingObjGetDef(obj);
+if (virNWFilterBindingDeleteEnsureACL(binding->conn, def) < 0)
+goto cleanup;
+
+virNWFilterTeardownFilter(def);
+virNWFilterBindingObjDelete(obj, driver->bindingDir);
+virNWFilterBindingObjListRemove(driver->bindings, obj);
+
+ret = 0;
+
+ cleanup:
+virNWFilterBindingObjEndAPI();
+return ret;
+}
+
+
static virNWFilterDriver nwfilterDriver = {
.name = "nwfilter",
.connectNumOfNWFilters = nwfilterConnectNumOfNWFilters, /* 0.8.0 */
@@ -801,6 +878,8 @@ static virNWFilterDriver nwfilterDriver = {
.nwfilterBindingLookupByPortDev = nwfilterBindingLookupByPortDev, /* 4.5.0
*/
.connectListAllNWFilterBindings = nwfilterConnectListAllNWFilterBindings,
/* 4.5.0 */
.nwfilterBindingGetXMLDesc = nwfilterBindingGetXMLDesc, /* 4.5.0 */
+.nwfilterBindingCreateXML = nwfilterBindingCreateXML, /* 4.5.0 */
+.nwfilterBindingDelete = nwfilterBindingDelete, /* 4.5.0 */
};
--
2.17.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
