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