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

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

 



Hey,

Here is a review for the 2 new gobjects you introduced (given the size of
the patch, I'd tend to split the introduction of these 2 new objects in a
separate patch, this would make the patch with changes to the existing code
much smaller).

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/Makefile.am b/libvirt-sandbox/Makefile.am
> index 0882490..4de8766 100644
> --- a/libvirt-sandbox/Makefile.am
> +++ b/libvirt-sandbox/Makefile.am
> @@ -57,6 +57,8 @@ SANDBOX_HEADER_FILES = \
>  			libvirt-sandbox-config.h \
>  			libvirt-sandbox-config-network.h \
>  			libvirt-sandbox-config-network-address.h \
> +			libvirt-sandbox-config-network-filterref.h \
> +			libvirt-sandbox-config-network-filterref-parameter.h \
>  			libvirt-sandbox-config-network-route.h \
>  			libvirt-sandbox-config-mount.h \
>  			libvirt-sandbox-config-mount-file.h \
> @@ -85,6 +87,8 @@ SANDBOX_SOURCE_FILES = \
>  			libvirt-sandbox-config.c \
>  			libvirt-sandbox-config-network.c \
>  			libvirt-sandbox-config-network-address.c \
> +			libvirt-sandbox-config-network-filterref.c \
> +			libvirt-sandbox-config-network-filterref-parameter.c \
>  			libvirt-sandbox-config-network-route.c \
>  			libvirt-sandbox-config-mount.c \
>  			libvirt-sandbox-config-mount-file.c \
> diff --git a/libvirt-sandbox/libvirt-sandbox-config-network-filterref-parameter.c b/libvirt-sandbox/libvirt-sandbox-config-network-filterref-parameter.c
> new file mode 100644
> index 0000000..c1ed941
> --- /dev/null
> +++ b/libvirt-sandbox/libvirt-sandbox-config-network-filterref-parameter.c
> @@ -0,0 +1,205 @@
> +/*
> + * libvirt-sandbox-config-network-filterref-parameter.c: libvirt sandbox configuration
> + *
> + * Copyright (C) 2013 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + * Author: Ian Main <imain@xxxxxxxxxx>
> + */
> +
> +#include <config.h>
> +#include <string.h>
> +
> +#include "libvirt-sandbox/libvirt-sandbox.h"
> +
> +/**
> + * SECTION: libvirt-sandbox-config-network-filterref-parameter
> + * @short_description: Set parameters for a filter reference.
> + * @include: libvirt-sandbox/libvirt-sandbox.h
> + *
> + * Provides an object to store filter parameter name and value.
> + *
> + * The GVirSandboxConfigNetworkFilterrefParameter object stores a
> + * name and value required to set a single parameter of a filter reference.
> + */
> +
> +#define GVIR_SANDBOX_CONFIG_NETWORK_FILTERREF_PARAMETER_GET_PRIVATE(obj)                         \
> +        (G_TYPE_INSTANCE_GET_PRIVATE((obj), GVIR_SANDBOX_TYPE_CONFIG_NETWORK_FILTERREF_PARAMETER, GVirSandboxConfigNetworkFilterrefParameterPrivate))
> +
> +struct _GVirSandboxConfigNetworkFilterrefParameterPrivate
> +{
> +    gchar *name;
> +    gchar *value;
> +};
> +
> +G_DEFINE_TYPE(GVirSandboxConfigNetworkFilterrefParameter, gvir_sandbox_config_network_filterref_parameter, G_TYPE_OBJECT);
> +
> +
> +enum {
> +    PROP_0,
> +    PROP_NAME,
> +    PROP_VALUE,
> +};
> +
> +enum {
> +    LAST_SIGNAL
> +};
> +
> +//static gint signals[LAST_SIGNAL];
> +
> +
> +static void gvir_sandbox_config_network_filterref_parameter_get_property(GObject *object,
> +                                                                         guint prop_id,
> +                                                                         GValue *value,
> +                                                                         GParamSpec *pspec)
> +{
> +    GVirSandboxConfigNetworkFilterrefParameter *config = GVIR_SANDBOX_CONFIG_NETWORK_FILTERREF_PARAMETER(object);
> +    GVirSandboxConfigNetworkFilterrefParameterPrivate *priv = config->priv;
> +
> +    switch (prop_id) {
> +    case PROP_NAME:
> +        g_value_set_string(value, priv->name);
> +        break;
> +
> +    case PROP_VALUE:
> +        g_value_set_string(value, priv->value);
> +        break;
> +
> +    default:
> +        G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> +    }
> +}
> +
> +
> +static void gvir_sandbox_config_network_filterref_parameter_set_property(GObject *object,
> +                                                                         guint prop_id,
> +                                                                         const GValue *value,
> +                                                                         GParamSpec *pspec)
> +{
> +    GVirSandboxConfigNetworkFilterrefParameter *config = GVIR_SANDBOX_CONFIG_NETWORK_FILTERREF_PARAMETER(object);
> +    GVirSandboxConfigNetworkFilterrefParameterPrivate *priv = config->priv;
> +
> +    switch (prop_id) {
> +    case PROP_NAME:
> +        g_free(priv->name);
> +        priv->name = g_value_dup_string(value);
> +        break;
> +
> +    case PROP_VALUE:
> +        g_free(priv->value);
> +        priv->value = g_value_dup_string(value);

You could call the setters here

> +        break;
> +
> +    default:
> +        G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> +    }
> +}
> +
> +
> +static void gvir_sandbox_config_network_filterref_parameter_finalize(GObject *object)
> +{
> +    GVirSandboxConfigNetworkFilterrefParameter *config = GVIR_SANDBOX_CONFIG_NETWORK_FILTERREF_PARAMETER(object);
> +    GVirSandboxConfigNetworkFilterrefParameterPrivate *priv = config->priv;
> +
> +    g_free(priv->name);
> +    g_free(priv->value);
> +
> +    G_OBJECT_CLASS(gvir_sandbox_config_network_filterref_parameter_parent_class)->finalize(object);
> +}
> +
> +
> +static void gvir_sandbox_config_network_filterref_parameter_class_init(GVirSandboxConfigNetworkFilterrefParameterClass *klass)
> +{
> +    GObjectClass *object_class = G_OBJECT_CLASS(klass);
> +
> +    object_class->finalize = gvir_sandbox_config_network_filterref_parameter_finalize;
> +    object_class->get_property = gvir_sandbox_config_network_filterref_parameter_get_property;
> +    object_class->set_property = gvir_sandbox_config_network_filterref_parameter_set_property;
> +
> +    g_object_class_install_property(object_class,
> +                                    PROP_NAME,
> +                                    g_param_spec_string("name",
> +                                                        "Name",
> +                                                        "Name of parameter",
> +                                                        NULL,
> +                                                        G_PARAM_READABLE |
> +                                                        G_PARAM_WRITABLE |
> +                                                        G_PARAM_STATIC_NAME |
> +                                                        G_PARAM_STATIC_NICK |
> +                                                        G_PARAM_STATIC_BLURB));

Can be G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS

> +
> +    g_object_class_install_property(object_class,
> +                                    PROP_VALUE,
> +                                    g_param_spec_string("value",
> +                                                        "Value",
> +                                                        "Value of parameter",
> +                                                        NULL,
> +                                                        G_PARAM_READABLE |
> +                                                        G_PARAM_WRITABLE |
> +                                                        G_PARAM_STATIC_NAME |
> +                                                        G_PARAM_STATIC_NICK |
> +                                                        G_PARAM_STATIC_BLURB));

Ditto

> +
> +    g_type_class_add_private(klass, sizeof(GVirSandboxConfigNetworkFilterrefParameterPrivate));
> +}
> +
> +
> +static void gvir_sandbox_config_network_filterref_parameter_init(GVirSandboxConfigNetworkFilterrefParameter *param)
> +{
> +    param->priv = GVIR_SANDBOX_CONFIG_NETWORK_FILTERREF_PARAMETER_GET_PRIVATE(param);
> +}
> +
> +
> +/**
> + * gvir_sandbox_config_network_filterref_parameter_new:
> + *
> + * Create a new network config with DHCP enabled
> + *
> + * Returns: (transfer full): a new sandbox network object
> + */
> +GVirSandboxConfigNetworkFilterrefParameter *gvir_sandbox_config_network_filterref_parameter_new(void)
> +{
> +    return GVIR_SANDBOX_CONFIG_NETWORK_FILTERREF_PARAMETER(g_object_new(GVIR_SANDBOX_TYPE_CONFIG_NETWORK_FILTERREF_PARAMETER,
> +                                                                        NULL));
> +}
> +
> +void gvir_sandbox_config_network_filterref_parameter_set_name(GVirSandboxConfigNetworkFilterrefParameter *param,
> +                                                                const gchar *name)
> +{

You could add a
g_return_val_if_fail(GVIR_SANDBOX_IS_CONFIG_NETWORK_FILTERREF_PARAMETER(param);
(iirc this also does return for NULL param)
Same comment applies to the other public methods

> +    GVirSandboxConfigNetworkFilterrefParameterPrivate *priv = param->priv;
> +    g_free(priv->name);
> +    priv->name = g_strdup(name);
> +}
> +
> +const gchar *gvir_sandbox_config_network_filterref_parameter_get_name(GVirSandboxConfigNetworkFilterrefParameter *param)
> +{
> +    GVirSandboxConfigNetworkFilterrefParameterPrivate *priv = param->priv;
> +    return priv->name;
> +}
> +
> +void gvir_sandbox_config_network_filterref_parameter_set_value(GVirSandboxConfigNetworkFilterrefParameter *param,
> +                                                                const gchar *value)
> +{
> +    GVirSandboxConfigNetworkFilterrefParameterPrivate *priv = param->priv;
> +    g_free(priv->value);
> +    priv->value = g_strdup(value);
> +}
> +
> +const gchar *gvir_sandbox_config_network_filterref_parameter_get_value(GVirSandboxConfigNetworkFilterrefParameter *param)
> +{
> +    GVirSandboxConfigNetworkFilterrefParameterPrivate *priv = param->priv;
> +    return priv->value;
> +}
> diff --git a/libvirt-sandbox/libvirt-sandbox-config-network-filterref-parameter.h b/libvirt-sandbox/libvirt-sandbox-config-network-filterref-parameter.h
> new file mode 100644
> index 0000000..f40895f
> --- /dev/null
> +++ b/libvirt-sandbox/libvirt-sandbox-config-network-filterref-parameter.h
> @@ -0,0 +1,75 @@
> +/*
> + * libvirt-sandbox-config-network-filterref-parameter.h: libvirt sandbox configuration
> + *
> + * Copyright (C) 2013 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + * Author: Ian Main <imain@xxxxxxxxxx>
> + */
> +
> +#if !defined(__LIBVIRT_SANDBOX_H__) && !defined(LIBVIRT_SANDBOX_BUILD)
> +#error "Only <libvirt-sandbox/libvirt-sandbox.h> can be included directly."
> +#endif
> +
> +#ifndef __LIBVIRT_SANDBOX_CONFIG_NETWORK_FILTERREF_PARAMETER_H__
> +#define __LIBVIRT_SANDBOX_CONFIG_NETWORK_FILTERREF_PARAMETER_H__
> +
> +G_BEGIN_DECLS
> +
> +#define GVIR_SANDBOX_TYPE_CONFIG_NETWORK_FILTERREF_PARAMETER            (gvir_sandbox_config_network_filterref_parameter_get_type ())
> +#define GVIR_SANDBOX_CONFIG_NETWORK_FILTERREF_PARAMETER(obj)            (G_TYPE_CHECK_INSTANCE_CAST ((obj), GVIR_SANDBOX_TYPE_CONFIG_NETWORK_FILTERREF_PARAMETER, GVirSandboxConfigNetworkFilterrefParameter))
> +#define GVIR_SANDBOX_CONFIG_NETWORK_FILTERREF_PARAMETER_CLASS(klass)    (G_TYPE_CHECK_CLASS_CAST ((klass), GVIR_SANDBOX_TYPE_CONFIG_NETWORK_FILTERREF_PARAMETER, GVirSandboxConfigNetworkFilterrefParameterClass))
> +#define GVIR_SANDBOX_IS_CONFIG_NETWORK_FILTERREF_PARAMETER(obj)         (G_TYPE_CHECK_INSTANCE_TYPE ((obj), GVIR_SANDBOX_TYPE_CONFIG_NETWORK_FILTERREF_PARAMETER))
> +#define GVIR_SANDBOX_IS_CONFIG_NETWORK_FILTERREF_PARAMETER_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), GVIR_SANDBOX_TYPE_CONFIG_NETWORK_FILTERREF_PARAMETER))
> +#define GVIR_SANDBOX_CONFIG_NETWORK_FILTERREF_PARAMETER_GET_CLASS(obj)  (G_TYPE_INSTANCE_GET_CLASS ((obj), GVIR_SANDBOX_TYPE_CONFIG_NETWORK_FILTERREF_PARAMETER, GVirSandboxConfigNetworkFilterrefParameterClass))
> +
> +#define GVIR_SANDBOX_TYPE_CONFIG_NETWORK_FILTERREF_PARAMETER_HANDLE      (gvir_sandbox_config_network_filterref_parameter_handle_get_type ())

I don't think this HANDLE #define is needed

> +
> +typedef struct _GVirSandboxConfigNetworkFilterrefParameter GVirSandboxConfigNetworkFilterrefParameter;
> +typedef struct _GVirSandboxConfigNetworkFilterrefParameterPrivate GVirSandboxConfigNetworkFilterrefParameterPrivate;
> +typedef struct _GVirSandboxConfigNetworkFilterrefParameterClass GVirSandboxConfigNetworkFilterrefParameterClass;
> +
> +struct _GVirSandboxConfigNetworkFilterrefParameter
> +{
> +    GObject parent;
> +
> +    GVirSandboxConfigNetworkFilterrefParameterPrivate *priv;
> +
> +    /* Do not add fields to this struct */
> +};
> +
> +struct _GVirSandboxConfigNetworkFilterrefParameterClass
> +{
> +    GObjectClass parent_class;
> +
> +    gpointer padding[LIBVIRT_SANDBOX_CLASS_PADDING];
> +};
> +
> +GType gvir_sandbox_config_network_filterref_parameter_get_type(void);
> +
> +GVirSandboxConfigNetworkFilterrefParameter *gvir_sandbox_config_network_filterref_parameter_new(void);
> +
> +void gvir_sandbox_config_network_filterref_parameter_set_name(GVirSandboxConfigNetworkFilterrefParameter *param,
> +                                                              const gchar *name);
> +const gchar *gvir_sandbox_config_network_filterref_parameter_get_name(GVirSandboxConfigNetworkFilterrefParameter *param);
> +
> +void gvir_sandbox_config_network_filterref_parameter_set_value(GVirSandboxConfigNetworkFilterrefParameter *param,
> +                                                               const gchar *value);
> +const gchar *gvir_sandbox_config_network_filterref_parameter_get_value(GVirSandboxConfigNetworkFilterrefParameter *param);
> +
> +G_END_DECLS
> +
> +#endif /* __LIBVIRT_SANDBOX_CONFIG_NETWORK_FILTERREF_PARAMETER_H__ */
> diff --git a/libvirt-sandbox/libvirt-sandbox-config-network-filterref.c b/libvirt-sandbox/libvirt-sandbox-config-network-filterref.c
> new file mode 100644
> index 0000000..71cec31
> --- /dev/null
> +++ b/libvirt-sandbox/libvirt-sandbox-config-network-filterref.c
> @@ -0,0 +1,209 @@
> +/*
> + * libvirt-sandbox-config-mount.c: libvirt sandbox configuration

s/mount/network-filterref.c

> + *
> + * Copyright (C) 2013 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + * Author: Ian Main <imain@xxxxxxxxxx>
> + */
> +
> +#include <config.h>
> +#include <string.h>
> +
> +#include "libvirt-sandbox/libvirt-sandbox.h"
> +
> +/**
> + * SECTION: libvirt-sandbox-config-network-filterref
> + * @short_description: Add a network filter to a network interface.
> + * @include: libvirt-sandbox/libvirt-sandbox.h
> + * @see_aloso: #GVirSandboxConfig
> + *
> + * Provides an object to store the name of the filter reference.
> + *
> + * The GVirSandboxConfigNetworkFilterref object stores the name of the filter
> + * references associated with a network interface.
> + */
> +
> +#define GVIR_SANDBOX_CONFIG_NETWORK_FILTERREF_GET_PRIVATE(obj)                         \
> +        (G_TYPE_INSTANCE_GET_PRIVATE((obj), GVIR_SANDBOX_TYPE_CONFIG_NETWORK_FILTERREF, GVirSandboxConfigNetworkFilterrefPrivate))
> +
> +struct _GVirSandboxConfigNetworkFilterrefPrivate
> +{
> +    gchar *filter;
> +    GList *parameters;
> +};
> +
> +G_DEFINE_TYPE(GVirSandboxConfigNetworkFilterref, gvir_sandbox_config_network_filterref, G_TYPE_OBJECT);
> +
> +
> +enum {
> +    PROP_0,
> +    PROP_FILTER
> +};
> +
> +enum {
> +    LAST_SIGNAL
> +};
> +
> +//static gint signals[LAST_SIGNAL];
> +
> +static void gvir_sandbox_config_network_filterref_get_property(GObject *object,
> +                                                               guint prop_id,
> +                                                               GValue *value,
> +                                                               GParamSpec *pspec)
> +{
> +    GVirSandboxConfigNetworkFilterref *config = GVIR_SANDBOX_CONFIG_NETWORK_FILTERREF(object);
> +    GVirSandboxConfigNetworkFilterrefPrivate *priv = config->priv;
> +
> +    switch (prop_id) {
> +    case PROP_FILTER:
> +        g_value_set_string(value, priv->filter);
> +        break;
> +
> +    default:
> +        G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> +    }
> +}
> +
> +
> +static void gvir_sandbox_config_network_filterref_set_property(GObject *object,
> +                                                             guint prop_id,
> +                                                             const GValue *value,
> +                                                             GParamSpec *pspec)
> +{
> +    GVirSandboxConfigNetworkFilterref *config = GVIR_SANDBOX_CONFIG_NETWORK_FILTERREF(object);
> +    GVirSandboxConfigNetworkFilterrefPrivate *priv = config->priv;
> +
> +    switch (prop_id) {
> +    case PROP_FILTER:
> +        g_free(priv->filter);
> +        priv->filter = g_value_dup_string(value);

This could call gvir_sandbox_config_network_filterref_set_name()

> +        break;
> +
> +    default:
> +        G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> +    }
> +}
> +
> +
> +
> +static void gvir_sandbox_config_network_filterref_finalize(GObject *object)
> +{
> +    GVirSandboxConfigNetworkFilterref *config = GVIR_SANDBOX_CONFIG_NETWORK_FILTERREF(object);
> +    GVirSandboxConfigNetworkFilterrefPrivate *priv = config->priv;
> +
> +    g_free(priv->filter);
> +    g_list_foreach(priv->parameters, (GFunc)g_object_unref, NULL);

A g_list_free(priv->parameters); is missing here

> +
> +    G_OBJECT_CLASS(gvir_sandbox_config_network_filterref_parent_class)->finalize(object);
> +}
> +
> +
> +static void gvir_sandbox_config_network_filterref_class_init(GVirSandboxConfigNetworkFilterrefClass *klass)
> +{
> +    GObjectClass *object_class = G_OBJECT_CLASS(klass);
> +
> +    object_class->finalize = gvir_sandbox_config_network_filterref_finalize;
> +    object_class->get_property = gvir_sandbox_config_network_filterref_get_property;
> +    object_class->set_property = gvir_sandbox_config_network_filterref_set_property;
> +
> +    g_object_class_install_property(object_class,
> +                                    PROP_FILTER,
> +                                    g_param_spec_string("filter",
> +                                                        "Filter name",
> +                                                        "The filter reference name",
> +                                                        NULL,
> +                                                        G_PARAM_READABLE |
> +                                                        G_PARAM_WRITABLE |

This could be G_PARAM_READWRITE

> +                                                        G_PARAM_CONSTRUCT_ONLY |
> +                                                        G_PARAM_STATIC_NAME |
> +                                                        G_PARAM_STATIC_NICK |
> +                                                        G_PARAM_STATIC_BLURB));

and these 3 could be G_PARAM_STATIC_STRINGS


> +
> +    g_type_class_add_private(klass, sizeof(GVirSandboxConfigNetworkFilterrefPrivate));
> +}
> +
> +/**
> + * gvir_sandbox_config_network_filterref_new:
> + *
> + * Create a new network filterref config.
> + *
> + * Returns: (transfer full): a new sandbox network_filterref object
> + */
> +GVirSandboxConfigNetworkFilterref *gvir_sandbox_config_network_filterref_new(void)
> +{
> +    return GVIR_SANDBOX_CONFIG_NETWORK_FILTERREF(g_object_new(GVIR_SANDBOX_TYPE_CONFIG_NETWORK_FILTERREF,
> +                                                              NULL));

The filter name is marked 'construct-only' but you are not passing it when
you create the object. This means it will never be possible to set it on
the object.

> +}
> +
> +
> +static void gvir_sandbox_config_network_filterref_init(GVirSandboxConfigNetworkFilterref *config)
> +{
> +    config->priv = GVIR_SANDBOX_CONFIG_NETWORK_FILTERREF_GET_PRIVATE(config);
> +}
> +
> +
> +/**
> + * gvir_sandbox_config_network_filterref_get_name:
> + * @config: (transfer none): the network filter reference name
> + *
> + * Retrieves the network filter reference name.
> + *
> + * Returns: (transfer none): the network filter reference name.
> + */
> +const gchar *gvir_sandbox_config_network_filterref_get_name(GVirSandboxConfigNetworkFilterref *config)

maybe 'filter' rather than 'config' would be a more explicit name as an arg
throughout this file? (no strong feeling either way, just a suggestion,
feel free to ignore)

> +{
> +    GVirSandboxConfigNetworkFilterrefPrivate *priv = config->priv;
> +    return priv->filter;
> +}

I don't know what the convention is in libvirt-sandbox, but
spice-gtk/libvirt-glib try to protect public API calls with
g_return_val_if_fail(GVIR_SANDBOX_IS_CONFIG_NETWORK_FILTER(config), NULL);
so that invalid objects are caught early on

> +
> +void gvir_sandbox_config_network_filterref_set_name(GVirSandboxConfigNetworkFilterref *config,
> +                                                    const gchar *name)
> +{
> +    GVirSandboxConfigNetworkFilterrefPrivate *priv = config->priv;
> +    g_free(priv->filter);
> +    priv->filter = g_strdup(name);
> +}

Having this setter is not consistent with the 'name' property being
construct-only, I guess the G_PARAM_CONSTRUCT_ONLY should be removed from
the property creation.

> +
> +/**
> + * gvir_sandbox_config_network_filterref_add_parameter:
> + * @filter: (transfer none): the network filter reference.
> + * @param: (transfer none): the filter parameter
> + *
> + * Add a parameter to a network filter reference.
> + */
> +void gvir_sandbox_config_network_filterref_add_parameter(GVirSandboxConfigNetworkFilterref *filter,
> +                                                         GVirSandboxConfigNetworkFilterrefParameter *param)
> +{
> +    GVirSandboxConfigNetworkFilterrefPrivate *priv = filter->priv;
> +    priv->parameters = g_list_append(priv->parameters, g_object_ref(param));
> +}

The type checks mentioned above could be
g_return_if_fail(GVIR_SANDBOX_IS_CONFIG_NETWORK_FILTER(config));
g_return_if_fail(GVIR_SANDBOX_IS_CONFIG_NETWORK_FILTER_PARAMETER(config));
here


> +
> +
> +/**
> + * gvir_sandbox_config_network_filterref_get_parameters:
> + * @filter: (transfer none): the filter reference configuration
> + *
> + * Retrieve the list of parameters associated with a network filter reference
> + *
> + * Returns: (transfer full)(element-type GVirSandboxConfigNetworkFilterrefParameter): the parameter list
> + */
> +GList *gvir_sandbox_config_network_filterref_get_parameters(GVirSandboxConfigNetworkFilterref *filter)
> +{
> +    GVirSandboxConfigNetworkFilterrefPrivate *priv = filter->priv;
> +    g_list_foreach(priv->parameters, (GFunc)g_object_ref, NULL);
> +    return g_list_copy(priv->parameters);
> +}
> diff --git a/libvirt-sandbox/libvirt-sandbox-config-network-filterref.h b/libvirt-sandbox/libvirt-sandbox-config-network-filterref.h
> new file mode 100644
> index 0000000..4925530
> --- /dev/null
> +++ b/libvirt-sandbox/libvirt-sandbox-config-network-filterref.h
> @@ -0,0 +1,75 @@
> +/*
> + * libvirt-sandbox-config-mount.h: libvirt sandbox configuration

s/mount/network-filterref/

> + *
> + * Copyright (C) 2013 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + * Author: Ian Main <imain@xxxxxxxxxx>
> + */
> +
> +#if !defined(__LIBVIRT_SANDBOX_H__) && !defined(LIBVIRT_SANDBOX_BUILD)
> +#error "Only <libvirt-sandbox/libvirt-sandbox.h> can be included directly."
> +#endif
> +
> +#ifndef __LIBVIRT_SANDBOX_CONFIG_NETWORK_FILTERREF_H__
> +#define __LIBVIRT_SANDBOX_CONFIG_NETWORK_FILTERREF_H__
> +
> +G_BEGIN_DECLS
> +
> +#define GVIR_SANDBOX_TYPE_CONFIG_NETWORK_FILTERREF            (gvir_sandbox_config_network_filterref_get_type ())
> +#define GVIR_SANDBOX_CONFIG_NETWORK_FILTERREF(obj)            (G_TYPE_CHECK_INSTANCE_CAST ((obj), GVIR_SANDBOX_TYPE_CONFIG_NETWORK_FILTERREF, GVirSandboxConfigNetworkFilterref))
> +#define GVIR_SANDBOX_CONFIG_NETWORK_FILTERREF_CLASS(klass)    (G_TYPE_CHECK_CLASS_CAST ((klass), GVIR_SANDBOX_TYPE_CONFIG_NETWORK_FILTERREF, GVirSandboxConfigNetworkFilterrefClass))
> +#define GVIR_SANDBOX_IS_CONFIG_NETWORK_FILTERREF(obj)         (G_TYPE_CHECK_INSTANCE_TYPE ((obj), GVIR_SANDBOX_TYPE_CONFIG_NETWORK_FILTERREF))
> +#define GVIR_SANDBOX_IS_CONFIG_NETWORK_FILTERREF_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), GVIR_SANDBOX_TYPE_CONFIG_NETWORK_FILTERREF))
> +#define GVIR_SANDBOX_CONFIG_NETWORK_FILTERREF_GET_CLASS(obj)  (G_TYPE_INSTANCE_GET_CLASS ((obj), GVIR_SANDBOX_TYPE_CONFIG_NETWORK_FILTERREF, GVirSandboxConfigNetworkFilterrefClass))
> +
> +#define GVIR_SANDBOX_TYPE_CONFIG_NETWORK_FILTERREF_HANDLE      (gvir_sandbox_config_network_filterref_handle_get_type ())

This _HANDLE #define can be removed

> +
> +typedef struct _GVirSandboxConfigNetworkFilterref GVirSandboxConfigNetworkFilterref;
> +typedef struct _GVirSandboxConfigNetworkFilterrefPrivate GVirSandboxConfigNetworkFilterrefPrivate;
> +typedef struct _GVirSandboxConfigNetworkFilterrefClass GVirSandboxConfigNetworkFilterrefClass;
> +
> +struct _GVirSandboxConfigNetworkFilterref
> +{
> +    GObject parent;
> +
> +    GVirSandboxConfigNetworkFilterrefPrivate *priv;
> +
> +    /* Do not add fields to this struct */
> +};
> +
> +struct _GVirSandboxConfigNetworkFilterrefClass
> +{
> +    GObjectClass parent_class;
> +
> +    gpointer padding[LIBVIRT_SANDBOX_CLASS_PADDING];
> +};
> +
> +GType gvir_sandbox_config_network_filterref_get_type(void);
> +
> +GVirSandboxConfigNetworkFilterref *gvir_sandbox_config_network_filterref_new(void);
> +
> +const gchar *gvir_sandbox_config_network_filterref_get_name(GVirSandboxConfigNetworkFilterref *config);
> +void gvir_sandbox_config_network_filterref_set_name(GVirSandboxConfigNetworkFilterref *filter, const gchar *name);
> +
> +void gvir_sandbox_config_network_filterref_add_parameter(GVirSandboxConfigNetworkFilterref *filter,
> +                                                         GVirSandboxConfigNetworkFilterrefParameter *param);
> +GList *gvir_sandbox_config_network_filterref_get_parameters(GVirSandboxConfigNetworkFilterref *filter);
> +
> +
> +G_END_DECLS
> +
> +#endif /* __LIBVIRT_SANDBOX_CONFIG_NETWORK_FILTERREF_H__ */
> diff --git a/libvirt-sandbox/libvirt-sandbox.h b/libvirt-sandbox/libvirt-sandbox.h
> index a3f0b2c..adb21a1 100644
> --- a/libvirt-sandbox/libvirt-sandbox.h
> +++ b/libvirt-sandbox/libvirt-sandbox.h
> @@ -25,6 +25,7 @@
>  
>  /* External includes */
>  #include <libvirt-gobject/libvirt-gobject.h>
> +#include <locale.h>
>  
>  /* Local includes */
>  #include <libvirt-sandbox/libvirt-sandbox-main.h>
> @@ -37,6 +38,8 @@
>  #include <libvirt-sandbox/libvirt-sandbox-config-mount-guest-bind.h>
>  #include <libvirt-sandbox/libvirt-sandbox-config-mount-ram.h>
>  #include <libvirt-sandbox/libvirt-sandbox-config-network-address.h>
> +#include <libvirt-sandbox/libvirt-sandbox-config-network-filterref-parameter.h>
> +#include <libvirt-sandbox/libvirt-sandbox-config-network-filterref.h>

Nit, this is not consistent with the order you put these files in
Makefile.am

>  #include <libvirt-sandbox/libvirt-sandbox-config-network-route.h>
>  #include <libvirt-sandbox/libvirt-sandbox-config-network.h>
>  #include <libvirt-sandbox/libvirt-sandbox-config.h>
> diff --git a/libvirt-sandbox/libvirt-sandbox.sym b/libvirt-sandbox/libvirt-sandbox.sym
> index 7b7c8be..c46ccd9 100644
> --- a/libvirt-sandbox/libvirt-sandbox.sym
> +++ b/libvirt-sandbox/libvirt-sandbox.sym
> @@ -44,6 +44,7 @@ LIBVIRT_SANDBOX_0.2.1 {
>  	gvir_sandbox_config_mount_ram_set_usage;
>  
>  	gvir_sandbox_config_network_add_address;
> +	gvir_sandbox_config_network_set_filterref;
>  	gvir_sandbox_config_network_add_route;
>  	gvir_sandbox_config_network_get_type;
>  	gvir_sandbox_config_network_get_dhcp;
> @@ -51,6 +52,7 @@ LIBVIRT_SANDBOX_0.2.1 {
>  	gvir_sandbox_config_network_get_source;
>  	gvir_sandbox_config_network_get_routes;
>  	gvir_sandbox_config_network_get_addresses;
> +	gvir_sandbox_config_network_get_filterref;
>  	gvir_sandbox_config_network_new;
>  	gvir_sandbox_config_network_set_dhcp;
>  	gvir_sandbox_config_network_set_mac;
> @@ -65,6 +67,18 @@ LIBVIRT_SANDBOX_0.2.1 {
>  	gvir_sandbox_config_network_address_set_primary;
>  	gvir_sandbox_config_network_address_set_prefix;
>  
> +	gvir_sandbox_config_network_filterref_get_type;
> +	gvir_sandbox_config_network_filterref_new;
> +	gvir_sandbox_config_network_filterref_get_name;
> +	gvir_sandbox_config_network_filterref_set_name;

add_parameter/get_parameters are in the public
GVirSandboxConfigNetworkFiltterref header, so they should be listed here

> +
> +	gvir_sandbox_config_network_filterref_parameter_get_type;
> +	gvir_sandbox_config_network_filterref_parameter_new;
> +	gvir_sandbox_config_network_filterref_parameter_get_name;
> +	gvir_sandbox_config_network_filterref_parameter_set_name;
> +	gvir_sandbox_config_network_filterref_parameter_get_value;
> +	gvir_sandbox_config_network_filterref_parameter_set_value;
> +
>  	gvir_sandbox_config_network_route_get_type;
>  	gvir_sandbox_config_network_route_get_prefix;
>  	gvir_sandbox_config_network_route_get_gateway;
> -- 
> 1.8.1.4
> 
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: pgppI3uX9YOXE.pgp
Description: PGP signature

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