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

Re: [libvirt] [libvirt-glib 5/6] Remove now redundant 'path' property



On Tue, Feb 28, 2012 at 08:25:06PM +0200, Zeeshan Ali (Khattak) wrote:
> From: "Zeeshan Ali (Khattak)" <zeeshanak gnome org>
> 
> Remove now redundant 'path' property from GVirDomainDevice subclasses.
> ---
>  libvirt-gobject/libvirt-gobject-domain-disk.c      |   88 ++++----------------
>  libvirt-gobject/libvirt-gobject-domain-disk.h      |    3 +-
>  libvirt-gobject/libvirt-gobject-domain-interface.c |   89 +++-----------------
>  libvirt-gobject/libvirt-gobject-domain-interface.h |    3 +-
>  4 files changed, 31 insertions(+), 152 deletions(-)
> 
> diff --git a/libvirt-gobject/libvirt-gobject-domain-disk.c b/libvirt-gobject/libvirt-gobject-domain-disk.c
> index fb7672e..42e0e6c 100644
> --- a/libvirt-gobject/libvirt-gobject-domain-disk.c
> +++ b/libvirt-gobject/libvirt-gobject-domain-disk.c
> @@ -34,75 +34,22 @@
>  #define GVIR_DOMAIN_DISK_GET_PRIVATE(obj)                         \
>          (G_TYPE_INSTANCE_GET_PRIVATE((obj), GVIR_TYPE_DOMAIN_DISK, GVirDomainDiskPrivate))
>  
> -struct _GVirDomainDiskPrivate
> -{
> -    gchar *path;
> -};

The rest of the code (GVirConfig*) always has a Private structure with a
gboolean unused when it's empty, I'd rather we stayed consistent (though I
keep thinking that we should remove these empty private structs everywhere
:)

> -
>  G_DEFINE_TYPE(GVirDomainDisk, gvir_domain_disk, GVIR_TYPE_DOMAIN_DEVICE);
>  
> -enum {
> -    PROP_0,
> -    PROP_PATH,
> -};
> -
>  #define GVIR_DOMAIN_DISK_ERROR gvir_domain_disk_error_quark()
>  
> -
>  static GQuark
>  gvir_domain_disk_error_quark(void)
>  {
>      return g_quark_from_static_string("gvir-domain-disk");
>  }
>  
> -static void gvir_domain_disk_get_property(GObject *object,
> -                                          guint prop_id,
> -                                          GValue *value,
> -                                          GParamSpec *pspec)
> -{
> -    GVirDomainDisk *self = GVIR_DOMAIN_DISK(object);
> -    GVirDomainDiskPrivate *priv = self->priv;
> -
> -    switch (prop_id) {
> -    case PROP_PATH:
> -        g_value_set_string(value, priv->path);
> -        break;
> -
> -    default:
> -        G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> -    }
> -}
> -
> -
> -static void gvir_domain_disk_set_property(GObject *object,
> -                                          guint prop_id,
> -                                          const GValue *value,
> -                                          GParamSpec *pspec)
> -{
> -    GVirDomainDisk *self = GVIR_DOMAIN_DISK(object);
> -    GVirDomainDiskPrivate *priv = self->priv;
> -
> -    switch (prop_id) {
> -    case PROP_PATH:
> -        g_free(priv->path);
> -        priv->path = g_value_dup_string(value);
> -        break;
> -
> -    default:
> -        G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> -    }
> -}
> -
> -
>  static void gvir_domain_disk_finalize(GObject *object)
>  {
>      GVirDomainDisk *self = GVIR_DOMAIN_DISK(object);
> -    GVirDomainDiskPrivate *priv = self->priv;
>  
>      g_debug("Finalize GVirDomainDisk=%p", self);
>  
> -    g_free(priv->path);
> -
>      G_OBJECT_CLASS(gvir_domain_disk_parent_class)->finalize(object);
>  }
>  
> @@ -111,27 +58,11 @@ static void gvir_domain_disk_class_init(GVirDomainDiskClass *klass)
>      GObjectClass *object_class = G_OBJECT_CLASS (klass);
>  
>      object_class->finalize = gvir_domain_disk_finalize;
> -    object_class->get_property = gvir_domain_disk_get_property;
> -    object_class->set_property = gvir_domain_disk_set_property;
> -
> -    g_object_class_install_property(object_class,
> -                                    PROP_PATH,
> -                                    g_param_spec_string("path",
> -                                                        "Path",
> -                                                        "The disk path",
> -                                                        NULL,
> -                                                        G_PARAM_READWRITE |
> -                                                        G_PARAM_CONSTRUCT_ONLY |
> -                                                        G_PARAM_STATIC_STRINGS));
> -
> -    g_type_class_add_private(klass, sizeof(GVirDomainDiskPrivate));

