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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list