[libvirt] [PATCH 06/18] storage: util: Clean up arguments of virStorageFileGetMetadataInternal

[Eric Blake]

On 04/20/2014 04:13 PM, Peter Krempa wrote:
> Avoid passing lot of arguments into guts of metadata retrieval to fill
> the actual structure. Temporarily fill the structure before passing it
> down to the actual metadata extractor.
> 
> This will later help the inversion of the steps taken to extract the
> metadata so that this function can be fully converted to
> virStorageSource as the data struct.
> ---
>  src/util/virstoragefile.c | 164 +++++++++++++++++++++++-----------------------
>  1 file changed, 81 insertions(+), 83 deletions(-)
> 

> -/* Given a header in BUF with length LEN, as parsed from the file with
> - * user-provided name PATH and opened from CANONPATH, and where any
> - * relative backing file will be opened from DIRECTORY, and assuming
> - * it has the given FORMAT, populate the newly-allocated META with
> - * information about the file and its backing store.  */
> +/* Given a header in BUF with length LEN, as parsed from the storage file
> + * assuming it has the given FORMAT, populate information into META
> + * with information about the file and its backing store. Return format
> + * of the backing store as BACKING_FORMAT. PATH and FORMAT have to be
> + * pre-populated in META*/

Cosmetic - I usually stick space before */

>  {
>      int ret = -1;
> 
> -    VIR_DEBUG("path=%s, canonPath=%s, dir=%s, buf=%p, len=%zu, format=%d",
> -              path, canonPath, directory, buf, len, format);
> +    VIR_DEBUG("path=%s, buf=%p, len=%zu, meta->format=%d",
> +              meta->path, buf, len, meta->format);

The diff would be slightly smaller if you do:

int format = meta->format;

up front, then keep the original use of format everywhere else...

> 
> -    if (VIR_STRDUP(meta->path, path) < 0)
> -        goto cleanup;
> -    if (VIR_STRDUP(meta->canonPath, canonPath) < 0)
> -        goto cleanup;
> -    if (VIR_STRDUP(meta->relDir, directory) < 0)
> -        goto cleanup;
> +    if (meta->format == VIR_STORAGE_FILE_AUTO)
> +        meta->format = virStorageFileProbeFormatFromBuf(meta->path, buf, len);

...well, right here, you'd have to do 'format = meta->format =
virStorage...();', but the rest of the function has fewer changes.  But
it doesn't matter to me; I'm fine keeping it as you wrote it.

> +static virStorageFileMetadataPtr
> +virStorageFileMetadataNew(const char *path,
> +                          int format)
> +{
> +    virStorageFileMetadataPtr ret = NULL;
> +
> +    if (VIR_ALLOC(ret) < 0)
> +        return NULL;
> +
> +    ret->format = format;
> +    ret->type = VIR_STORAGE_TYPE_FILE;
> +
> +    if (VIR_STRDUP(ret->path, path) < 0)
> +        goto error;
> +
> +    if (!(ret->canonPath = canonicalize_file_name(path))) {

Same complaint as in 3/18 - you should only call
canonicalize_file_name() if you KNOW that path should be interpreted as
a file name.

> @@ -979,19 +994,11 @@ virStorageFileGetMetadataFromBuf(const char *path,
>                                   int *backingFormat)
>  {
>      virStorageFileMetadataPtr ret = NULL;
> -    char *canonPath;
> 
> -    if (!(canonPath = canonicalize_file_name(path)) &&
> -        VIR_STRDUP(canonPath, path) < 0) {
> -        virReportSystemError(errno, _("unable to resolve '%s'"), path);
> +    if (!(ret = virStorageFileMetadataNew(path, format)))
>          return NULL;
> -    }

Minor merge conflicts with my suggested resolution to 3/18 - but should
be obvious resolution.

> @@ -1071,7 +1072,6 @@ virStorageFileGetMetadataFromFDInternal(const char *path,
>      return ret;
>  }
> 
> -
>  /**

Spurious whitespace change.

ACK with this squashed in (and any additional trivial cleanups you want
to optionally do based on my comments above):

diff --git i/src/util/virstoragefile.c w/src/util/virstoragefile.c
index b0fe2ae..ff8de43 100644
--- i/src/util/virstoragefile.c
+++ w/src/util/virstoragefile.c
@@ -948,8 +948,12 @@ virStorageFileMetadataNew(const char *path,
     if (VIR_STRDUP(ret->path, path) < 0)
         goto error;

-    if (!(ret->canonPath = canonicalize_file_name(path))) {
-        virReportSystemError(errno, _("unable to resolve '%s'"), path);
+    if (virStorageIsFile(path)) {
+        if (!(ret->canonPath = canonicalize_file_name(path))) {
+            virReportSystemError(errno, _("unable to resolve '%s'"), path);
+            goto error;
+        }
+    } else if (VIR_STRDUP(ret->canonPath, path) < 0) {
         goto error;
     }

@@ -1072,6 +1076,7 @@
virStorageFileGetMetadataFromFDInternal(virStorageFileMetadataPtr meta,
     return ret;
 }

+
 /**
  * virStorageFileGetMetadataFromFD:
  *


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140422/883162eb/attachment-0001.sig>