This would need to stay if we keep the Private struct

>  }
>  
>  static void gvir_domain_disk_init(GVirDomainDisk *self)
>  {
>      g_debug("Init GVirDomainDisk=%p", self);
> -
> -    self->priv = GVIR_DOMAIN_DISK_GET_PRIVATE(self);

Ditto.

>  }
>  
>  static GVirDomainDiskStats *
> @@ -151,6 +82,15 @@ gvir_domain_disk_stats_free(GVirDomainDiskStats *stats)
>  G_DEFINE_BOXED_TYPE(GVirDomainDiskStats, gvir_domain_disk_stats,
>                      gvir_domain_disk_stats_copy, gvir_domain_disk_stats_free)
>  
> +static const gchar *gvir_domain_disk_get_path(GVirDomainDisk *self)
> +{
> +    GVirConfigDomainDevice *config;
> +
> +    config = gvir_domain_device_get_config(GVIR_DOMAIN_DEVICE(self));

config needs to be unref'ed after use.
> +
> +    return gvir_config_domain_disk_get_target_dev (GVIR_CONFIG_DOMAIN_DISK (config));

and the return value would be non-const after the changes I mentioned in my
previous review.

> +}
> +
>  /**
>   * gvir_domain_disk_get_stats:
>   * @self: the domain disk
> @@ -166,15 +106,15 @@ GVirDomainDiskStats *gvir_domain_disk_get_stats(GVirDomainDisk *self, GError **e
>  {
>      GVirDomainDiskStats *ret = NULL;
>      virDomainBlockStatsStruct stats;
> -    GVirDomainDiskPrivate *priv;
>      virDomainPtr handle;
> +    const gchar *path;
>  
>      g_return_val_if_fail(GVIR_IS_DOMAIN_DISK(self), NULL);
>  
> -    priv = self->priv;
>      handle = gvir_domain_device_get_domain_handle(GVIR_DOMAIN_DEVICE(self));
> +    path = gvir_domain_disk_get_path (self);

will need to be freed

>  
> -    if (virDomainBlockStats(handle, priv->path, &stats, sizeof (stats)) < 0) {
> +    if (virDomainBlockStats(handle, path, &stats, sizeof (stats)) < 0) {
>          gvir_set_error_literal(err, GVIR_DOMAIN_DISK_ERROR,
>                                 0,
>                                 "Unable to get domain disk stats");
> @@ -211,13 +151,15 @@ gboolean gvir_domain_disk_resize(GVirDomainDisk *self,
>  {
>      gboolean ret = FALSE;
>      virDomainPtr handle;
> +    const gchar *path;
>  
>      g_return_val_if_fail(GVIR_IS_DOMAIN_DISK(self), FALSE);
>      g_return_val_if_fail(err == NULL || *err != NULL, FALSE);
>  
>      handle = gvir_domain_device_get_domain_handle(GVIR_DOMAIN_DEVICE(self));
> +    path = gvir_domain_disk_get_path (self);

and same here

>  
> -    if (virDomainBlockResize(handle, self->priv->path, size, flags) < 0) {
> +    if (virDomainBlockResize(handle, path, size, flags) < 0) {
>          gvir_set_error_literal(err, GVIR_DOMAIN_DISK_ERROR,
>                                 0,
>                                 "Failed to resize domain disk");
> diff --git a/libvirt-gobject/libvirt-gobject-domain-disk.h b/libvirt-gobject/libvirt-gobject-domain-disk.h
> index 1788d63..fb8b3dc 100644
> --- a/libvirt-gobject/libvirt-gobject-domain-disk.h
> +++ b/libvirt-gobject/libvirt-gobject-domain-disk.h
> @@ -56,7 +56,8 @@ struct _GVirDomainDisk
>  {
>      GVirDomainDevice parent;
>  
> -    GVirDomainDiskPrivate *priv;
> +    /* In case we need a private struct in future */
> +    gpointer padding[1];

