[libvirt] [PATCHv2 0/5] fix regression in symlink'd qcow2 backing files
See https://bugzilla.redhat.com/show_bug.cgi?id=903248 for more gory details. This fixes a regression introduced in commit 8250783. V1 was here: https://www.redhat.com/archives/libvir-list/2013-February/msg00340.html changes since then: rebase to latest, add a testsuite, split one patch into two for easier review, and push already-acked changes. Eric Blake (5): storage: rearrange functions storage: prepare for refactoring storage: refactor metadata lookup storage: don't follow backing chain symlinks too eagerly storage: test backing chain traversal .gitignore | 1 + src/util/virstoragefile.c | 314 +++++++++++++------------- src/util/virstoragefile.h | 1 + tests/Makefile.am | 6 + tests/virstoragetest.c | 546 ++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 720 insertions(+), 148 deletions(-) create mode 100644 tests/virstoragetest.c -- 1.8.1.2
No semantic change; done so the next patch doesn't need a forward declaration of a static function. * src/util/virstoragefile.c (virStorageFileProbeFormatFromBuf): Hoist earlier. --- src/util/virstoragefile.c | 82 +++++++++++++++++++++++------------------------ 1 file changed, 41 insertions(+), 41 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index dc28539..f2cbaa1 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -617,6 +617,47 @@ virBackingStoreIsFile(const char *backing) } static int +virStorageFileProbeFormatFromBuf(const char *path, + unsigned char *buf, + size_t buflen) +{ + int format = VIR_STORAGE_FILE_RAW; + int i; + int possibleFormat = VIR_STORAGE_FILE_RAW; + VIR_DEBUG("path=%s", path); + + /* First check file magic */ + for (i = 0 ; i < VIR_STORAGE_FILE_LAST ; i++) { + if (virStorageFileMatchesMagic(i, buf, buflen)) { + if (!virStorageFileMatchesVersion(i, buf, buflen)) { + possibleFormat = i; + continue; + } + format = i; + goto cleanup; + } + } + + if (possibleFormat != VIR_STORAGE_FILE_RAW) + VIR_WARN("File %s matches %s magic, but version is wrong. " + "Please report new version to libvir-list@redhat.com", + path, virStorageFileFormatTypeToString(possibleFormat)); + + /* No magic, so check file extension */ + for (i = 0 ; i < VIR_STORAGE_FILE_LAST ; i++) { + if (virStorageFileMatchesExtension(i, path)) { + format = i; + goto cleanup; + } + } + +cleanup: + VIR_DEBUG("format=%d", format); + return format; +} + + +static int virStorageFileGetMetadataFromBuf(int format, const char *path, unsigned char *buf, @@ -707,47 +748,6 @@ virStorageFileGetMetadataFromBuf(int format, } -static int -virStorageFileProbeFormatFromBuf(const char *path, - unsigned char *buf, - size_t buflen) -{ - int format = VIR_STORAGE_FILE_RAW; - int i; - int possibleFormat = VIR_STORAGE_FILE_RAW; - VIR_DEBUG("path=%s", path); - - /* First check file magic */ - for (i = 0 ; i < VIR_STORAGE_FILE_LAST ; i++) { - if (virStorageFileMatchesMagic(i, buf, buflen)) { - if (!virStorageFileMatchesVersion(i, buf, buflen)) { - possibleFormat = i; - continue; - } - format = i; - goto cleanup; - } - } - - if (possibleFormat != VIR_STORAGE_FILE_RAW) - VIR_WARN("File %s matches %s magic, but version is wrong. " - "Please report new version to libvir-list@redhat.com", - path, virStorageFileFormatTypeToString(possibleFormat)); - - /* No magic, so check file extension */ - for (i = 0 ; i < VIR_STORAGE_FILE_LAST ; i++) { - if (virStorageFileMatchesExtension(i, path)) { - format = i; - goto cleanup; - } - } - -cleanup: - VIR_DEBUG("format=%d", format); - return format; -} - - /** * virStorageFileProbeFormatFromFD: * -- 1.8.1.2
On 02/15/13 21:38, Eric Blake wrote:
No semantic change; done so the next patch doesn't need a forward declaration of a static function.
* src/util/virstoragefile.c (virStorageFileProbeFormatFromBuf): Hoist earlier. --- src/util/virstoragefile.c | 82 +++++++++++++++++++++++------------------------ 1 file changed, 41 insertions(+), 41 deletions(-)
Trivial. ACK. Peter
virStorageFileGetMetadataFromFD is the only caller of virStorageFileGetMetadataFromBuf; and it doesn't care about the difference between a return of 0 (total success) or 1 (metadata was inconsistent, but pointer was populated as best as possible); only about a return of -1 (could not read metadata or out of memory). Changing the return type, and normalizing the variable names used, will make merging the functions easier in the next commit. * src/util/virstoragefile.c (virStorageFileGetMetadataFromBuf): Change return value, and rename some variables. (virStorageFileGetMetadataFromFD): Rename some variables. --- src/util/virstoragefile.c | 50 +++++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 23 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index f2cbaa1..83b00e2 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -657,13 +657,15 @@ cleanup: } -static int +static virStorageFileMetadataPtr virStorageFileGetMetadataFromBuf(int format, const char *path, unsigned char *buf, - size_t buflen, + size_t len, virStorageFileMetadata *meta) { + virStorageFileMetadata *ret = NULL; + VIR_DEBUG("path=%s format=%d", path, format); /* XXX we should consider moving virStorageBackendUpdateVolInfo @@ -671,14 +673,13 @@ virStorageFileGetMetadataFromBuf(int format, */ if (format <= VIR_STORAGE_FILE_NONE || format >= VIR_STORAGE_FILE_LAST || - !fileTypeInfo[format].magic) { - return 0; - } + !fileTypeInfo[format].magic) + goto done; /* Optionally extract capacity from file */ if (fileTypeInfo[format].sizeOffset != -1) { - if ((fileTypeInfo[format].sizeOffset + 8) > buflen) - return 1; + if ((fileTypeInfo[format].sizeOffset + 8) > len) + goto done; if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN) meta->capacity = virReadBufInt64LE(buf + @@ -689,7 +690,7 @@ virStorageFileGetMetadataFromBuf(int format, /* Avoid unlikely, but theoretically possible overflow */ if (meta->capacity > (ULLONG_MAX / fileTypeInfo[format].sizeMultiplier)) - return 1; + goto done; meta->capacity *= fileTypeInfo[format].sizeMultiplier; } @@ -704,14 +705,14 @@ virStorageFileGetMetadataFromBuf(int format, if (fileTypeInfo[format].getBackingStore != NULL) { char *backing; int backingFormat; - int ret = fileTypeInfo[format].getBackingStore(&backing, - &backingFormat, - buf, buflen); - if (ret == BACKING_STORE_INVALID) - return 1; + int store = fileTypeInfo[format].getBackingStore(&backing, + &backingFormat, + buf, len); + if (store == BACKING_STORE_INVALID) + goto done; - if (ret == BACKING_STORE_ERROR) - return -1; + if (store == BACKING_STORE_ERROR) + goto cleanup; meta->backingStoreIsFile = false; if (backing != NULL) { @@ -719,7 +720,7 @@ virStorageFileGetMetadataFromBuf(int format, if (meta->backingStore == NULL) { virReportOOMError(); VIR_FREE(backing); - return -1; + goto cleanup; } if (virBackingStoreIsFile(backing)) { meta->backingStoreIsFile = true; @@ -744,7 +745,10 @@ virStorageFileGetMetadataFromBuf(int format, } } - return 0; +done: + ret = meta; +cleanup: + return ret; } @@ -858,7 +862,7 @@ virStorageFileGetMetadataFromFD(const char *path, int format) { virStorageFileMetadata *meta = NULL; - unsigned char *head = NULL; + unsigned char *buf = NULL; ssize_t len = STORAGE_MAX_HEAD; virStorageFileMetadata *ret = NULL; struct stat sb; @@ -884,18 +888,18 @@ virStorageFileGetMetadataFromFD(const char *path, goto cleanup; } - if (VIR_ALLOC_N(head, len) < 0) { + if (VIR_ALLOC_N(buf, len) < 0) { virReportOOMError(); goto cleanup; } - if ((len = read(fd, head, len)) < 0) { + if ((len = read(fd, buf, len)) < 0) { virReportSystemError(errno, _("cannot read header '%s'"), path); goto cleanup; } if (format == VIR_STORAGE_FILE_AUTO) - format = virStorageFileProbeFormatFromBuf(path, head, len); + format = virStorageFileProbeFormatFromBuf(path, buf, len); if (format <= VIR_STORAGE_FILE_NONE || format >= VIR_STORAGE_FILE_LAST) { @@ -904,14 +908,14 @@ virStorageFileGetMetadataFromFD(const char *path, goto cleanup; } - if (virStorageFileGetMetadataFromBuf(format, path, head, len, meta) < 0) + if (!virStorageFileGetMetadataFromBuf(format, path, buf, len, meta)) goto cleanup; ret = meta; meta = NULL; cleanup: virStorageFileFreeMetadata(meta); - VIR_FREE(head); + VIR_FREE(buf); return ret; } -- 1.8.1.2
On 02/15/13 21:38, Eric Blake wrote:
virStorageFileGetMetadataFromFD is the only caller of virStorageFileGetMetadataFromBuf; and it doesn't care about the difference between a return of 0 (total success) or 1 (metadata was inconsistent, but pointer was populated as best as possible); only about a return of -1 (could not read metadata or out of memory). Changing the return type, and normalizing the variable names used, will make merging the functions easier in the next commit.
* src/util/virstoragefile.c (virStorageFileGetMetadataFromBuf): Change return value, and rename some variables. (virStorageFileGetMetadataFromFD): Rename some variables. --- src/util/virstoragefile.c | 50 +++++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 23 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index f2cbaa1..83b00e2 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -657,13 +657,15 @@ cleanup: }
-static int +static virStorageFileMetadataPtr virStorageFileGetMetadataFromBuf(int format, const char *path, unsigned char *buf, - size_t buflen, + size_t len, virStorageFileMetadata *meta) { + virStorageFileMetadata *ret = NULL; + VIR_DEBUG("path=%s format=%d", path, format);
/* XXX we should consider moving virStorageBackendUpdateVolInfo @@ -671,14 +673,13 @@ virStorageFileGetMetadataFromBuf(int format, */ if (format <= VIR_STORAGE_FILE_NONE || format >= VIR_STORAGE_FILE_LAST || - !fileTypeInfo[format].magic) { - return 0; - } + !fileTypeInfo[format].magic) + goto done;
/* Optionally extract capacity from file */ if (fileTypeInfo[format].sizeOffset != -1) { - if ((fileTypeInfo[format].sizeOffset + 8) > buflen) - return 1; + if ((fileTypeInfo[format].sizeOffset + 8) > len) + goto done;
if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN) meta->capacity = virReadBufInt64LE(buf + @@ -689,7 +690,7 @@ virStorageFileGetMetadataFromBuf(int format, /* Avoid unlikely, but theoretically possible overflow */ if (meta->capacity > (ULLONG_MAX / fileTypeInfo[format].sizeMultiplier)) - return 1; + goto done; meta->capacity *= fileTypeInfo[format].sizeMultiplier; }
@@ -704,14 +705,14 @@ virStorageFileGetMetadataFromBuf(int format, if (fileTypeInfo[format].getBackingStore != NULL) { char *backing; int backingFormat; - int ret = fileTypeInfo[format].getBackingStore(&backing, - &backingFormat, - buf, buflen); - if (ret == BACKING_STORE_INVALID) - return 1; + int store = fileTypeInfo[format].getBackingStore(&backing, + &backingFormat, + buf, len); + if (store == BACKING_STORE_INVALID) + goto done;
- if (ret == BACKING_STORE_ERROR) - return -1; + if (store == BACKING_STORE_ERROR) + goto cleanup;
meta->backingStoreIsFile = false; if (backing != NULL) { @@ -719,7 +720,7 @@ virStorageFileGetMetadataFromBuf(int format, if (meta->backingStore == NULL) { virReportOOMError(); VIR_FREE(backing); - return -1; + goto cleanup; } if (virBackingStoreIsFile(backing)) { meta->backingStoreIsFile = true; @@ -744,7 +745,10 @@ virStorageFileGetMetadataFromBuf(int format, } }
- return 0; +done: + ret = meta;
Um, is the ret variable really needed here? The only value it can take is "meta" and return it right after. If this isn't needed in the next patches I'd rather go for "return meta" here.
+cleanup:
Rename this to "error"
+ return ret;
And "return NULL" instead;
}
ACK if: 1) this change will be needed later 2) tweaked according to my suggestions Peter
On 02/15/2013 02:22 PM, Peter Krempa wrote:
- return 0; +done: + ret = meta;
Um, is the ret variable really needed here? The only value it can take is "meta" and return it right after. If this isn't needed in the next patches I'd rather go for "return meta" here.
+cleanup:
Rename this to "error"
+ return ret;
And "return NULL" instead;
Next patch turns moves the allocation into this function, at which point the tail end becomes: done: ret = meta; meta = NULL; cleanup: virStorageFileFreeMetadata(meta); VIR_FREE(buf); return ret;
}
ACK if: 1) this change will be needed later 2) tweaked according to my suggestions
I'm going with option 1 - the labels make more sense once patch 3 is in place. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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); + 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; } @@ -861,62 +906,7 @@ virStorageFileGetMetadataFromFD(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; - - 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); - 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; - } - - if (!virStorageFileGetMetadataFromBuf(format, path, buf, len, meta)) - goto cleanup; - ret = meta; - meta = NULL; - -cleanup: - virStorageFileFreeMetadata(meta); - VIR_FREE(buf); - return ret; + return virStorageFileGetMetadataInternal(path, fd, format); } /* Recursive workhorse for virStorageFileGetMetadata. */ @@ -945,7 +935,7 @@ virStorageFileGetMetadataRecurse(const char *path, int format, return NULL; } - ret = virStorageFileGetMetadataFromFD(path, fd, format); + ret = virStorageFileGetMetadataInternal(path, fd, format); if (VIR_CLOSE(fd) < 0) VIR_WARN("could not close file %s", path); -- 1.8.1.2
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
If you have a qcow2 file /path1/to/file pointed to by symlink /path2/symlink, and pass qemu /path2/symlink, then qemu treats a relative backing file in the qcow2 metadata as being relative to /path2, not /path1/to. Yes, this means that it is possible to create a qcow2 file where the choice of WHICH directory and symlink you access its contents from will then determine WHICH backing file (if any) you actually find; the results can be rather screwy, but we have to match what qemu does. Libvirt and qemu default to creating absolute backing file names, so most users don't hit this. But at least VDSM uses symlinks and relative backing names alongside the --reuse-external flags to libvirt snapshot operations, with the result that libvirt was failing to follow the intended chain of backing files, and then backing files were not granted the necessary sVirt permissions to be opened by qemu. See https://bugzilla.redhat.com/show_bug.cgi?id=903248 for more gory details. This fixes a regression introduced in commit 8250783. I tested this patch by creating the following chain: ls /home/eblake/Downloads/Fedora.iso # raw file for base cd /var/lib/libvirt/images qemu-img create -f qcow2 \ -obacking_file=/home/eblake/Downloads/Fedora.iso,backing_fmt=raw one mkdir sub cd sub ln -s ../one onelink qemu-img create -f qcow2 \ -obacking_file=../sub/onelink,backing_fmt=qcow2 two mv two .. ln -s ../two twolink qemu-img create -f qcow2 \ -obacking_file=../sub/twolink,backing_fmt=qcow2 three mv three .. ln -s ../three threelink then pointing my domain at /var/lib/libvirt/images/sub/threelink. Prior to this patch, I got complaints about missing backing files; afterwards, I was able to verify that the backing chain (and hence DAC and SELinux relabels) of the entire chain worked. * src/util/virstoragefile.h (_virStorageFileMetadata): Add directory member. * src/util/virstoragefile.c (absolutePathFromBaseFile): Drop, replaced by... (virFindBackingFile): ...better function. (virStorageFileGetMetadataInternal): Add an argument. (virStorageFileGetMetadataFromFD, virStorageFileChainLookup) (virStorageFileGetMetadata): Update callers. --- src/util/virstoragefile.c | 88 ++++++++++++++++++++++++++++++----------------- src/util/virstoragefile.h | 1 + 2 files changed, 57 insertions(+), 32 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index d741d3d..5bb2b58 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -497,44 +497,56 @@ qedGetBackingStore(char **res, } /** - * Return an absolute path corresponding to PATH, which is absolute or relative - * to the directory containing BASE_FILE, or NULL on error + * Given a starting point START (either an original file name, or the + * directory containing the original name, depending on START_IS_DIR) + * and a possibly relative backing file NAME, compute the relative + * DIRECTORY (optional) and CANONICAL (mandatory) location of the + * backing file. Return 0 on success, negative on error. */ -static char * -absolutePathFromBaseFile(const char *base_file, const char *path) +static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(5) +virFindBackingFile(const char *start, bool start_is_dir, const char *path, + char **directory, char **canonical) { - char *res = NULL; - char *tmp = NULL; - size_t d_len = dir_len(base_file); + char *combined = NULL; + int ret = -1; - /* If path is already absolute, or if dirname(base_file) is ".", - just return a copy of path. */ - if (*path == '/' || d_len == 0) { - if (!(res = canonicalize_file_name(path))) - virReportSystemError(errno, - _("Can't canonicalize path '%s'"), path); + if (*path == '/') { + /* Safe to cast away const */ + combined = (char *)path; + } else { + size_t d_len = start_is_dir ? strlen(start) : dir_len(start); - goto cleanup; + if (d_len > INT_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("name too long: '%s'"), start); + goto cleanup; + } else if (!d_len) { + start = "."; + d_len = 1; + } + if (virAsprintf(&combined, "%.*s/%s", (int)d_len, start, path) < 0) { + virReportOOMError(); + goto cleanup; + } } - /* Ensure that the following cast-to-int is valid. */ - if (d_len > INT_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Directory name too long: '%s'"), base_file); + if (directory && !(*directory = mdir_name(combined))) { + virReportOOMError(); goto cleanup; } - if (virAsprintf(&tmp, "%.*s/%s", (int) d_len, base_file, path) < 0) { - virReportOOMError(); + if (!(*canonical = canonicalize_file_name(combined))) { + virReportSystemError(errno, + _("Can't canonicalize path '%s'"), path); goto cleanup; } - if (!(res = canonicalize_file_name(tmp))) - virReportSystemError(errno, _("Can't canonicalize path '%s'"), path); + ret = 0; cleanup: - VIR_FREE(tmp); - return res; + if (combined != path) + VIR_FREE(combined); + return ret; } @@ -657,9 +669,13 @@ cleanup: } +/* Given a file descriptor FD open on PATH, and optionally opened from + * a given DIRECTORY, return metadata about that file, assuming it has + * the given FORMAT. */ static virStorageFileMetadataPtr virStorageFileGetMetadataInternal(const char *path, int fd, + const char *directory, int format) { virStorageFileMetadata *meta = NULL; @@ -766,8 +782,11 @@ virStorageFileGetMetadataInternal(const char *path, if (virBackingStoreIsFile(backing)) { meta->backingStoreIsFile = true; meta->backingStoreRaw = meta->backingStore; - meta->backingStore = absolutePathFromBaseFile(path, backing); - if (meta->backingStore == NULL) { + meta->backingStore = NULL; + if (virFindBackingFile(directory ? directory : path, + !!directory, backing, + &meta->directory, + &meta->backingStore) < 0) { /* the backing file is (currently) unavailable, treat this * file as standalone: * backingStoreRaw is kept to mark broken image chains */ @@ -906,13 +925,13 @@ virStorageFileGetMetadataFromFD(const char *path, int fd, int format) { - return virStorageFileGetMetadataInternal(path, fd, format); + return virStorageFileGetMetadataInternal(path, fd, NULL, format); } /* Recursive workhorse for virStorageFileGetMetadata. */ static virStorageFileMetadataPtr -virStorageFileGetMetadataRecurse(const char *path, int format, - uid_t uid, gid_t gid, +virStorageFileGetMetadataRecurse(const char *path, const char *directory, + int format, uid_t uid, gid_t gid, bool allow_probe, virHashTablePtr cycle) { int fd; @@ -935,7 +954,7 @@ virStorageFileGetMetadataRecurse(const char *path, int format, return NULL; } - ret = virStorageFileGetMetadataInternal(path, fd, format); + ret = virStorageFileGetMetadataInternal(path, fd, directory, format); if (VIR_CLOSE(fd) < 0) VIR_WARN("could not close file %s", path); @@ -947,6 +966,7 @@ virStorageFileGetMetadataRecurse(const char *path, int format, ret->backingStoreFormat = VIR_STORAGE_FILE_AUTO; format = ret->backingStoreFormat; ret->backingMeta = virStorageFileGetMetadataRecurse(ret->backingStore, + ret->directory, format, uid, gid, allow_probe, @@ -994,7 +1014,7 @@ virStorageFileGetMetadata(const char *path, int format, if (format <= VIR_STORAGE_FILE_NONE) format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; - ret = virStorageFileGetMetadataRecurse(path, format, uid, gid, + ret = virStorageFileGetMetadataRecurse(path, NULL, format, uid, gid, allow_probe, cycle); virHashFree(cycle); return ret; @@ -1014,6 +1034,7 @@ virStorageFileFreeMetadata(virStorageFileMetadata *meta) virStorageFileFreeMetadata(meta->backingMeta); VIR_FREE(meta->backingStore); VIR_FREE(meta->backingStoreRaw); + VIR_FREE(meta->directory); VIR_FREE(meta); } @@ -1316,7 +1337,10 @@ virStorageFileChainLookup(virStorageFileMetadataPtr chain, const char *start, STREQ(name, owner->backingStore)) { break; } else if (owner->backingStoreIsFile) { - char *absName = absolutePathFromBaseFile(*parent, name); + char *absName = NULL; + if (virFindBackingFile(owner->directory, true, name, + NULL, &absName) < 0) + goto error; if (absName && STREQ(absName, owner->backingStore)) { VIR_FREE(absName); break; diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 821d07e..d7b4508 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -56,6 +56,7 @@ typedef virStorageFileMetadata *virStorageFileMetadataPtr; struct _virStorageFileMetadata { char *backingStore; /* Canonical name (absolute file, or protocol) */ char *backingStoreRaw; /* If file, original name, possibly relative */ + char *directory; /* The directory containing basename(backingStoreRaw) */ int backingStoreFormat; /* enum virStorageFileFormat */ bool backingStoreIsFile; virStorageFileMetadataPtr backingMeta; -- 1.8.1.2
On 02/15/13 21:38, Eric Blake wrote:
If you have a qcow2 file /path1/to/file pointed to by symlink /path2/symlink, and pass qemu /path2/symlink, then qemu treats a relative backing file in the qcow2 metadata as being relative to /path2, not /path1/to. Yes, this means that it is possible to create a qcow2 file where the choice of WHICH directory and symlink you access its contents from will then determine WHICH backing file (if any) you actually find; the results can be rather screwy, but we have to match what qemu does.
Libvirt and qemu default to creating absolute backing file names, so most users don't hit this. But at least VDSM uses symlinks and relative backing names alongside the --reuse-external flags to libvirt snapshot operations, with the result that libvirt was failing to follow the intended chain of backing files, and then backing files were not granted the necessary sVirt permissions to be opened by qemu.
See https://bugzilla.redhat.com/show_bug.cgi?id=903248 for more gory details. This fixes a regression introduced in commit 8250783.
I tested this patch by creating the following chain:
ls /home/eblake/Downloads/Fedora.iso # raw file for base cd /var/lib/libvirt/images qemu-img create -f qcow2 \ -obacking_file=/home/eblake/Downloads/Fedora.iso,backing_fmt=raw one mkdir sub cd sub ln -s ../one onelink qemu-img create -f qcow2 \ -obacking_file=../sub/onelink,backing_fmt=qcow2 two mv two .. ln -s ../two twolink qemu-img create -f qcow2 \ -obacking_file=../sub/twolink,backing_fmt=qcow2 three mv three .. ln -s ../three threelink
then pointing my domain at /var/lib/libvirt/images/sub/threelink. Prior to this patch, I got complaints about missing backing files; afterwards, I was able to verify that the backing chain (and hence DAC and SELinux relabels) of the entire chain worked.
* src/util/virstoragefile.h (_virStorageFileMetadata): Add directory member. * src/util/virstoragefile.c (absolutePathFromBaseFile): Drop, replaced by... (virFindBackingFile): ...better function. (virStorageFileGetMetadataInternal): Add an argument. (virStorageFileGetMetadataFromFD, virStorageFileChainLookup) (virStorageFileGetMetadata): Update callers. --- src/util/virstoragefile.c | 88 ++++++++++++++++++++++++++++++----------------- src/util/virstoragefile.h | 1 + 2 files changed, 57 insertions(+), 32 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index d741d3d..5bb2b58 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -497,44 +497,56 @@ qedGetBackingStore(char **res, }
/** - * Return an absolute path corresponding to PATH, which is absolute or relative - * to the directory containing BASE_FILE, or NULL on error + * Given a starting point START (either an original file name, or the + * directory containing the original name, depending on START_IS_DIR) + * and a possibly relative backing file NAME, compute the relative + * DIRECTORY (optional) and CANONICAL (mandatory) location of the + * backing file. Return 0 on success, negative on error. */ -static char * -absolutePathFromBaseFile(const char *base_file, const char *path) +static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(5) +virFindBackingFile(const char *start, bool start_is_dir, const char *path, + char **directory, char **canonical) { - char *res = NULL; - char *tmp = NULL; - size_t d_len = dir_len(base_file); + char *combined = NULL; + int ret = -1;
- /* If path is already absolute, or if dirname(base_file) is ".", - just return a copy of path. */ - if (*path == '/' || d_len == 0) { - if (!(res = canonicalize_file_name(path))) - virReportSystemError(errno, - _("Can't canonicalize path '%s'"), path); + if (*path == '/') { + /* Safe to cast away const */ + combined = (char *)path; + } else { + size_t d_len = start_is_dir ? strlen(start) : dir_len(start);
- goto cleanup; + if (d_len > INT_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("name too long: '%s'"), start); + goto cleanup; + } else if (!d_len) {
I'd go for (d_len == 0) here so that it will not be confused with pointers.
+ start = "."; + d_len = 1; + } + if (virAsprintf(&combined, "%.*s/%s", (int)d_len, start, path) < 0) { + virReportOOMError(); + goto cleanup; + } }
Otherwise looks reasonable. ACK. Peter
On 02/15/2013 03:08 PM, Peter Krempa wrote:
On 02/15/13 21:38, Eric Blake wrote:
If you have a qcow2 file /path1/to/file pointed to by symlink /path2/symlink, and pass qemu /path2/symlink, then qemu treats a relative backing file in the qcow2 metadata as being relative to /path2, not /path1/to. Yes, this means that it is possible to create a qcow2 file where the choice of WHICH directory and symlink you access its contents from will then determine WHICH backing file (if any) you actually find; the results can be rather screwy, but we have to match what qemu does.
Libvirt and qemu default to creating absolute backing file names, so most users don't hit this. But at least VDSM uses symlinks and relative backing names alongside the --reuse-external flags to libvirt snapshot operations, with the result that libvirt was failing to follow the intended chain of backing files, and then backing files were not granted the necessary sVirt permissions to be opened by qemu.
See https://bugzilla.redhat.com/show_bug.cgi?id=903248 for more gory details. This fixes a regression introduced in commit 8250783.
I tested this patch by creating the following chain:
and that chain is more-or-less in patch 5/5.
+ } else if (!d_len) {
I'd go for (d_len == 0) here so that it will not be confused with pointers.
Done.
+ start = "."; + d_len = 1; + } + if (virAsprintf(&combined, "%.*s/%s", (int)d_len, start, path) < 0) { + virReportOOMError(); + goto cleanup; + } }
Otherwise looks reasonable.
ACK.
Will push shortly. Thanks for the review. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Testing our backing chain handling will make it much easier to ensure that we avoid issues in the future. If only I had written this test before I first caused several regressions... * tests/virstoragetest.c: New test. * tests/Makefile.am (test_programs): Build it. * .gitignore: Ignore new files. --- .gitignore | 1 + tests/Makefile.am | 6 + tests/virstoragetest.c | 546 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 553 insertions(+) create mode 100644 tests/virstoragetest.c diff --git a/.gitignore b/.gitignore index 8afbf33..23fdc91 100644 --- a/.gitignore +++ b/.gitignore @@ -184,6 +184,7 @@ /tests/virnet*test /tests/virportallocatortest /tests/virshtest +/tests/virstoragetest /tests/virstringtest /tests/virtimetest /tests/viruritest diff --git a/tests/Makefile.am b/tests/Makefile.am index 7d0a88e..cafdae0 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -100,6 +100,7 @@ test_programs = virshtest sockettest \ virstringtest \ virportallocatortest \ sysinfotest \ + virstoragetest \ $(NULL) if WITH_GNUTLS @@ -565,6 +566,11 @@ virstringtest_SOURCES = \ virstringtest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\"" $(AM_CFLAGS) virstringtest_LDADD = $(LDADDS) +virstoragetest_SOURCES = \ + virstoragetest.c testutils.h testutils.c +virstoragetest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\"" $(AM_CFLAGS) +virstoragetest_LDADD = $(LDADDS) + virlockspacetest_SOURCES = \ virlockspacetest.c testutils.h testutils.c virlockspacetest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\"" $(AM_CFLAGS) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c new file mode 100644 index 0000000..6b4ca99 --- /dev/null +++ b/tests/virstoragetest.c @@ -0,0 +1,546 @@ +/* + * Copyright (C) 2013 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Eric Blake <eblake@redhat.com> + */ + +#include <config.h> + +#include <stdlib.h> + +#include "testutils.h" +#include "vircommand.h" +#include "virerror.h" +#include "virlog.h" +#include "virstoragefile.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +#define datadir abs_builddir "/virstoragedata" + +/* This test creates the following files, all in datadir: + + * raw: 1024-byte raw file + * qcow2: qcow2 file with 'raw' as backing + * wrap: qcow2 file with 'qcow2' as backing + * qed: qed file with 'raw' as backing + * sub/link1: symlink to qcow2 + * sub/link2: symlink to wrap + * + * Relative names to these files are known at compile time, but absolute + * and canonical names depend on where the test is run; for convenience, + * we pre-populate the computation of these names for use during the test. +*/ + +static char *qemuimg; +static char *absraw; +static char *canonraw; +static char *absqcow2; +static char *canonqcow2; +static char *abswrap; +static char *absqed; +static char *abslink2; + +static void +testCleanupImages(void) +{ + virCommandPtr cmd; + + VIR_FREE(qemuimg); + VIR_FREE(absraw); + VIR_FREE(canonraw); + VIR_FREE(absqcow2); + VIR_FREE(canonqcow2); + VIR_FREE(abswrap); + VIR_FREE(absqed); + VIR_FREE(abslink2); + + if (chdir(abs_builddir) < 0) { + fprintf(stderr, "unable to return to correct directory, refusing to " + "clean up %s\n", datadir); + return; + } + + cmd = virCommandNewArgList("rm", "-rf", datadir, NULL); + ignore_value(virCommandRun(cmd, NULL)); + virCommandFree(cmd); +} + +static int +testPrepImages(void) +{ + int ret = EXIT_FAILURE; + virCommandPtr cmd = NULL; + + qemuimg = virFindFileInPath("kvm-img"); + if (!qemuimg) + qemuimg = virFindFileInPath("qemu-img"); + if (!qemuimg) { + fprintf(stderr, "qemu-img missing or too old; skipping this test\n"); + return EXIT_AM_SKIP; + } + + if (virAsprintf(&absraw, "%s/raw", datadir) < 0 || + virAsprintf(&absqcow2, "%s/qcow2", datadir) < 0 || + virAsprintf(&abswrap, "%s/wrap", datadir) < 0 || + virAsprintf(&absqed, "%s/qed", datadir) < 0 || + virAsprintf(&abslink2, "%s/sub/link2", datadir) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (virFileMakePath(datadir "/sub") < 0) { + fprintf(stderr, "unable to create directory %s\n", datadir "/sub"); + goto cleanup; + } + + if (chdir(datadir) < 0) { + fprintf(stderr, "unable to test relative backing chains\n"); + goto cleanup; + } + + /* I'm lazy enough to use a shell one-liner instead of open/write/close */ + virCommandFree(cmd); + cmd = virCommandNewArgList("sh", "-c", "printf %1024d 0 > raw", NULL); + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + if (!(canonraw = canonicalize_file_name(absraw))) { + virReportOOMError(); + goto cleanup; + } + + /* Create a qcow2 wrapping relative raw; later on, we modify its + * metadata to test other configurations */ + virCommandFree(cmd); + cmd = virCommandNewArgList("qemu-img", "create", "-f", "qcow2", + "-obacking_file=raw,backing_fmt=raw", "qcow2", + NULL); + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + /* Make sure our later uses of 'qemu-img rebase' will work */ + virCommandFree(cmd); + cmd = virCommandNewArgList("qemu-img", "rebase", "-u", "-f", "qcow2", + "-F", "raw", "-b", "raw", "qcow2", NULL); + if (virCommandRun(cmd, NULL) < 0) { + fprintf(stderr, "qemu-img is too old; skipping this test\n"); + ret = EXIT_AM_SKIP; + goto cleanup; + } + if (!(canonqcow2 = canonicalize_file_name(absqcow2))) { + virReportOOMError(); + goto cleanup; + } + + /* Create a second qcow2 wrapping the first, to be sure that we + * can correctly avoid insecure probing. */ + virCommandFree(cmd); + cmd = virCommandNewArgList("qemu-img", "create", "-f", "qcow2", NULL); + virCommandAddArgFormat(cmd, "-obacking_file=%s,backing_fmt=qcow2", + absqcow2); + virCommandAddArg(cmd, "wrap"); + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + /* Create a qed file. */ + virCommandFree(cmd); + cmd = virCommandNewArgList("qemu-img", "create", "-f", "qed", NULL); + virCommandAddArgFormat(cmd, "-obacking_file=%s,backing_fmt=raw", + absraw); + virCommandAddArg(cmd, "qed"); + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + /* Create some symlinks in a sub-directory. */ + if (symlink("../qcow2", datadir "/sub/link1") < 0 || + symlink("../wrap", datadir "/sub/link2") < 0) { + fprintf(stderr, "unable to create symlink"); + goto cleanup; + } + + ret = 0; +cleanup: + virCommandFree(cmd); + if (ret) + testCleanupImages(); + return ret; +} + +typedef struct _testFileData testFileData; +struct _testFileData +{ + const char *expBackingStore; + const char *expBackingStoreRaw; + const char *expDirectory; + enum virStorageFileFormat expFormat; + bool expIsFile; + unsigned long long expCapacity; + bool expEncrypted; +}; + +enum { + EXP_PASS = 0, + EXP_FAIL = 1, + EXP_WARN = 2, + ALLOW_PROBE = 4, +}; + +struct testChainData +{ + const char *start; + enum virStorageFileFormat format; + const testFileData *files; + int nfiles; + unsigned int flags; +}; + +static int +testStorageChain(const void *args) +{ + const struct testChainData *data = args; + int ret = -1; + virStorageFileMetadataPtr meta; + virStorageFileMetadataPtr elt; + int i = 0; + + meta = virStorageFileGetMetadata(data->start, data->format, -1, -1, + (data->flags & ALLOW_PROBE) != 0); + if (!meta) { + if (data->flags & EXP_FAIL) { + virResetLastError(); + ret = 0; + } + goto cleanup; + } else if (data->flags & EXP_FAIL) { + fprintf(stderr, "call should have failed\n"); + goto cleanup; + } + if (data->flags & EXP_WARN) + virResetLastError(); + + elt = meta; + while (elt) { + char *expect = NULL; + char *actual = NULL; + + if (i == data->nfiles) { + fprintf(stderr, "probed chain was too long\n"); + goto cleanup; + } + + if (virAsprintf(&expect, + "store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld %d", + NULLSTR(data->files[i].expBackingStore), + NULLSTR(data->files[i].expBackingStoreRaw), + NULLSTR(data->files[i].expDirectory), + data->files[i].expFormat, + data->files[i].expIsFile, + data->files[i].expCapacity, + data->files[i].expEncrypted) < 0 || + virAsprintf(&actual, + "store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld %d", + NULLSTR(elt->backingStore), + NULLSTR(elt->backingStoreRaw), + NULLSTR(elt->directory), + elt->backingStoreFormat, elt->backingStoreIsFile, + elt->capacity, elt->encrypted) < 0) { + virReportOOMError(); + VIR_FREE(expect); + VIR_FREE(actual); + goto cleanup; + } + if (STRNEQ(expect, actual)) { + virtTestDifference(stderr, expect, actual); + VIR_FREE(expect); + VIR_FREE(actual); + goto cleanup; + } + VIR_FREE(expect); + VIR_FREE(actual); + elt = elt->backingMeta; + i++; + } + if (i != data->nfiles) { + fprintf(stderr, "probed chain was too short\n"); + goto cleanup; + } + + ret = 0; +cleanup: + virStorageFileFreeMetadata(meta); + return ret; +} + +static int +mymain(void) +{ + int ret; + virCommandPtr cmd = NULL; + + /* Prep some files with qemu-img; if that is not found on PATH, or + * if it lacks support for qcow2 and qed, skip this test. */ + if ((ret = testPrepImages()) != 0) + return ret; + +#define TEST_ONE_CHAIN(id, start, format, chain, flags) \ + do { \ + struct testChainData data = { \ + start, format, chain, ARRAY_CARDINALITY(chain), flags, \ + }; \ + if (virtTestRun("Storage backing chain " id, 1, \ + testStorageChain, &data) < 0) \ + ret = -1; \ + } while (0) + +#define TEST_CHAIN(id, relstart, absstart, format, chain1, flags1, \ + chain2, flags2, chain3, flags3, chain4, flags4) \ + do { \ + TEST_ONE_CHAIN(#id "a", relstart, format, chain1, flags1); \ + TEST_ONE_CHAIN(#id "b", relstart, format, chain2, flags2); \ + TEST_ONE_CHAIN(#id "c", absstart, format, chain3, flags3); \ + TEST_ONE_CHAIN(#id "d", absstart, format, chain4, flags4); \ + } while (0) + + /* Expected details about files in chains */ + const testFileData raw = { + NULL, NULL, NULL, VIR_STORAGE_FILE_NONE, false, 0, false, + }; + const testFileData qcow2_relback_relstart = { + canonraw, "raw", ".", VIR_STORAGE_FILE_RAW, true, 1024, false, + }; + const testFileData qcow2_relback_absstart = { + canonraw, "raw", datadir, VIR_STORAGE_FILE_RAW, true, 1024, false, + }; + const testFileData qcow2_absback = { + canonraw, absraw, datadir, VIR_STORAGE_FILE_RAW, true, 1024, false, + }; + const testFileData qcow2_as_probe = { + canonraw, absraw, datadir, VIR_STORAGE_FILE_AUTO, true, 1024, false, + }; + const testFileData qcow2_bogus = { + NULL, datadir "/bogus", datadir, VIR_STORAGE_FILE_NONE, + false, 1024, false, + }; + const testFileData qcow2_protocol = { + "nbd:example.org:6000", NULL, NULL, VIR_STORAGE_FILE_RAW, + false, 1024, false, + }; + const testFileData wrap = { + canonqcow2, absqcow2, datadir, VIR_STORAGE_FILE_QCOW2, + true, 1024, false, + }; + const testFileData wrap_as_raw = { + canonqcow2, absqcow2, datadir, VIR_STORAGE_FILE_RAW, + true, 1024, false, + }; + const testFileData wrap_as_probe = { + canonqcow2, absqcow2, datadir, VIR_STORAGE_FILE_AUTO, + true, 1024, false, + }; + const testFileData qed = { + canonraw, absraw, datadir, VIR_STORAGE_FILE_RAW, + true, 1024, false, + }; + const testFileData link1_rel = { + canonraw, "../raw", "sub/../sub/..", VIR_STORAGE_FILE_RAW, + true, 1024, false, + }; + const testFileData link1_abs = { + canonraw, "../raw", datadir "/sub/../sub/..", VIR_STORAGE_FILE_RAW, + true, 1024, false, + }; + const testFileData link2_rel = { + canonqcow2, "../sub/link1", "sub/../sub", VIR_STORAGE_FILE_QCOW2, + true, 1024, false, + }; + const testFileData link2_abs = { + canonqcow2, "../sub/link1", datadir "/sub/../sub", + VIR_STORAGE_FILE_QCOW2, true, 1024, false, + }; + + /* The actual tests, in several groups. */ + + /* Missing file */ + const testFileData chain0[] = { }; + TEST_ONE_CHAIN("0", "bogus", VIR_STORAGE_FILE_RAW, chain0, EXP_FAIL); + + /* Raw image, whether with right format or no specified format */ + const testFileData chain1[] = { raw }; + TEST_CHAIN(1, "raw", absraw, VIR_STORAGE_FILE_RAW, + chain1, EXP_PASS, + chain1, ALLOW_PROBE | EXP_PASS, + chain1, EXP_PASS, + chain1, ALLOW_PROBE | EXP_PASS); + TEST_CHAIN(2, "raw", absraw, VIR_STORAGE_FILE_AUTO, + chain1, EXP_PASS, + chain1, ALLOW_PROBE | EXP_PASS, + chain1, EXP_PASS, + chain1, ALLOW_PROBE | EXP_PASS); + + /* Qcow2 file with relative raw backing, format provided */ + const testFileData chain3a[] = { qcow2_relback_relstart, raw }; + const testFileData chain3c[] = { qcow2_relback_absstart, raw }; + const testFileData chain4a[] = { raw }; + TEST_CHAIN(3, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, + chain3a, EXP_PASS, + chain3a, ALLOW_PROBE | EXP_PASS, + chain3c, EXP_PASS, + chain3c, ALLOW_PROBE | EXP_PASS); + TEST_CHAIN(4, "qcow2", absqcow2, VIR_STORAGE_FILE_AUTO, + chain4a, EXP_PASS, + chain3a, ALLOW_PROBE | EXP_PASS, + chain4a, EXP_PASS, + chain3c, ALLOW_PROBE | EXP_PASS); + + /* Rewrite qcow2 file to use absolute backing name */ + virCommandFree(cmd); + cmd = virCommandNewArgList(qemuimg, "rebase", "-u", "-f", "qcow2", + "-F", "raw", "-b", absraw, "qcow2", NULL); + if (virCommandRun(cmd, NULL) < 0) + ret = -1; + + /* Qcow2 file with raw as absolute backing, backing format provided */ + const testFileData chain5[] = { qcow2_absback, raw }; + const testFileData chain6[] = { raw }; + TEST_CHAIN(5, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, + chain5, EXP_PASS, + chain5, ALLOW_PROBE | EXP_PASS, + chain5, EXP_PASS, + chain5, ALLOW_PROBE | EXP_PASS); + TEST_CHAIN(6, "qcow2", absqcow2, VIR_STORAGE_FILE_AUTO, + chain6, EXP_PASS, + chain5, ALLOW_PROBE | EXP_PASS, + chain6, EXP_PASS, + chain5, ALLOW_PROBE | EXP_PASS); + + /* Wrapped file access */ + const testFileData chain7[] = { wrap, qcow2_absback, raw }; + TEST_CHAIN(7, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2, + chain7, EXP_PASS, + chain7, ALLOW_PROBE | EXP_PASS, + chain7, EXP_PASS, + chain7, ALLOW_PROBE | EXP_PASS); + + /* Rewrite qcow2 and wrap file to omit backing file type */ + virCommandFree(cmd); + cmd = virCommandNewArgList(qemuimg, "rebase", "-u", "-f", "qcow2", + "-b", absraw, "qcow2", NULL); + if (virCommandRun(cmd, NULL) < 0) + ret = -1; + + virCommandFree(cmd); + cmd = virCommandNewArgList(qemuimg, "rebase", "-u", "-f", "qcow2", + "-b", absqcow2, "wrap", NULL); + if (virCommandRun(cmd, NULL) < 0) + ret = -1; + + /* Qcow2 file with raw as absolute backing, backing format omitted */ + const testFileData chain8a[] = { wrap_as_raw, raw }; + const testFileData chain8b[] = { wrap_as_probe, qcow2_as_probe, raw }; + TEST_CHAIN(8, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2, + chain8a, EXP_PASS, + chain8b, ALLOW_PROBE | EXP_PASS, + chain8a, EXP_PASS, + chain8b, ALLOW_PROBE | EXP_PASS); + + /* Rewrite qcow2 to a missing backing file, with backing type */ + virCommandFree(cmd); + cmd = virCommandNewArgList(qemuimg, "rebase", "-u", "-f", "qcow2", + "-F", "qcow2", "-b", datadir "/bogus", + "qcow2", NULL); + if (virCommandRun(cmd, NULL) < 0) + ret = -1; + + /* Qcow2 file with missing backing file but specified type */ + const testFileData chain9[] = { qcow2_bogus }; + TEST_CHAIN(9, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, + chain9, EXP_WARN, + chain9, ALLOW_PROBE | EXP_WARN, + chain9, EXP_WARN, + chain9, ALLOW_PROBE | EXP_WARN); + + /* Rewrite qcow2 to a missing backing file, without backing type */ + virCommandFree(cmd); + cmd = virCommandNewArgList(qemuimg, "rebase", "-u", "-f", "qcow2", + "-b", datadir "/bogus", "qcow2", NULL); + if (virCommandRun(cmd, NULL) < 0) + ret = -1; + + /* Qcow2 file with missing backing file and no specified type */ + const testFileData chain10[] = { qcow2_bogus }; + TEST_CHAIN(10, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, + chain10, EXP_WARN, + chain10, ALLOW_PROBE | EXP_WARN, + chain10, EXP_WARN, + chain10, ALLOW_PROBE | EXP_WARN); + + /* Rewrite qcow2 to use an nbd: protocol as backend */ + virCommandFree(cmd); + cmd = virCommandNewArgList(qemuimg, "rebase", "-u", "-f", "qcow2", + "-F", "raw", "-b", "nbd:example.org:6000", + "qcow2", NULL); + if (virCommandRun(cmd, NULL) < 0) + ret = -1; + + /* Qcow2 file with backing protocol instead of file */ + const testFileData chain11[] = { qcow2_protocol }; + TEST_CHAIN(11, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, + chain11, EXP_WARN, + chain11, ALLOW_PROBE | EXP_WARN, + chain11, EXP_WARN, + chain11, ALLOW_PROBE | EXP_WARN); + + /* qed file */ + const testFileData chain12a[] = { raw }; + const testFileData chain12b[] = { qed, raw }; + TEST_CHAIN(12, "qed", absqed, VIR_STORAGE_FILE_AUTO, + chain12a, EXP_PASS, + chain12b, ALLOW_PROBE | EXP_PASS, + chain12a, EXP_PASS, + chain12b, ALLOW_PROBE | EXP_PASS); + + /* Rewrite qcow2 and wrap file to use backing names relative to a + * symlink from a different directory */ + virCommandFree(cmd); + cmd = virCommandNewArgList(qemuimg, "rebase", "-u", "-f", "qcow2", + "-F", "raw", "-b", "../raw", "qcow2", NULL); + if (virCommandRun(cmd, NULL) < 0) + ret = -1; + + virCommandFree(cmd); + cmd = virCommandNewArgList(qemuimg, "rebase", "-u", "-f", "qcow2", + "-F", "qcow2", "-b", "../sub/link1", "wrap", + NULL); + if (virCommandRun(cmd, NULL) < 0) + ret = -1; + + /* Behavior of symlinks to qcow2 with relative backing files */ + const testFileData chain13a[] = { link2_rel, link1_rel, raw }; + const testFileData chain13c[] = { link2_abs, link1_abs, raw }; + TEST_CHAIN(13, "sub/link2", abslink2, VIR_STORAGE_FILE_QCOW2, + chain13a, EXP_PASS, + chain13a, ALLOW_PROBE | EXP_PASS, + chain13c, EXP_PASS, + chain13c, ALLOW_PROBE | EXP_PASS); + + /* Final cleanup */ + testCleanupImages(); + virCommandFree(cmd); + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIRT_TEST_MAIN(mymain) -- 1.8.1.2
On 02/15/13 21:38, Eric Blake wrote:
Testing our backing chain handling will make it much easier to ensure that we avoid issues in the future. If only I had written this test before I first caused several regressions...
* tests/virstoragetest.c: New test. * tests/Makefile.am (test_programs): Build it. * .gitignore: Ignore new files. --- .gitignore | 1 + tests/Makefile.am | 6 + tests/virstoragetest.c | 546 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 553 insertions(+) create mode 100644 tests/virstoragetest.c
I just skimmed through this test as I don't feel well enough to review it thoroughly. It seems OK to me and it compiles and runs okay so I give a weak ACK. Peter
On 02/15/2013 03:26 PM, Peter Krempa wrote:
On 02/15/13 21:38, Eric Blake wrote:
Testing our backing chain handling will make it much easier to ensure that we avoid issues in the future. If only I had written this test before I first caused several regressions...
* tests/virstoragetest.c: New test. * tests/Makefile.am (test_programs): Build it. * .gitignore: Ignore new files. --- .gitignore | 1 + tests/Makefile.am | 6 + tests/virstoragetest.c | 546 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 553 insertions(+) create mode 100644 tests/virstoragetest.c
I just skimmed through this test as I don't feel well enough to review it thoroughly. It seems OK to me and it compiles and runs okay so I give a weak ACK.
I did one more thing to strengthen the test; I'm squashing this before pushing. diff --git i/tests/virstoragetest.c w/tests/virstoragetest.c index 6b4ca99..9da58f3 100644 --- i/tests/virstoragetest.c +++ w/tests/virstoragetest.c @@ -228,8 +228,16 @@ testStorageChain(const void *args) fprintf(stderr, "call should have failed\n"); goto cleanup; } - if (data->flags & EXP_WARN) + if (data->flags & EXP_WARN) { + if (!virGetLastError()) { + fprintf(stderr, "call should have warned\n"); + goto cleanup; + } virResetLastError(); + } else if (virGetLastError()) { + fprintf(stderr, "call should not have warned\n"); + goto cleanup; + } elt = meta; while (elt) { @@ -498,10 +506,10 @@ mymain(void) /* Qcow2 file with backing protocol instead of file */ const testFileData chain11[] = { qcow2_protocol }; TEST_CHAIN(11, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - chain11, EXP_WARN, - chain11, ALLOW_PROBE | EXP_WARN, - chain11, EXP_WARN, - chain11, ALLOW_PROBE | EXP_WARN); + chain11, EXP_PASS, + chain11, ALLOW_PROBE | EXP_PASS, + chain11, EXP_PASS, + chain11, ALLOW_PROBE | EXP_PASS); /* qed file */ const testFileData chain12a[] = { raw }; -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa