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

Re: [libvirt] [PATCH libvirt-glib 1/5] Add suport for setting <filesystem> elements



Hi,

There's a typo in the subject "su*p*port"

On Wed, Nov 30, 2011 at 04:52:03PM +0000, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange redhat com>
> 
> Create a new GVirConfigDomainFilesys object for dealing with
> the <filesystem> element.
> 
> * libvirt-gconfig-domain-filesys.c, libvirt-gconfig-domain-filesys.h,
>   Makefile.am, libvirt-gconfig.sym, libvirt-gconfig.h: New
>   GVirConfigDomainFilesys object
> * libvirt-gconfig-object.c, libvirt-gconfig-object-private.h: Add
>   gvir_config_object_delete_node for removing an XML node
> ---
>  libvirt-gconfig/Makefile.am                      |    2 +
>  libvirt-gconfig/libvirt-gconfig-domain-filesys.c |  181 ++++++++++++++++++++++
>  libvirt-gconfig/libvirt-gconfig-domain-filesys.h |   98 ++++++++++++
>  libvirt-gconfig/libvirt-gconfig-object-private.h |    2 +
>  libvirt-gconfig/libvirt-gconfig-object.c         |   25 +++
>  libvirt-gconfig/libvirt-gconfig.h                |    1 +
>  libvirt-gconfig/libvirt-gconfig.sym              |   13 ++
>  7 files changed, 322 insertions(+), 0 deletions(-)
>  create mode 100644 libvirt-gconfig/libvirt-gconfig-domain-filesys.c
>  create mode 100644 libvirt-gconfig/libvirt-gconfig-domain-filesys.h
> 
> diff --git a/libvirt-gconfig/Makefile.am b/libvirt-gconfig/Makefile.am
> index 1879186..2c8c2a5 100644
> --- a/libvirt-gconfig/Makefile.am
> +++ b/libvirt-gconfig/Makefile.am
> @@ -16,6 +16,7 @@ GCONFIG_HEADER_FILES = \
>  			libvirt-gconfig-domain-clock.h \
>  			libvirt-gconfig-domain-device.h \
>  			libvirt-gconfig-domain-disk.h \
> +			libvirt-gconfig-domain-filesys.h \
>  			libvirt-gconfig-domain-graphics.h \
>  			libvirt-gconfig-domain-graphics-spice.h \
>  			libvirt-gconfig-domain-input.h \
> @@ -45,6 +46,7 @@ GCONFIG_SOURCE_FILES = \
>  			libvirt-gconfig-domain-clock.c \
>  			libvirt-gconfig-domain-device.c \
>  			libvirt-gconfig-domain-disk.c \
> +			libvirt-gconfig-domain-filesys.c \
>  			libvirt-gconfig-domain-graphics.c \
>  			libvirt-gconfig-domain-graphics-spice.c \
>  			libvirt-gconfig-domain-input.c \
> diff --git a/libvirt-gconfig/libvirt-gconfig-domain-filesys.c b/libvirt-gconfig/libvirt-gconfig-domain-filesys.c
> new file mode 100644
> index 0000000..6aa7f02
> --- /dev/null
> +++ b/libvirt-gconfig/libvirt-gconfig-domain-filesys.c

I'd go with fs rather than filesys, but I'm fine with filesys if you feel
it's a better name.


> @@ -0,0 +1,181 @@
> +/*
> + * libvirt-gobject-config-domain-filesys.c: libvirt glib integration
> + *
> + * Copyright (C) 2011 Red Hat
> + *
> + * 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., 59 Temple Place, Suite 330, Boston, MA 02111-1307  USA
> + *
> + * Author: Christophe Fergeau <cfergeau gmail com>

Ah, I must have written this during my nap after lunch :)

> + */
> +
> +#include <config.h>
> +
> +#include <string.h>
> +
> +#include <libxml/tree.h>
> +
> +#include "libvirt-gconfig/libvirt-gconfig.h"
> +#include "libvirt-gconfig/libvirt-gconfig-helpers-private.h"
> +#include "libvirt-gconfig/libvirt-gconfig-object-private.h"
> +
> +#define GVIR_CONFIG_DOMAIN_FILESYS_GET_PRIVATE(obj)                         \
> +        (G_TYPE_INSTANCE_GET_PRIVATE((obj), GVIR_TYPE_CONFIG_DOMAIN_FILESYS, GVirConfigDomainFilesysPrivate))
> +
> +struct _GVirConfigDomainFilesysPrivate
> +{
> +    GVirConfigDomainFilesysType type;
> +};
> +
> +G_DEFINE_TYPE(GVirConfigDomainFilesys, gvir_config_domain_filesys, GVIR_TYPE_CONFIG_DOMAIN_DEVICE);
> +
> +
> +static void gvir_config_domain_filesys_class_init(GVirConfigDomainFilesysClass *klass)
> +{
> +    g_type_class_add_private(klass, sizeof(GVirConfigDomainFilesysPrivate));
> +}
> +
> +
> +static void gvir_config_domain_filesys_init(GVirConfigDomainFilesys *filesys)
> +{
> +    g_debug("Init GVirConfigDomainFilesys=%p", filesys);
> +
> +    filesys->priv = GVIR_CONFIG_DOMAIN_FILESYS_GET_PRIVATE(filesys);
> +}
> +
> +
> +GVirConfigDomainFilesys *gvir_config_domain_filesys_new(void)
> +{
> +    GVirConfigObject *object;
> +
> +    object = gvir_config_object_new(GVIR_TYPE_CONFIG_DOMAIN_FILESYS,
> +                                    "filesystem", NULL);
> +    return GVIR_CONFIG_DOMAIN_FILESYS(object);
> +}
> +
> +GVirConfigDomainFilesys *gvir_config_domain_filesys_new_from_xml(const gchar *xml,
> +                                                                 GError **error)
> +{
> +    GVirConfigObject *object;
> +
> +    object = gvir_config_object_new_from_xml(GVIR_TYPE_CONFIG_DOMAIN_FILESYS,
> +                                             "filesystem", NULL, xml, error);
> +    return GVIR_CONFIG_DOMAIN_FILESYS(object);
> +}
> +
> +void gvir_config_domain_filesys_set_type(GVirConfigDomainFilesys *filesys,
> +                                         GVirConfigDomainFilesysType type)
> +{
> +    xmlNodePtr node;
> +    const char *type_str;
> +
> +    g_return_if_fail(GVIR_IS_CONFIG_DOMAIN_FILESYS(filesys));
> +
> +    node = gvir_config_object_get_xml_node(GVIR_CONFIG_OBJECT(filesys));
> +    g_return_if_fail(node != NULL);
> +    type_str = gvir_config_genum_get_nick(GVIR_TYPE_CONFIG_DOMAIN_FILESYS_TYPE,
> +                                          type);
> +    g_return_if_fail(type_str != NULL);
> +    xmlNewProp(node, (xmlChar*)"type", (xmlChar*)type_str);
> +    filesys->priv->type = type;

gvir_config_object_set_attribute_with_type should work here

> +}
> +
> +void gvir_config_domain_filesys_set_access_type(GVirConfigDomainFilesys *filesys,
> +                                                GVirConfigDomainFilesysAccessType type)
> +{
> +    xmlNodePtr node;
> +    const char *type_str;
> +
> +    g_return_if_fail(GVIR_IS_CONFIG_DOMAIN_FILESYS(filesys));
> +
> +    node = gvir_config_object_get_xml_node(GVIR_CONFIG_OBJECT(filesys));
> +    g_return_if_fail(node != NULL);
> +    type_str = gvir_config_genum_get_nick(GVIR_TYPE_CONFIG_DOMAIN_FILESYS_ACCESS_TYPE,
> +                                          type);
> +    g_return_if_fail(type_str != NULL);
> +    xmlNewProp(node, (xmlChar*)"accesmode", (xmlChar*)type_str);

and here too

> +}
> +
> +void gvir_config_domain_filesys_set_driver_type(GVirConfigDomainFilesys *filesys,
> +                                                GVirConfigDomainFilesysDriverType type)
> +{
> +    GVirConfigObject *node;
> +
> +    g_return_if_fail(GVIR_IS_CONFIG_DOMAIN_FILESYS(filesys));
> +    node = gvir_config_object_add_child(GVIR_CONFIG_OBJECT(filesys), "driver");
> +    g_return_if_fail(GVIR_IS_CONFIG_OBJECT(node));
> +    if (type != GVIR_CONFIG_DOMAIN_FILESYS_DRIVER_DEFAULT)
> +        gvir_config_object_set_attribute_with_type(
> +            node, "type",
> +            GVIR_TYPE_CONFIG_DOMAIN_FILESYS_DRIVER_TYPE,
> +            type, NULL);
> +    /* else XXX delete existing attribute ? */
> +    g_object_unref(G_OBJECT(node));
> +}
> +
> +void gvir_config_domain_filesys_set_source(GVirConfigDomainFilesys *filesys,
> +                                           const char *source)
> +{
> +    GVirConfigObject *source_node;
> +    const char *attribute_name;
> +
> +    g_return_if_fail(GVIR_IS_CONFIG_DOMAIN_FILESYS(filesys));
> +    source_node = gvir_config_object_replace_child(GVIR_CONFIG_OBJECT(filesys),
> +                                                   "source");
> +    g_return_if_fail(source_node != NULL);
> +
> +    switch (filesys->priv->type) {
> +        case GVIR_CONFIG_DOMAIN_FILESYS_MOUNT:
> +            attribute_name = "dir";
> +            break;
> +        case GVIR_CONFIG_DOMAIN_FILESYS_FILE:
> +            attribute_name = "file";
> +            break;
> +        case GVIR_CONFIG_DOMAIN_FILESYS_BLOCK:
> +            attribute_name = "dev";
> +            break;
> +        case GVIR_CONFIG_DOMAIN_FILESYS_TEMPLATE:
> +            attribute_name = "name";
> +            break;
> +        default:
> +            g_return_if_reached();
> +    }
> +
> +    gvir_config_object_set_attribute(source_node,
> +                                     attribute_name, source,
> +                                     NULL);
> +    g_object_unref(G_OBJECT(source_node));
> +}
> +
> +
> +void gvir_config_domain_filesys_set_target(GVirConfigDomainFilesys *filesys,
> +                                           const char *path)
> +{
> +    GVirConfigObject *node;
> +
> +    g_return_if_fail(GVIR_IS_CONFIG_DOMAIN_FILESYS(filesys));
> +    node = gvir_config_object_add_child(GVIR_CONFIG_OBJECT(filesys), "target");
> +    g_return_if_fail(GVIR_IS_CONFIG_OBJECT(node));
> +    gvir_config_object_set_attribute(node, "dir", path, NULL);
> +    g_object_unref(G_OBJECT(node));
> +}
> +
> +void gvir_config_domain_filesys_set_readonly(GVirConfigDomainFilesys *filesys,
> +                                             gboolean readonly)
> +{
> +    if (readonly)
> +        gvir_config_object_replace_child(GVIR_CONFIG_OBJECT(filesys), "readonly");

You need to unref _replace_child return value

> +    else
> +        gvir_config_object_delete_child(GVIR_CONFIG_OBJECT(filesys), "readonly");
> +}
> diff --git a/libvirt-gconfig/libvirt-gconfig-domain-filesys.h b/libvirt-gconfig/libvirt-gconfig-domain-filesys.h
> new file mode 100644
> index 0000000..bfb98df
> --- /dev/null
> +++ b/libvirt-gconfig/libvirt-gconfig-domain-filesys.h
> @@ -0,0 +1,98 @@
> +/*
> + * libvirt-gobject-domain-filesys.c: libvirt gobject integration

Nit, filesys.h

> + *
> + * Copyright (C) 2011 Red Hat
> + *
> + * 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., 59 Temple Place, Suite 330, Boston, MA 02111-1307  USA
> + *
> + * Author: Christophe Fergeau <cfergeau gmail com>

I'm pleading non-guilty once again :)

> + */
> +
> +#if !defined(__LIBVIRT_GCONFIG_H__) && !defined(LIBVIRT_GCONFIG_BUILD)
> +#error "Only <libvirt-gconfig/libvirt-gconfig.h> can be included directly."
> +#endif
> +
> +#ifndef __LIBVIRT_GCONFIG_DOMAIN_FILESYS_H__
> +#define __LIBVIRT_GCONFIG_DOMAIN_FILESYS_H__
> +
> +G_BEGIN_DECLS
> +
> +#define GVIR_TYPE_CONFIG_DOMAIN_FILESYS            (gvir_config_domain_filesys_get_type ())
> +#define GVIR_CONFIG_DOMAIN_FILESYS(obj)            (G_TYPE_CHECK_INSTANCE_CAST ((obj), GVIR_TYPE_CONFIG_DOMAIN_FILESYS, GVirConfigDomainFilesys))
> +#define GVIR_CONFIG_DOMAIN_FILESYS_CLASS(klass)    (G_TYPE_CHECK_CLASS_CAST ((klass), GVIR_TYPE_CONFIG_DOMAIN_FILESYS, GVirConfigDomainFilesysClass))
> +#define GVIR_IS_CONFIG_DOMAIN_FILESYS(obj)         (G_TYPE_CHECK_INSTANCE_TYPE ((obj), GVIR_TYPE_CONFIG_DOMAIN_FILESYS))
> +#define GVIR_IS_CONFIG_DOMAIN_FILESYS_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), GVIR_TYPE_CONFIG_DOMAIN_FILESYS))
> +#define GVIR_CONFIG_DOMAIN_FILESYS_GET_CLASS(obj)  (G_TYPE_INSTANCE_GET_CLASS ((obj), GVIR_TYPE_CONFIG_DOMAIN_FILESYS, GVirConfigDomainFilesysClass))
> +
> +typedef struct _GVirConfigDomainFilesys GVirConfigDomainFilesys;
> +typedef struct _GVirConfigDomainFilesysPrivate GVirConfigDomainFilesysPrivate;
> +typedef struct _GVirConfigDomainFilesysClass GVirConfigDomainFilesysClass;
> +
> +struct _GVirConfigDomainFilesys
> +{
> +    GVirConfigDomainDevice parent;
> +
> +    GVirConfigDomainFilesysPrivate *priv;
> +
> +    /* Do not add fields to this struct */
> +};
> +
> +struct _GVirConfigDomainFilesysClass
> +{
> +    GVirConfigDomainDeviceClass parent_class;
> +
> +    gpointer padding[20];
> +};
> +
> +typedef enum {
> +    GVIR_CONFIG_DOMAIN_FILESYS_MOUNT,
> +    GVIR_CONFIG_DOMAIN_FILESYS_BLOCK,
> +    GVIR_CONFIG_DOMAIN_FILESYS_FILE,
> +    GVIR_CONFIG_DOMAIN_FILESYS_TEMPLATE,
> +} GVirConfigDomainFilesysType;
> +
> +typedef enum {
> +    GVIR_CONFIG_DOMAIN_FILESYS_ACCESS_PASSTHROUGH,
> +    GVIR_CONFIG_DOMAIN_FILESYS_ACCESS_MAPPED,
> +    GVIR_CONFIG_DOMAIN_FILESYS_ACCESS_SQUASHED,
> +} GVirConfigDomainFilesysAccessType;
> +
> +typedef enum {
> +    GVIR_CONFIG_DOMAIN_FILESYS_DRIVER_DEFAULT,
> +    GVIR_CONFIG_DOMAIN_FILESYS_DRIVER_PATH,
> +    GVIR_CONFIG_DOMAIN_FILESYS_DRIVER_HANDLE,
> +} GVirConfigDomainFilesysDriverType;
> +
> +GType gvir_config_domain_filesys_get_type(void);
> +
> +GVirConfigDomainFilesys *gvir_config_domain_filesys_new(void);
> +GVirConfigDomainFilesys *gvir_config_domain_filesys_new_from_xml(const gchar *xml,
> +                                                                 GError **error);
> +
> +void gvir_config_domain_filesys_set_type(GVirConfigDomainFilesys *filesys,
> +                                         GVirConfigDomainFilesysType type);
> +void gvir_config_domain_filesys_set_access_type(GVirConfigDomainFilesys *filesys,
> +                                                GVirConfigDomainFilesysAccessType type);
> +void gvir_config_domain_filesys_set_driver_type(GVirConfigDomainFilesys *filesys,
> +                                                GVirConfigDomainFilesysDriverType type);
> +void gvir_config_domain_filesys_set_source(GVirConfigDomainFilesys *filesys,
> +                                           const char *source);
> +void gvir_config_domain_filesys_set_target(GVirConfigDomainFilesys *filesys,
> +                                           const char *target);
> +void gvir_config_domain_filesys_set_readonly(GVirConfigDomainFilesys *filesys,
> +                                             gboolean readonly);
> +G_END_DECLS
> +
> +#endif /* __LIBVIRT_GCONFIG_DOMAIN_FILESYS_H__ */
> diff --git a/libvirt-gconfig/libvirt-gconfig-object-private.h b/libvirt-gconfig/libvirt-gconfig-object-private.h
> index f196be6..c564ac5 100644
> --- a/libvirt-gconfig/libvirt-gconfig-object-private.h
> +++ b/libvirt-gconfig/libvirt-gconfig-object-private.h
> @@ -39,6 +39,8 @@ GVirConfigObject *gvir_config_object_add_child(GVirConfigObject *object,
>                                                 const char *child_name);
>  GVirConfigObject *gvir_config_object_replace_child(GVirConfigObject *object,
>                                                     const char *child_name);
> +void gvir_config_object_delete_child(GVirConfigObject *object,
> +                                     const char *child_name);
>  void gvir_config_object_set_child(GVirConfigObject *object,
>                                    xmlNodePtr child);
>  
> diff --git a/libvirt-gconfig/libvirt-gconfig-object.c b/libvirt-gconfig/libvirt-gconfig-object.c
> index 902e014..7fb1d4f 100644
> --- a/libvirt-gconfig/libvirt-gconfig-object.c
> +++ b/libvirt-gconfig/libvirt-gconfig-object.c
> @@ -380,6 +380,31 @@ gvir_config_object_replace_child(GVirConfigObject *object,
>                                             NULL));
>  }
>  
> +
> +G_GNUC_INTERNAL void
> +gvir_config_object_delete_child(GVirConfigObject *object,
> +                                const char *child_name)
> +{
> +    xmlNodePtr parent_node;
> +    xmlNodePtr old_node;
> +
> +    g_return_if_fail(GVIR_IS_CONFIG_OBJECT(object));
> +    g_return_if_fail(child_name != NULL);
> +
> +    parent_node = gvir_config_object_get_xml_node(GVIR_CONFIG_OBJECT(object));
> +    g_return_if_fail (parent_node != NULL);

This is nitpicking but there's a space before (
> +
> +    if (!(old_node = gvir_config_xml_get_element(parent_node, child_name, NULL)))
> +        return;
> +
> +    /* FIXME: should we make sure there are no multiple occurrences
> +     * of this node?
> +     */
> +    xmlUnlinkNode(old_node);
> +    xmlFreeNode(old_node);
> +}


I think we will get memory corruption if this API is combined with
_attach_child:

device_node = gvir_config_object_new("device");
fs_node = gvir_config_object_new("fs");
gvir_config_object_attach(device_node, fs_node);
gvir_config_object_delete_child(device_node, "fs");
g_object_unref(G_OBJECT(fs_node));

The xmlNodePtr held by fs_node will be freed twice, once by _delete_child
and when _finalize runs after the call to g_object_unref

Having each GVirConfigObject keep a list of its GVirConfigObject children
would make it possible to handle this case I think.

I'm fine with getting this function in even if there are known issues with
it.

> +
> +
>  G_GNUC_INTERNAL void
>  gvir_config_object_set_node_content(GVirConfigObject *object,
>                                      const char *node_name,
> diff --git a/libvirt-gconfig/libvirt-gconfig.h b/libvirt-gconfig/libvirt-gconfig.h
> index ad9f59a..ed7623c 100644
> --- a/libvirt-gconfig/libvirt-gconfig.h
> +++ b/libvirt-gconfig/libvirt-gconfig.h
> @@ -33,6 +33,7 @@
>  #include <libvirt-gconfig/libvirt-gconfig-domain-clock.h>
>  #include <libvirt-gconfig/libvirt-gconfig-domain-device.h>
>  #include <libvirt-gconfig/libvirt-gconfig-domain-disk.h>
> +#include <libvirt-gconfig/libvirt-gconfig-domain-filesys.h>
>  #include <libvirt-gconfig/libvirt-gconfig-domain-graphics.h>
>  #include <libvirt-gconfig/libvirt-gconfig-domain-graphics-spice.h>
>  #include <libvirt-gconfig/libvirt-gconfig-domain-input.h>
> diff --git a/libvirt-gconfig/libvirt-gconfig.sym b/libvirt-gconfig/libvirt-gconfig.sym
> index 9da3933..3ad7b1f 100644
> --- a/libvirt-gconfig/libvirt-gconfig.sym
> +++ b/libvirt-gconfig/libvirt-gconfig.sym
> @@ -47,6 +47,19 @@ LIBVIRT_GCONFIG_0.0.1 {
>  	gvir_config_domain_disk_set_target_dev;
>  	gvir_config_domain_disk_set_type;
>  
> +	gvir_config_domain_filesys_get_type;
> +	gvir_config_domain_filesys_type_get_type;
> +	gvir_config_domain_filesys_access_type_get_type;
> +	gvir_config_domain_filesys_driver_type_get_type;
> +	gvir_config_domain_filesys_new;
> +	gvir_config_domain_filesys_new_from_xml;
> +	gvir_config_domain_filesys_set_access_type;
> +	gvir_config_domain_filesys_set_driver_type;
> +	gvir_config_domain_filesys_set_readonly;
> +	gvir_config_domain_filesys_set_source;
> +	gvir_config_domain_filesys_set_target;
> +	gvir_config_domain_filesys_set_type;
> +
>  	gvir_config_domain_graphics_get_type;
>  
>  	gvir_config_domain_graphics_spice_get_type;
> -- 
> 1.7.6.4
> 
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: pgpej3o2vwsep.pgp
Description: PGP signature


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