Avoiding this seems like a good reason for not keeping the Private struct
(at least name it gpointer priv);

>  
>      /* Do not add fields to this struct */
>  };
> diff --git a/libvirt-gobject/libvirt-gobject-domain-interface.c b/libvirt-gobject/libvirt-gobject-domain-interface.c
> index 0917e03..9ec3877 100644
> --- a/libvirt-gobject/libvirt-gobject-domain-interface.c
> +++ b/libvirt-gobject/libvirt-gobject-domain-interface.c
> @@ -31,78 +31,22 @@
>  
>  #include "libvirt-gobject/libvirt-gobject-domain-device-private.h"
>  
> -#define GVIR_DOMAIN_INTERFACE_GET_PRIVATE(obj)                         \
> -        (G_TYPE_INSTANCE_GET_PRIVATE((obj), GVIR_TYPE_DOMAIN_INTERFACE, GVirDomainInterfacePrivate))
> -
> -struct _GVirDomainInterfacePrivate
> -{
> -    gchar *path;
> -};

Same comment about the Private struct.

> -
>  G_DEFINE_TYPE(GVirDomainInterface, gvir_domain_interface, GVIR_TYPE_DOMAIN_DEVICE);
>  
> -enum {
> -    PROP_0,
> -    PROP_PATH,
> -};
> -
>  #define GVIR_DOMAIN_INTERFACE_ERROR gvir_domain_interface_error_quark()
>  
> -
>  static GQuark
>  gvir_domain_interface_error_quark(void)
>  {
>      return g_quark_from_static_string("gvir-domain-interface");
>  }
>  
> -static void gvir_domain_interface_get_property(GObject *object,
> -                                               guint prop_id,
> -                                               GValue *value,
> -                                               GParamSpec *pspec)
> -{
> -    GVirDomainInterface *self = GVIR_DOMAIN_INTERFACE(object);
> -    GVirDomainInterfacePrivate *priv = self->priv;
> -
> -    switch (prop_id) {
> -    case PROP_PATH:
> -        g_value_set_string(value, priv->path);
> -        break;
> -
> -    default:
> -        G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> -    }
> -}
> -
> -
> -static void gvir_domain_interface_set_property(GObject *object,
> -                                          guint prop_id,
> -                                          const GValue *value,
> -                                          GParamSpec *pspec)
> -{
> -    GVirDomainInterface *self = GVIR_DOMAIN_INTERFACE(object);
> -    GVirDomainInterfacePrivate *priv = self->priv;
> -
> -    switch (prop_id) {
> -    case PROP_PATH:
> -        g_free(priv->path);
> -        priv->path = g_value_dup_string(value);
> -        break;
> -
> -    default:
> -        G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> -    }
> -}
> -
> -
>  static void gvir_domain_interface_finalize(GObject *object)
>  {
>      GVirDomainInterface *self = GVIR_DOMAIN_INTERFACE(object);
> -    GVirDomainInterfacePrivate *priv = self->priv;
>  
>      g_debug("Finalize GVirDomainInterface=%p", self);
>  
> -    g_free(priv->path);
> -
>      G_OBJECT_CLASS(gvir_domain_interface_parent_class)->finalize(object);
>  }
>  
> @@ -111,27 +55,11 @@ static void gvir_domain_interface_class_init(GVirDomainInterfaceClass *klass)
>      GObjectClass *object_class = G_OBJECT_CLASS (klass);
>  
>      object_class->finalize = gvir_domain_interface_finalize;
> -    object_class->get_property = gvir_domain_interface_get_property;
> -    object_class->set_property = gvir_domain_interface_set_property;
> -
> -    g_object_class_install_property(object_class,
> -                                    PROP_PATH,
> -                                    g_param_spec_string("path",
> -                                                        "Path",
> -                                                        "The interface path",
> -                                                        NULL,
> -                                                        G_PARAM_READWRITE |
> -                                                        G_PARAM_CONSTRUCT_ONLY |
> -                                                        G_PARAM_STATIC_STRINGS));
> -
> -    g_type_class_add_private(klass, sizeof(GVirDomainInterfacePrivate));
>  }
>  
>  static void gvir_domain_interface_init(GVirDomainInterface *self)
>  {
>      g_debug("Init GVirDomainInterface=%p", self);
> -
> -    self->priv = GVIR_DOMAIN_INTERFACE_GET_PRIVATE(self);
>  }
>  
>  static GVirDomainInterfaceStats *
> @@ -140,17 +68,24 @@ gvir_domain_interface_stats_copy(GVirDomainInterfaceStats *stats)
>      return g_slice_dup(GVirDomainInterfaceStats, stats);
>  }
>  
> -
>  static void
>  gvir_domain_interface_stats_free(GVirDomainInterfaceStats *stats)
>  {
>      g_slice_free(GVirDomainInterfaceStats, stats);
>  }
>  
> -
>  G_DEFINE_BOXED_TYPE(GVirDomainInterfaceStats, gvir_domain_interface_stats,
>                      gvir_domain_interface_stats_copy, gvir_domain_interface_stats_free)
>  
> +static const gchar *gvir_domain_interface_get_path(GVirDomainInterface *self)
> +{
> +    GVirConfigDomainDevice *config;
> +
> +    config = gvir_domain_device_get_config(GVIR_DOMAIN_DEVICE(self));
> +
> +    return gvir_config_domain_interface_get_ifname (GVIR_CONFIG_DOMAIN_INTERFACE (config));

Same leaks as earlier
> +}
> +
>  /**
>   * gvir_domain_interface_get_stats:
>   * @self: the domain interface
> @@ -166,15 +101,15 @@ GVirDomainInterfaceStats *gvir_domain_interface_get_stats(GVirDomainInterface *s
>  {
>      GVirDomainInterfaceStats *ret = NULL;
>      virDomainInterfaceStatsStruct stats;
> -    GVirDomainInterfacePrivate *priv;
>      virDomainPtr handle;
> +    const gchar *path;
>  
>      g_return_val_if_fail(GVIR_IS_DOMAIN_INTERFACE(self), NULL);
>  
> -    priv = self->priv;
>      handle = gvir_domain_device_get_domain_handle(GVIR_DOMAIN_DEVICE(self));
> +    path = gvir_domain_interface_get_path (self);

Same here.

>  
> -    if (virDomainInterfaceStats(handle, priv->path, &stats, sizeof (stats)) < 0) {
> +    if (virDomainInterfaceStats(handle, path, &stats, sizeof (stats)) < 0) {
>          gvir_set_error_literal(err, GVIR_DOMAIN_INTERFACE_ERROR,
>                                 0,
>                                 "Unable to get domain interface stats");
> diff --git a/libvirt-gobject/libvirt-gobject-domain-interface.h b/libvirt-gobject/libvirt-gobject-domain-interface.h
> index 62848db..26b7d1c 100644
> --- a/libvirt-gobject/libvirt-gobject-domain-interface.h
> +++ b/libvirt-gobject/libvirt-gobject-domain-interface.h
> @@ -59,7 +59,8 @@ struct _GVirDomainInterface
>  {
>      GVirDomainDevice parent;
>  
> -    GVirDomainInterfacePrivate *priv;
> +    /* In case we need a private struct in future */
> +    gpointer padding[1];

Can you send an updated version of this patch once you have fixed all of
these things?

Thanks,

Christophe

Attachment: pgpQyI7JSyBdD.pgp
Description: PGP signature


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