[libvirt] [PATCHv2 3/5] storage: refactor metadata lookup

[Peter Krempa]

On 02/15/13 21:38, Eric Blake wrote:
> Prior to this patch, we had the callchains:
> external users
>    \-> virStorageFileGetMetadataFromFD
>        \-> virStorageFileGetMetadataFromBuf
> virStorageFileGetMetadataRecurse
>    \-> virStorageFileGetMetadataFromFD
>        \-> virStorageFileGetMetadataFromBuf
>
> However, a future patch wants to add an additional parameter to
> the bottom of the chain, for use by virStorageFileGetMetadataRecurse,
> without affecting existing external callers.  Since there is only a
> single caller of the internal function, we can repurpose it to fit
> our needs, with this patch giving us:
>
> external users
>    \-> virStorageFileGetMetadataFromFD
>        \-> virStorageFileGetMetadataInternal
> virStorageFileGetMetadataRecurse /
>    \-> virStorageFileGetMetadataInternal
>
> * src/util/virstoragefile.c (virStorageFileGetMetadataFromFD):
> Move most of the guts...
> (virStorageFileGetMetadataFromBuf): ...here, and rename...
> (virStorageFileGetMetadataInternal): ...to this.
> (virStorageFileGetMetadataRecurse): Use internal helper.
> ---
>   src/util/virstoragefile.c | 122 +++++++++++++++++++++-------------------------
>   1 file changed, 56 insertions(+), 66 deletions(-)
>
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 83b00e2..d741d3d 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -658,22 +658,63 @@ cleanup:
>
>
>   static virStorageFileMetadataPtr
> -virStorageFileGetMetadataFromBuf(int format,
> -                                 const char *path,
> -                                 unsigned char *buf,
> -                                 size_t len,
> -                                 virStorageFileMetadata *meta)
> +virStorageFileGetMetadataInternal(const char *path,
> +                                  int fd,
> +                                  int format)
>   {
> +    virStorageFileMetadata *meta = NULL;
> +    unsigned char *buf = NULL;
> +    ssize_t len = STORAGE_MAX_HEAD;
>       virStorageFileMetadata *ret = NULL;
> +    struct stat sb;
> +
> +    VIR_DEBUG("path=%s, fd=%d, format=%d", path, fd, format);
>
> -    VIR_DEBUG("path=%s format=%d", path, format);
> +    if (VIR_ALLOC(meta) < 0) {
> +        virReportOOMError();
> +        return NULL;
> +    }
> +
> +    if (fstat(fd, &sb) < 0) {
> +        virReportSystemError(errno,
> +                             _("cannot stat file '%s'"),
> +                             path);
> +        goto cleanup;
> +    }
> +
> +    /* No header to probe for directories, but also no backing file */
> +    if (S_ISDIR(sb.st_mode))
> +        return meta;
> +
> +    if (lseek(fd, 0, SEEK_SET) == (off_t)-1) {
> +        virReportSystemError(errno, _("cannot seek to start of '%s'"), path);
> +        goto cleanup;
> +    }
> +
> +    if (VIR_ALLOC_N(buf, len) < 0) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
> +    if ((len = read(fd, buf, len)) < 0) {
> +        virReportSystemError(errno, _("cannot read header '%s'"), path);

Preexisting, but: s/header/header of/

> +        goto cleanup;
> +    }
> +
> +    if (format == VIR_STORAGE_FILE_AUTO)
> +        format = virStorageFileProbeFormatFromBuf(path, buf, len);
> +
> +    if (format <= VIR_STORAGE_FILE_NONE ||
> +        format >= VIR_STORAGE_FILE_LAST) {
> +        virReportSystemError(EINVAL, _("unknown storage file format %d"),
> +                             format);
> +        goto cleanup;
> +    }
>
>       /* XXX we should consider moving virStorageBackendUpdateVolInfo
>        * code into this method, for non-magic files
>        */
> -    if (format <= VIR_STORAGE_FILE_NONE ||
> -        format >= VIR_STORAGE_FILE_LAST ||
> -        !fileTypeInfo[format].magic)
> +    if (!fileTypeInfo[format].magic)
>           goto done;
>
>       /* Optionally extract capacity from file */
> @@ -747,7 +788,11 @@ virStorageFileGetMetadataFromBuf(int format,
>
>   done:
>       ret = meta;
> +    meta = NULL;
> +
>   cleanup:
> +    virStorageFileFreeMetadata(meta);
> +    VIR_FREE(buf);
>       return ret;
>   }

Ah, now the change makes sense.

>

ACK.

Peter