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

Re: [libvirt] [libvirt-glib] All string getters should return 'const'



Hey,

Thanks for doing this!
I'll do a more in depth review on Monday if noone beats me to it, just some
quick notes:
On Wed, Mar 07, 2012 at 06:02:06AM +0200, Zeeshan Ali (Khattak) wrote:
> From: "Zeeshan Ali (Khattak)" <zeeshanak gnome org>
> -char *
> -gvir_config_xml_get_child_element_content_glib (xmlNode    *node,
> +const char *
> +gvir_config_xml_get_child_element_content_glib (xmlNode *node,
>                                                  const char *child_name)
>  {
> -        xmlChar *content;
> +    const xmlChar *content;
>  
> -        content = gvir_config_xml_get_child_element_content (node, child_name);
> +    content = gvir_config_xml_get_child_element_content(node, child_name);
>  
> -        return libxml_str_to_glib(content);
> +    return (const char *)content;
>  }

Not really useful to have a function whose only use is doing a cast for us,
I'd kill it and have get_child_element_content return a char *. For
functions which take a xmlChar *, cast it to char * and then return it, it
may be better to use BAD_CAST to do the cast (
http://xmlsoft.org/html/libxml-xmlstring.html#BAD_CAST ) so that we can
easily spot these functions if we need to some day.

>  
> -G_GNUC_INTERNAL xmlChar *
> +G_GNUC_INTERNAL const xmlChar *
>  gvir_config_xml_get_attribute_content(xmlNodePtr node, const char *attr_name)
>  {
> -    return xmlGetProp(node, (const xmlChar*)attr_name);
> +    xmlAttr *attr;
> +
> +    for (attr = node->properties; attr; attr = attr->next) {
> +        if (attr->name == NULL)
> +            continue;
> +
> +        if (strcmp (attr_name, (char *)attr->name) == 0)
> +            break;
> +    }
> +
> +    return attr->children->content;
>  }
>  
> -G_GNUC_INTERNAL char *
> +G_GNUC_INTERNAL const char *
>  gvir_config_xml_get_attribute_content_glib(xmlNodePtr node, const char *attr_name)
>  {
> -    xmlChar *attr;
> +    const xmlChar *attr;
>  
>      attr = gvir_config_xml_get_attribute_content(node, attr_name);
>  
> -    return libxml_str_to_glib(attr);
> +    return (const char *) attr;
>  }

Can be killed too.

>  
>  const char *gvir_config_genum_get_nick (GType enum_type, gint value)
> diff --git a/libvirt-gconfig/libvirt-gconfig-object-private.h b/libvirt-gconfig/libvirt-gconfig-object-private.h
> index 41cbfe8..a6b7395 100644
> --- a/libvirt-gconfig/libvirt-gconfig-object-private.h
> +++ b/libvirt-gconfig/libvirt-gconfig-object-private.h
> @@ -31,17 +31,17 @@ GVirConfigObject *gvir_config_object_new_from_tree(GType type,
>                                                     const char *schema,
>                                                     xmlNodePtr tree);
>  xmlNodePtr gvir_config_object_get_xml_node(GVirConfigObject *config);
> -char *gvir_config_object_get_node_content(GVirConfigObject *object,
> -                                          const char *node_name);
> +const char *gvir_config_object_get_node_content(GVirConfigObject *object,
> +                                                const char *node_name);
>  guint64 gvir_config_object_get_node_content_uint64(GVirConfigObject *object,
>                                                     const char *node_name);
>  gint gvir_config_object_get_node_content_genum(GVirConfigObject *object,
>                                                 const char *node_name,
>                                                 GType enum_type,
>                                                 gint default_value);
> -char *gvir_config_object_get_attribute(GVirConfigObject *object,
> -                                       const char *node_name,
> -                                       const char *attr_name);
> +const char *gvir_config_object_get_attribute(GVirConfigObject *object,
> +                                             const char *node_name,
> +                                             const char *attr_name);
>  gint gvir_config_object_get_attribute_genum(GVirConfigObject *object,
>                                              const char *node_name,
>                                              const char *attr_name,
> diff --git a/libvirt-gconfig/libvirt-gconfig-object.c b/libvirt-gconfig/libvirt-gconfig-object.c
> index b637960..d99a0a2 100644
> --- a/libvirt-gconfig/libvirt-gconfig-object.c
> +++ b/libvirt-gconfig/libvirt-gconfig-object.c
> @@ -274,7 +274,7 @@ gvir_config_object_get_xml_node(GVirConfigObject *config)
>      return config->priv->node;
>  }
>  
> -G_GNUC_INTERNAL char *
> +G_GNUC_INTERNAL const char *
>  gvir_config_object_get_node_content(GVirConfigObject *object,
>                                      const char *node_name)
>  {
> @@ -287,7 +287,7 @@ gvir_config_object_get_node_content(GVirConfigObject *object,
>      return gvir_config_xml_get_child_element_content_glib(node, node_name);
>  }
>  
> -G_GNUC_INTERNAL char *
> +G_GNUC_INTERNAL const char *
>  gvir_config_object_get_attribute(GVirConfigObject *object,
>                                   const char *node_name,
>                                   const char *attr_name)
> @@ -559,7 +559,7 @@ gvir_config_object_get_node_content_uint64(GVirConfigObject *object,
>                                             const char *node_name)
>  {
>      xmlNodePtr node;
> -    xmlChar *str;
> +    const xmlChar *str;
>      guint64 value;
>  
>      node = gvir_config_object_get_xml_node(GVIR_CONFIG_OBJECT(object));
> @@ -571,7 +571,6 @@ gvir_config_object_get_node_content_uint64(GVirConfigObject *object,
>          return 0;
>  
>      value = g_ascii_strtoull((char *)str, NULL, 0);
> -    xmlFree(str);
>  
>      return value;
>  }
> @@ -583,7 +582,7 @@ gvir_config_object_get_node_content_genum(GVirConfigObject *object,
>                                            gint default_value)
>  {
>      xmlNodePtr node;
> -    xmlChar *str;
> +    const xmlChar *str;
>      gint value;
>  
>      node = gvir_config_object_get_xml_node(GVIR_CONFIG_OBJECT(object));
> @@ -595,7 +594,6 @@ gvir_config_object_get_node_content_genum(GVirConfigObject *object,
>          return default_value;
>  
>      value = gvir_config_genum_get_value(enum_type, (char *)str, default_value);
> -    xmlFree(str);
>  
>      return value;
>  }
> @@ -608,7 +606,7 @@ gvir_config_object_get_attribute_genum(GVirConfigObject *object,
>                                         gint default_value)
>  {
>      xmlNodePtr node;
> -    xmlChar *attr_val;
> +    const xmlChar *attr_val;
>      gint value;
>  
>      g_return_val_if_fail(attr_name != NULL, default_value);
> @@ -629,7 +627,6 @@ gvir_config_object_get_attribute_genum(GVirConfigObject *object,
>  
>      value = gvir_config_genum_get_value(enum_type, (char *)attr_val,
>                                          default_value);
> -    xmlFree(attr_val);
>  
>      return value;
>  }
> diff --git a/libvirt-gconfig/tests/test-domain-create.c b/libvirt-gconfig/tests/test-domain-create.c
> index a92413d..8c9a6ba 100644
> --- a/libvirt-gconfig/tests/test-domain-create.c
> +++ b/libvirt-gconfig/tests/test-domain-create.c
> @@ -32,10 +32,14 @@
>  
>  const char *features[] = { "foo", "bar", "baz", NULL };
>  
> +#define g_str_const_check(str1, str2) G_STMT_START { \
> +    g_assert((str1) != NULL); \
> +    g_assert(g_strcmp0((str1), (str2)) == 0); \
> +} G_STMT_END
> +
>  #define g_str_check(str1, str2) G_STMT_START { \
>      char *alloced_str = (str1); \
> -    g_assert(alloced_str != NULL); \
> -    g_assert(g_strcmp0(alloced_str, (str2)) == 0); \
> +    g_str_const_check(alloced_str, (str2)); \
>      g_free(alloced_str); \
>  } G_STMT_END

Is g_str_check still useful? Actually, the whole macro is probably useless
now since it's just g_assert(g_strcmp0((str1), (str2)) == 0);

>  
> @@ -51,7 +55,7 @@ int main(int argc, char **argv)
>      domain = gvir_config_domain_new();
>      g_assert(domain != NULL);
>      gvir_config_domain_set_name(domain, "foo");
> -    g_str_check(gvir_config_domain_get_name(domain), "foo");
> +    g_str_const_check(gvir_config_domain_get_name(domain), "foo");
>  
>      gvir_config_domain_set_memory(domain, 1234);
>      g_assert(gvir_config_domain_get_memory(domain) == 1234);
> @@ -113,12 +117,12 @@ int main(int argc, char **argv)
>  
>      g_assert(gvir_config_domain_disk_get_disk_type(disk) == GVIR_CONFIG_DOMAIN_DISK_FILE);
>      g_assert(gvir_config_domain_disk_get_guest_device_type(disk) == GVIR_CONFIG_DOMAIN_DISK_GUEST_DEVICE_DISK);
> -    g_str_check(gvir_config_domain_disk_get_source(disk), "/tmp/foo/bar");
> +    g_str_const_check(gvir_config_domain_disk_get_source(disk), "/tmp/foo/bar");
>      g_assert(gvir_config_domain_disk_get_driver_cache(disk) == GVIR_CONFIG_DOMAIN_DISK_CACHE_NONE);
> -    g_str_check(gvir_config_domain_disk_get_driver_name(disk), "qemu");
> -    g_str_check(gvir_config_domain_disk_get_driver_type(disk), "qcow2");
> +    g_str_const_check(gvir_config_domain_disk_get_driver_name(disk), "qemu");
> +    g_str_const_check(gvir_config_domain_disk_get_driver_type(disk), "qcow2");
>      g_assert(gvir_config_domain_disk_get_target_bus(disk) == GVIR_CONFIG_DOMAIN_DISK_BUS_IDE);
> -    g_str_check(gvir_config_domain_disk_get_target_dev(disk), "hda");
> +    g_str_const_check(gvir_config_domain_disk_get_target_dev(disk), "hda");
>  
>  
>      /* network interfaces node */
> diff --git a/libvirt-gconfig/tests/test-domain-parse.c b/libvirt-gconfig/tests/test-domain-parse.c
> index c264ff9..11880de 100644
> --- a/libvirt-gconfig/tests/test-domain-parse.c
> +++ b/libvirt-gconfig/tests/test-domain-parse.c
> @@ -34,7 +34,7 @@
>  int main(int argc, char **argv)
>  {
>      GVirConfigDomain *domain;
> -    char *name;
> +    const char *name;
>      GStrv features;
>      char *xml;
>      GError *error = NULL;
> @@ -69,7 +69,6 @@ int main(int argc, char **argv)
>      name = gvir_config_domain_get_name(domain);
>      g_assert(name != NULL);
>      g_assert(strcmp(name, "foo") == 0);
> -    g_free(name);
>  
>      g_assert(gvir_config_domain_get_memory(domain) == 987654321);
>  
> diff --git a/libvirt-gobject/libvirt-gobject-domain-disk.c b/libvirt-gobject/libvirt-gobject-domain-disk.c
> index d8fb63d..fb85328 100644
> --- a/libvirt-gobject/libvirt-gobject-domain-disk.c
> +++ b/libvirt-gobject/libvirt-gobject-domain-disk.c
> @@ -90,10 +90,10 @@ 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 gchar *gvir_domain_disk_get_path(GVirDomainDisk *self)
> +static const gchar *gvir_domain_disk_get_path(GVirDomainDisk *self)
>  {
>      GVirConfigDomainDevice *config;
> -    gchar *path;
> +    const gchar *path;
>  
>      config = gvir_domain_device_get_config(GVIR_DOMAIN_DEVICE(self));
>      path = gvir_config_domain_disk_get_target_dev(GVIR_CONFIG_DOMAIN_DISK(config));
> @@ -119,7 +119,7 @@ GVirDomainDiskStats *gvir_domain_disk_get_stats(GVirDomainDisk *self, GError **e
>      GVirDomainDiskStats *ret = NULL;
>      virDomainBlockStatsStruct stats;
>      virDomainPtr handle;
> -    gchar *path;
> +    const gchar *path;
>  
>      g_return_val_if_fail(GVIR_IS_DOMAIN_DISK(self), NULL);
>  
> @@ -142,7 +142,6 @@ GVirDomainDiskStats *gvir_domain_disk_get_stats(GVirDomainDisk *self, GError **e
>  
>  end:
>      virDomainFree(handle);
> -    g_free(path);
>      return ret;
>  }
>  
> @@ -164,7 +163,7 @@ gboolean gvir_domain_disk_resize(GVirDomainDisk *self,
>  {
>      gboolean ret = FALSE;
>      virDomainPtr handle;
> -    gchar *path;
> +    const gchar *path;
>  
>      g_return_val_if_fail(GVIR_IS_DOMAIN_DISK(self), FALSE);
>      g_return_val_if_fail(err == NULL || *err == NULL, FALSE);
> @@ -183,6 +182,5 @@ gboolean gvir_domain_disk_resize(GVirDomainDisk *self,
>  
>  end:
>      virDomainFree(handle);
> -    g_free(path);
>      return ret;
>  }
> diff --git a/libvirt-gobject/libvirt-gobject-domain-interface.c b/libvirt-gobject/libvirt-gobject-domain-interface.c
> index 4436466..9f4b30d 100644
> --- a/libvirt-gobject/libvirt-gobject-domain-interface.c
> +++ b/libvirt-gobject/libvirt-gobject-domain-interface.c
> @@ -88,10 +88,10 @@ gvir_domain_interface_stats_free(GVirDomainInterfaceStats *stats)
>  G_DEFINE_BOXED_TYPE(GVirDomainInterfaceStats, gvir_domain_interface_stats,
>                      gvir_domain_interface_stats_copy, gvir_domain_interface_stats_free)
>  
> -static gchar *gvir_domain_interface_get_path(GVirDomainInterface *self)
> +static const gchar *gvir_domain_interface_get_path(GVirDomainInterface *self)
>  {
>      GVirConfigDomainDevice *config;
> -    gchar *path = NULL;
> +    const gchar *path = NULL;
>  
>      config = gvir_domain_device_get_config(GVIR_DOMAIN_DEVICE(self));
>      if (GVIR_CONFIG_IS_DOMAIN_INTERFACE_USER(self))
> @@ -121,7 +121,7 @@ GVirDomainInterfaceStats *gvir_domain_interface_get_stats(GVirDomainInterface *s
>      GVirDomainInterfaceStats *ret = NULL;
>      virDomainInterfaceStatsStruct stats;
>      virDomainPtr handle;
> -    gchar *path;
> +    const gchar *path;
>  
>      g_return_val_if_fail(GVIR_IS_DOMAIN_INTERFACE(self), NULL);
>  
> @@ -151,6 +151,5 @@ GVirDomainInterfaceStats *gvir_domain_interface_get_stats(GVirDomainInterface *s
>  
>  end:
>      virDomainFree(handle);
> -    g_free(path);
>      return ret;
>  }
> -- 
> 1.7.7.6
> 
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: pgppEX4gHkpvg.pgp
Description: PGP signature


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