Re: [PATCH 4/5] storage_conf: Left fixes or improvements for storage_conf.c

["Daniel P. Berrange" <berrange@xxxxxxxxxx>]

On Thu, May 16, 2013 at 08:40:53PM +0800, Osier Yang wrote:
> virStorageVolDefParseXML:
>   * Create "virStorageVolDefPtr def", and use ret to track the
>     return value; frees the strings at "cleanup" label instead
>     of freeing them in the middle.
> 
> virStorageVolDefFormat:
>   * Use macro NULLSTR
> ---
>  src/conf/storage_conf.c | 93 ++++++++++++++++++++++++-------------------------
>  1 file changed, 46 insertions(+), 47 deletions(-)
> 
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index dd55d2c..431a8eb 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -972,8 +972,8 @@ virStoragePoolDefParseNode(xmlDocPtr xml,
>      virStoragePoolDefPtr def = NULL;
>  
>      if (STRNEQ((const char *)root->name, "pool")) {
> -        virReportError(VIR_ERR_XML_ERROR,
> -                       "%s", _("unknown root element for storage pool"));
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("unknown root element for storage pool"));
>          goto cleanup;
>      }
>  
> @@ -1149,8 +1149,8 @@ virStoragePoolDefFormat(virStoragePoolDefPtr def)
>  
>      type = virStoragePoolTypeToString(def->type);
>      if (!type) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       "%s", _("unexpected pool type"));
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("unexpected pool type"));
>          goto cleanup;
>      }
>      virBufferAsprintf(&buf, "<pool type='%s'>\n", type);
> @@ -1214,8 +1214,8 @@ virStorageSize(const char *unit,
>                 unsigned long long *ret)
>  {
>      if (virStrToLong_ull(val, NULL, 10, ret) < 0) {
> -        virReportError(VIR_ERR_XML_ERROR,
> -                       "%s", _("malformed capacity element"));
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("malformed capacity element"));
>          return -1;
>      }
>      /* off_t is signed, so you cannot create a file larger than 2**63
> @@ -1230,64 +1230,62 @@ static virStorageVolDefPtr
>  virStorageVolDefParseXML(virStoragePoolDefPtr pool,
>                           xmlXPathContextPtr ctxt)
>  {
> -    virStorageVolDefPtr ret;
> +    virStorageVolDefPtr def;
>      virStorageVolOptionsPtr options;
>      char *allocation = NULL;
>      char *capacity = NULL;
>      char *unit = NULL;
>      xmlNodePtr node;
> +    virStorageVolDefPtr ret = NULL;

IMHO having 2 variables for the same thing is a bit odd. IOW, I
think the current code is better than what you've done here. If
you want to have unified cleanup, do it like


     cleanup:
       VIR_FREE(allocation);
       VIR_FREE(capacity);
       VIR_FREE(unit);
       return ret;

     error:
       virStorageVolDefFree(ret);
       ret = NULL;
       goto cleanup;


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list