Re: [libvirt-sandbox PATCH V2] Add filter support.

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

 



On Thu, Nov 21, 2013 at 03:27:46PM +0100, Christophe Fergeau wrote:
> Hey,
> 
> Here's an attempt at reviewing the rest of this patch. I'm not familiar at
> all with libvirt-sandbox, so I'm not in a really good position to review
> how this fits in the overall design of the library.
> 
> Christophe
> 
> On Tue, Nov 05, 2013 at 05:25:54PM -0800, Ian Main wrote:
> > This patch adds two new classes, filterref and filterref-parameter.
> > Network interfaces can now have an associated filter reference with any
> > number of filterref parameters.  Also added filter= option to
> > virt-sandbox tool.
> > 
> > V2:
> > 
> > - Changed set_filter to set_name and get_filter to get_name.
> > ---
> >  libvirt-sandbox/Makefile.am                        |   4 +
> >  .../libvirt-sandbox-builder-container.c            |  37 +++-
> >  libvirt-sandbox/libvirt-sandbox-builder-machine.c  |  36 ++++
> >  ...rt-sandbox-config-network-filterref-parameter.c | 205 ++++++++++++++++++++
> >  ...rt-sandbox-config-network-filterref-parameter.h |  75 ++++++++
> >  .../libvirt-sandbox-config-network-filterref.c     | 209 +++++++++++++++++++++
> >  .../libvirt-sandbox-config-network-filterref.h     |  75 ++++++++
> >  libvirt-sandbox/libvirt-sandbox-config-network.c   |  33 ++++
> >  libvirt-sandbox/libvirt-sandbox-config-network.h   |   4 +
> >  libvirt-sandbox/libvirt-sandbox-config.c           |  39 ++++
> >  libvirt-sandbox/libvirt-sandbox.h                  |   3 +
> >  libvirt-sandbox/libvirt-sandbox.sym                |  14 ++
> >  12 files changed, 733 insertions(+), 1 deletion(-)
> >  create mode 100644 libvirt-sandbox/libvirt-sandbox-config-network-filterref-parameter.c
> >  create mode 100644 libvirt-sandbox/libvirt-sandbox-config-network-filterref-parameter.h
> >  create mode 100644 libvirt-sandbox/libvirt-sandbox-config-network-filterref.c
> >  create mode 100644 libvirt-sandbox/libvirt-sandbox-config-network-filterref.h
> > 
> > diff --git a/libvirt-sandbox/libvirt-sandbox-builder-container.c b/libvirt-sandbox/libvirt-sandbox-builder-container.c
> > index 43ee5ef..db70403 100644
> > --- a/libvirt-sandbox/libvirt-sandbox-builder-container.c
> > +++ b/libvirt-sandbox/libvirt-sandbox-builder-container.c
> > @@ -319,11 +319,12 @@ static gboolean gvir_sandbox_builder_container_construct_devices(GVirSandboxBuil
> >      g_list_foreach(mounts, (GFunc)g_object_unref, NULL);
> >      g_list_free(mounts);
> >  
> > -
> >      tmp = networks = gvir_sandbox_config_get_networks(config);
> >      while (tmp) {
> >          const gchar *source, *mac;
> >          GVirSandboxConfigNetwork *network = GVIR_SANDBOX_CONFIG_NETWORK(tmp->data);
> > +        GVirSandboxConfigNetworkFilterref *filterref;
> > +        GVirConfigDomainInterfaceFilterref *glib_fref;
> >  
> >          iface = gvir_config_domain_interface_network_new();
> >          source = gvir_sandbox_config_network_get_source(network);
> > @@ -339,6 +340,40 @@ static gboolean gvir_sandbox_builder_container_construct_devices(GVirSandboxBuil
> >  
> >          gvir_config_domain_add_device(domain,
> >                                        GVIR_CONFIG_DOMAIN_DEVICE(iface));
> > +
> > +        filterref = gvir_sandbox_config_network_get_filterref(network);
> > +        if (filterref) {
> > +            GList *param_iter, *parameters;
> > +            const gchar *fref_name = gvir_sandbox_config_network_filterref_get_name(filterref);
> > +            glib_fref = gvir_config_domain_interface_filterref_new();
> > +            gvir_config_domain_interface_filterref_set_name(glib_fref, fref_name);
> > +            param_iter = parameters = gvir_sandbox_config_network_filterref_get_parameters(filterref);
> > +            while (param_iter) {
> > +                const gchar *name;
> > +                const gchar *value;
> > +                GVirSandboxConfigNetworkFilterrefParameter *param = GVIR_SANDBOX_CONFIG_NETWORK_FILTERREF_PARAMETER(param_iter->data);
> > +                GVirConfigDomainInterfaceFilterrefParameter *glib_param;
> > +
> > +                name = gvir_sandbox_config_network_filterref_parameter_get_name(param);
> > +                value = gvir_sandbox_config_network_filterref_parameter_get_value(param);
> > +
> > +                glib_param = gvir_config_domain_interface_filterref_parameter_new();
> > +                gvir_config_domain_interface_filterref_parameter_set_name(glib_param, name);
> > +                gvir_config_domain_interface_filterref_parameter_set_value(glib_param, value);
> > +
> > +                gvir_config_domain_interface_filterref_add_parameter(glib_fref, glib_param);
> > +                g_object_unref(glib_param);
> > +
> > +                param_iter = param_iter->next;
> > +            }
> > +
> > +            g_list_foreach(parameters, (GFunc)g_object_unref, NULL);
> > +            g_list_free(parameters);
> > +
> > +            gvir_config_domain_interface_set_filterref(GVIR_CONFIG_DOMAIN_INTERFACE(iface), glib_fref);
> > +            g_object_unref(glib_fref);
> > +        }
> > +
> >          g_object_unref(iface);
> >  
> >          tmp = tmp->next;
> > diff --git a/libvirt-sandbox/libvirt-sandbox-builder-machine.c b/libvirt-sandbox/libvirt-sandbox-builder-machine.c
> > index a8c5d8c..0cfedc7 100644
> > --- a/libvirt-sandbox/libvirt-sandbox-builder-machine.c
> > +++ b/libvirt-sandbox/libvirt-sandbox-builder-machine.c
> > @@ -577,6 +577,8 @@ static gboolean gvir_sandbox_builder_machine_construct_devices(GVirSandboxBuilde
> >      while (tmp) {
> >          const gchar *source, *mac;
> >          GVirSandboxConfigNetwork *network = GVIR_SANDBOX_CONFIG_NETWORK(tmp->data);
> > +        GVirSandboxConfigNetworkFilterref *filterref;
> > +        GVirConfigDomainInterfaceFilterref *glib_fref;
> >  
> >          source = gvir_sandbox_config_network_get_source(network);
> >          if (source) {
> > @@ -596,6 +598,40 @@ static gboolean gvir_sandbox_builder_machine_construct_devices(GVirSandboxBuilde
> >  
> >          gvir_config_domain_add_device(domain,
> >                                        GVIR_CONFIG_DOMAIN_DEVICE(iface));
> > +
> > +        filterref = gvir_sandbox_config_network_get_filterref(network);
> > +        if (filterref) {
> > +            GList *param_iter, *parameters;
> > +            const gchar *fref_name = gvir_sandbox_config_network_filterref_get_name(filterref);
> > +            glib_fref = gvir_config_domain_interface_filterref_new();
> > +            gvir_config_domain_interface_filterref_set_name(glib_fref, fref_name);
> > +            param_iter = parameters = gvir_sandbox_config_network_filterref_get_parameters(filterref);
> > +            while (param_iter) {
> > +                const gchar *name;
> > +                const gchar *value;
> > +                GVirSandboxConfigNetworkFilterrefParameter *param = GVIR_SANDBOX_CONFIG_NETWORK_FILTERREF_PARAMETER(param_iter->data);
> > +                GVirConfigDomainInterfaceFilterrefParameter *glib_param;
> > +
> > +                name = gvir_sandbox_config_network_filterref_parameter_get_name(param);
> > +                value = gvir_sandbox_config_network_filterref_parameter_get_value(param);
> > +
> > +                glib_param = gvir_config_domain_interface_filterref_parameter_new();
> > +                gvir_config_domain_interface_filterref_parameter_set_name(glib_param, name);
> > +                gvir_config_domain_interface_filterref_parameter_set_value(glib_param, value);
> > +
> > +                gvir_config_domain_interface_filterref_add_parameter(glib_fref, glib_param);
> > +                g_object_unref(glib_param);
> > +
> > +                param_iter = param_iter->next;
> > +            }
> > +
> > +            g_list_foreach(parameters, (GFunc)g_object_unref, NULL);
> > +            g_list_free(parameters);
> > +
> > +            gvir_config_domain_interface_set_filterref(iface, glib_fref);
> > +            g_object_unref(glib_fref);
> > +        }
> > +
> 
> This seems to be exactly the same code as what you added in
> libvirt-sandbox-builder-container.c, this should go into a helper
> (gvir_sandbox_config_network_filterref_get_config(), or
> gvir_sandbox_builder_get_network_filterref_config() I think).

Yes, I'm actually not entirely sure how to handle that.. there is a lot
of duplicate code between the two but a few things that are different.
I think we might have to refactor that part but I think maybe we need a
builder_common.c or something.

	Ian

[snip]

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