[libvirt] [PATCH 0/4] qemu: Support qemuDomainGetBlockInfo on remote storage
And a few refactors. Peter Krempa (4): util: storage: Inline use of virStorageFileGetMetadataFromFDInternal util: storage: Allow specifying format for virStorageFileGetMetadataFromBuf storage: gluster: Optimize header reader functions qemu: refactor qemuDomainGetBlockInfo to work with remote storage src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 84 ++++++++++++++--------- src/storage/storage_backend_gluster.c | 48 ++----------- src/util/virstoragefile.c | 124 +++++++++++++++------------------- src/util/virstoragefile.h | 5 +- 5 files changed, 118 insertions(+), 144 deletions(-) -- 2.0.0
There was just one callsite left. Integrate the body to the only calling function. --- src/util/virstoragefile.c | 91 ++++++++++++++++++++--------------------------- 1 file changed, 39 insertions(+), 52 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 9208b77..01d4a7e 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -973,16 +973,31 @@ virStorageFileGetMetadataFromBuf(const char *path, } -/* Internal version that also supports a containing directory name. */ -static int -virStorageFileGetMetadataFromFDInternal(virStorageSourcePtr meta, - int fd, - int *backingFormat) +/** + * virStorageFileGetMetadataFromFD: + * + * Extract metadata about the storage volume with the specified + * image format. If image format is VIR_STORAGE_FILE_AUTO, it + * will probe to automatically identify the format. Does not recurse. + * + * Callers are advised never to use VIR_STORAGE_FILE_AUTO as a + * format, since a malicious guest can turn a raw file into any + * other non-raw format at will. + * + * Caller MUST free the result after use via virStorageSourceFree. + */ +virStorageSourcePtr +virStorageFileGetMetadataFromFD(const char *path, + int fd, + int format, + int *backingFormat) + { + virStorageSourcePtr ret = NULL; + virStorageSourcePtr meta = NULL; char *buf = NULL; ssize_t len = VIR_STORAGE_MAX_HEADER; struct stat sb; - int ret = -1; int dummy; if (!backingFormat) @@ -992,17 +1007,20 @@ virStorageFileGetMetadataFromFDInternal(virStorageSourcePtr meta, if (fstat(fd, &sb) < 0) { virReportSystemError(errno, - _("cannot stat file '%s'"), - meta->relPath); - return -1; + _("cannot stat file '%s'"), path); + return NULL; } + if (!(meta = virStorageFileMetadataNew(path, format))) + return NULL; + if (S_ISDIR(sb.st_mode)) { /* No header to probe for directories, but also no backing file. Just * update the metadata.*/ meta->type = VIR_STORAGE_TYPE_DIR; meta->format = VIR_STORAGE_FILE_DIR; - ret = 0; + ret = meta; + meta = NULL; goto cleanup; } @@ -1016,51 +1034,20 @@ virStorageFileGetMetadataFromFDInternal(virStorageSourcePtr meta, goto cleanup; } - ret = virStorageFileGetMetadataInternal(meta, buf, len, backingFormat); - - if (ret == 0) { - if (S_ISREG(sb.st_mode)) - meta->type = VIR_STORAGE_TYPE_FILE; - else if (S_ISBLK(sb.st_mode)) - meta->type = VIR_STORAGE_TYPE_BLOCK; - } - cleanup: - VIR_FREE(buf); - return ret; -} - - -/** - * virStorageFileGetMetadataFromFD: - * - * Extract metadata about the storage volume with the specified - * image format. If image format is VIR_STORAGE_FILE_AUTO, it - * will probe to automatically identify the format. Does not recurse. - * - * Callers are advised never to use VIR_STORAGE_FILE_AUTO as a - * format, since a malicious guest can turn a raw file into any - * other non-raw format at will. - * - * Caller MUST free the result after use via virStorageSourceFree. - */ -virStorageSourcePtr -virStorageFileGetMetadataFromFD(const char *path, - int fd, - int format, - int *backingFormat) - -{ - virStorageSourcePtr ret; - - if (!(ret = virStorageFileMetadataNew(path, format))) - return NULL; + if (virStorageFileGetMetadataInternal(meta, buf, len, backingFormat) < 0) + goto cleanup; + if (S_ISREG(sb.st_mode)) + meta->type = VIR_STORAGE_TYPE_FILE; + else if (S_ISBLK(sb.st_mode)) + meta->type = VIR_STORAGE_TYPE_BLOCK; - if (virStorageFileGetMetadataFromFDInternal(ret, fd, backingFormat) < 0) { - virStorageSourceFree(ret); - return NULL; - } + ret = meta; + meta = NULL; + cleanup: + virStorageSourceFree(meta); + VIR_FREE(buf); return ret; } -- 2.0.0
On 07/07/2014 06:16 AM, Peter Krempa wrote:
There was just one callsite left. Integrate the body to the only calling function. --- src/util/virstoragefile.c | 91 ++++++++++++++++++++--------------------------- 1 file changed, 39 insertions(+), 52 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
To allow reusing this function in the qemu driver we need to allow to specify the storage format. Also separate return of the backing store path now isn't necessary. --- src/storage/storage_backend_gluster.c | 5 ++++- src/util/virstoragefile.c | 31 ++++++++++++++----------------- src/util/virstoragefile.h | 2 +- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 1a2b4ec..5ecc098 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -294,10 +294,13 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, goto cleanup; if (!(meta = virStorageFileGetMetadataFromBuf(name, header, len, - &vol->backingStore.path, + VIR_STORAGE_FILE_AUTO, &vol->backingStore.format))) goto cleanup; + vol->backingStore.path = meta->backingStoreRaw; + meta->backingStoreRaw = NULL; + vol->target.format = meta->format; if (vol->backingStore.path && vol->backingStore.format < 0) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 01d4a7e..7ae4642 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -930,13 +930,15 @@ virStorageFileMetadataNew(const char *path, * @path: name of file, for error messages * @buf: header bytes from @path * @len: length of @buf - * @backing: output malloc'd name of backing image, if any + * @format: format of the storage file * @backingFormat: format of @backing * - * Extract metadata about the storage volume, including probing its - * format. Does not recurse. Callers are advised not to trust the - * learned format if a guest has ever used the volume when it was - * raw, since a malicious guest can turn a raw file into any + * Extract metadata about the storage volume with the specified + * image format. If image format is VIR_STORAGE_FILE_AUTO, it + * will probe to automatically identify the format. Does not recurse. + * + * Callers are advised never to use VIR_STORAGE_FILE_AUTO as a + * format, since a malicious guest can turn a raw file into any * other non-raw format at will. * * If the returned @backingFormat is VIR_STORAGE_FILE_AUTO @@ -950,25 +952,20 @@ virStorageSourcePtr virStorageFileGetMetadataFromBuf(const char *path, char *buf, size_t len, - char **backing, + int format, int *backingFormat) { virStorageSourcePtr ret = NULL; - virStorageSourcePtr meta = NULL; - if (!(meta = virStorageFileMetadataNew(path, VIR_STORAGE_FILE_AUTO))) + if (!(ret = virStorageFileMetadataNew(path, format))) return NULL; - if (virStorageFileGetMetadataInternal(meta, buf, len, - backingFormat) < 0) - goto cleanup; - if (VIR_STRDUP(*backing, meta->backingStoreRaw) < 0) - goto cleanup; + if (virStorageFileGetMetadataInternal(ret, buf, len, + backingFormat) < 0) { + virStorageSourceFree(ret); + return NULL; + } - ret = meta; - meta = NULL; - cleanup: - virStorageSourceFree(meta); return ret; } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 4f7357b..89ecc1e 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -290,7 +290,7 @@ virStorageSourcePtr virStorageFileGetMetadataFromFD(const char *path, virStorageSourcePtr virStorageFileGetMetadataFromBuf(const char *path, char *buf, size_t len, - char **backing, + int format, int *backingFormat) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5); -- 2.0.0
On 07/07/2014 06:16 AM, Peter Krempa wrote:
To allow reusing this function in the qemu driver we need to allow to specify the storage format. Also separate return of the backing store
s/to specify/specifying/
path now isn't necessary. --- src/storage/storage_backend_gluster.c | 5 ++++- src/util/virstoragefile.c | 31 ++++++++++++++----------------- src/util/virstoragefile.h | 2 +- 3 files changed, 19 insertions(+), 19 deletions(-)
+++ b/src/storage/storage_backend_gluster.c @@ -294,10 +294,13 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, goto cleanup;
if (!(meta = virStorageFileGetMetadataFromBuf(name, header, len, - &vol->backingStore.path, + VIR_STORAGE_FILE_AUTO, &vol->backingStore.format)))
+++ b/src/util/virstoragefile.c
+ * + * Callers are advised never to use VIR_STORAGE_FILE_AUTO as a + * format, since a malicious guest can turn a raw file into any * other non-raw format at will.
At this point, we may want to re-phrase this, as this very patch introduced a caller that passes AUTO. Maybe: Callers are advised never to use VIR_STORAGE_FILE_AUTO as a format on a file that might be raw if that file will then be passed to a guest, since a malicious guest ... ACK. I'll leave it up to you whether to reword the comment, or if you can come up with anything better. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07/07/14 22:57, Eric Blake wrote:
On 07/07/2014 06:16 AM, Peter Krempa wrote:
To allow reusing this function in the qemu driver we need to allow to specify the storage format. Also separate return of the backing store
s/to specify/specifying/
path now isn't necessary. --- src/storage/storage_backend_gluster.c | 5 ++++- src/util/virstoragefile.c | 31 ++++++++++++++----------------- src/util/virstoragefile.h | 2 +- 3 files changed, 19 insertions(+), 19 deletions(-)
+++ b/src/storage/storage_backend_gluster.c @@ -294,10 +294,13 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, goto cleanup;
if (!(meta = virStorageFileGetMetadataFromBuf(name, header, len, - &vol->backingStore.path, + VIR_STORAGE_FILE_AUTO, &vol->backingStore.format)))
+++ b/src/util/virstoragefile.c
+ * + * Callers are advised never to use VIR_STORAGE_FILE_AUTO as a + * format, since a malicious guest can turn a raw file into any * other non-raw format at will.
At this point, we may want to re-phrase this, as this very patch introduced a caller that passes AUTO. Maybe:
Callers are advised never to use VIR_STORAGE_FILE_AUTO as a format on a file that might be raw if that file will then be passed to a guest, since a malicious guest ...
ACK. I'll leave it up to you whether to reword the comment, or if you can come up with anything better.
I went with your wording as it's better than we have now and pushed the series. Thanks. Peter
The gluster code had two functions for reading volume headers, remove one and reuse the second one. --- src/storage/storage_backend_gluster.c | 43 ++--------------------------------- 1 file changed, 2 insertions(+), 41 deletions(-) diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 5ecc098..76d2461 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -651,9 +651,6 @@ virStorageFileBackendGlusterReadHeader(virStorageSourcePtr src, { virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; glfs_fd_t *fd = NULL; - size_t alloc = 0; - size_t size = 0; - int save_errno; ssize_t ret = -1; *buf = NULL; @@ -661,47 +658,11 @@ virStorageFileBackendGlusterReadHeader(virStorageSourcePtr src, if (!(fd = glfs_open(priv->vol, src->path, O_RDONLY))) { virReportSystemError(errno, _("Failed to open file '%s'"), src->path); - goto cleanup; - } - - /* code below is shamelessly stolen from saferead_lim */ - for (;;) { - int count; - int requested; - - if (size + BUFSIZ + 1 > alloc) { - alloc += alloc / 2; - if (alloc < size + BUFSIZ + 1) - alloc = size + BUFSIZ + 1; - - if (VIR_REALLOC_N(*buf, alloc) < 0) { - save_errno = errno; - break; - } - } - - /* Ensure that (size + requested <= max_len); */ - requested = MIN(size < max_len ? max_len - size : 0, - alloc - size - 1); - count = glfs_read(fd, *buf + size, requested, 0); - size += count; - - if (count != requested || requested == 0) { - save_errno = errno; - if (count < 0) { - virReportSystemError(errno, - _("cannot read header '%s'"), src->path); - break; - } - ret = size; - goto cleanup; - } + return -1; } - VIR_FREE(*buf); - errno = save_errno; + ret = virStorageBackendGlusterReadHeader(fd, src->path, max_len, buf); - cleanup: if (fd) glfs_close(fd); -- 2.0.0
On 07/07/2014 06:16 AM, Peter Krempa wrote:
The gluster code had two functions for reading volume headers, remove one and reuse the second one. --- src/storage/storage_backend_gluster.c | 43 ++--------------------------------- 1 file changed, 2 insertions(+), 41 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The qemu block info function relied on working with local storage. Break this assumption by adding support for remote volumes. Unfortunately we still need to take a hybrid approach as some of the operations require a filedescriptor. Previously you'd get: $ virsh domblkinfo gl vda error: cannot stat file '/img10': Bad file descriptor Now you get some stats: $ virsh domblkinfo gl vda Capacity: 10485760 Allocation: 197120 Physical: 197120 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1110198 --- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 84 ++++++++++++++++++++++++++++++----------------- src/util/virstoragefile.c | 2 +- src/util/virstoragefile.h | 3 ++ 4 files changed, 58 insertions(+), 32 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 18d5f28..6d7bf41 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1910,6 +1910,7 @@ virStorageFileGetSCSIKey; virStorageFileIsClusterFS; virStorageFileParseChainIndex; virStorageFileProbeFormat; +virStorageFileProbeFormatFromBuf; virStorageFileResize; virStorageIsFile; virStorageNetHostDefClear; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fe76d55..23f3f08 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10334,11 +10334,13 @@ qemuDomainGetBlockInfo(virDomainPtr dom, int activeFail = false; virQEMUDriverConfigPtr cfg = NULL; char *alias = NULL; + char *buf = NULL; + ssize_t len; virCheckFlags(0, -1); if (!(vm = qemuDomObjFromDomain(dom))) - goto cleanup; + return -1; cfg = virQEMUDriverGetConfig(driver); @@ -10346,8 +10348,7 @@ qemuDomainGetBlockInfo(virDomainPtr dom, goto cleanup; if (!path || path[0] == '\0') { - virReportError(VIR_ERR_INVALID_ARG, - "%s", _("NULL or empty path")); + virReportError(VIR_ERR_INVALID_ARG, "%s", _("NULL or empty path")); goto cleanup; } @@ -10357,48 +10358,68 @@ qemuDomainGetBlockInfo(virDomainPtr dom, _("invalid path %s not assigned to domain"), path); goto cleanup; } + disk = vm->def->disks[idx]; - path = virDomainDiskGetSource(disk); - if (!path) { - virReportError(VIR_ERR_INVALID_ARG, - _("disk %s does not currently have a source assigned"), - path); - goto cleanup; - } - /* The path is correct, now try to open it and get its size. */ - fd = qemuOpenFile(driver, vm, path, O_RDONLY, NULL, NULL); - if (fd == -1) - goto cleanup; + if (virStorageSourceIsLocalStorage(disk->src)) { + if (!disk->src->path) { + virReportError(VIR_ERR_INVALID_ARG, + _("disk '%s' does not currently have a source assigned"), + path); + goto cleanup; + } + + if ((fd = qemuOpenFile(driver, vm, path, O_RDONLY, NULL, NULL)) == -1) + goto cleanup; + + if (fstat(fd, &sb) < 0) { + virReportSystemError(errno, + _("cannot stat file '%s'"), disk->src->path); + goto cleanup; + } + + if ((len = virFileReadHeaderFD(fd, VIR_STORAGE_MAX_HEADER, &buf)) < 0) { + virReportSystemError(errno, _("cannot read header '%s'"), + disk->src->path); + goto cleanup; + } + } else { + if (virStorageFileInitAs(disk->src, cfg->user, cfg->group) < 0) + goto cleanup; + + if ((len = virStorageFileReadHeader(disk->src, VIR_STORAGE_MAX_HEADER, + &buf)) < 0) + goto cleanup; + + if (virStorageFileStat(disk->src, &sb) < 0) { + virReportSystemError(errno, _("failed to stat remote file '%s'"), + NULLSTR(disk->src->path)); + goto cleanup; + } + } /* Probe for magic formats */ if (virDomainDiskGetFormat(disk)) { format = virDomainDiskGetFormat(disk); } else { - if (cfg->allowDiskFormatProbing) { - if ((format = virStorageFileProbeFormat(path, - cfg->user, - cfg->group)) < 0) - goto cleanup; - } else { + if (!cfg->allowDiskFormatProbing) { virReportError(VIR_ERR_INTERNAL_ERROR, _("no disk format for %s and probing is disabled"), path); goto cleanup; } + + if ((format = virStorageFileProbeFormatFromBuf(disk->src->path, + buf, len)) < 0) + goto cleanup; } - if (!(meta = virStorageFileGetMetadataFromFD(path, fd, format, NULL))) + if (!(meta = virStorageFileGetMetadataFromBuf(disk->src->path, buf, len, + format, NULL))) goto cleanup; /* Get info for normal formats */ - if (fstat(fd, &sb) < 0) { - virReportSystemError(errno, - _("cannot stat file '%s'"), path); - goto cleanup; - } - - if (S_ISREG(sb.st_mode)) { + if (S_ISREG(sb.st_mode) || fd == -1) { #ifndef WIN32 info->physical = (unsigned long long)sb.st_blocks * (unsigned long long)DEV_BSIZE; @@ -10434,7 +10455,7 @@ qemuDomainGetBlockInfo(virDomainPtr dom, /* ..but if guest is not using raw disk format and on a block device, * then query highest allocated extent from QEMU */ - if (virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_BLOCK && + if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_BLOCK && format != VIR_STORAGE_FILE_RAW && S_ISBLK(sb.st_mode)) { qemuDomainObjPrivatePtr priv = vm->privateData; @@ -10475,6 +10496,8 @@ qemuDomainGetBlockInfo(virDomainPtr dom, VIR_FREE(alias); virStorageSourceFree(meta); VIR_FORCE_CLOSE(fd); + if (disk) + virStorageFileDeinit(disk->src); /* If we failed to get data from a domain because it's inactive and * it's not a persistent domain, then force failure. @@ -10484,8 +10507,7 @@ qemuDomainGetBlockInfo(virDomainPtr dom, _("domain is not running")); ret = -1; } - if (vm) - virObjectUnlock(vm); + virObjectUnlock(vm); virObjectUnref(cfg); return ret; } diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 7ae4642..6789463 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -682,7 +682,7 @@ virStorageIsRelative(const char *backing) } -static int +int virStorageFileProbeFormatFromBuf(const char *path, char *buf, size_t buflen) diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 89ecc1e..18d3a75 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -276,6 +276,9 @@ struct _virStorageSource { # endif int virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid); +int virStorageFileProbeFormatFromBuf(const char *path, + char *buf, + size_t buflen); int virStorageFileGetMetadataInternal(virStorageSourcePtr meta, char *buf, -- 2.0.0
On 07/07/2014 06:16 AM, Peter Krempa wrote:
The qemu block info function relied on working with local storage. Break this assumption by adding support for remote volumes. Unfortunately we still need to take a hybrid approach as some of the operations require a filedescriptor.
Previously you'd get: $ virsh domblkinfo gl vda error: cannot stat file '/img10': Bad file descriptor
Now you get some stats: $ virsh domblkinfo gl vda Capacity: 10485760 Allocation: 197120 Physical: 197120
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1110198 --- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 84 ++++++++++++++++++++++++++++++----------------- src/util/virstoragefile.c | 2 +- src/util/virstoragefile.h | 3 ++ 4 files changed, 58 insertions(+), 32 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa