Split out the opening of the file and fetch of the stat buffer into a helper qemuDomainStorageOpenStat. This will handle either opening the local or remote storage. Additionally split out the cleanup of that into a separate helper qemuDomainStorageCloseStat which will either close the file or call the virStorageFileDeinit function. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 93 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 72 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 653fe5d..6a312d2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11537,6 +11537,74 @@ qemuDomainMemoryPeek(virDomainPtr dom, * @cfg: driver configuration data * @vm: domain object * @src: storage source data + * @ret_fd: pointer to return open'd file descriptor + * @ret_sb: pointer to return stat buffer (local or remote) + * + * For local storage, open the file using qemuOpenFile and then use + * fstat() to grab the stat struct data for the caller. + * + * For remote storage, attempt to access the file and grab the stat + * struct data if the remote connection supports it. + * + * Returns 0 on success with @ret_fd and @ret_sb populated, -1 on failure + */ +static int +qemuDomainStorageOpenStat(virQEMUDriverPtr driver, + virQEMUDriverConfigPtr cfg, + virDomainObjPtr vm, + virStorageSourcePtr src, + int *ret_fd, + struct stat *ret_sb) +{ + if (virStorageSourceIsLocalStorage(src)) { + if ((*ret_fd = qemuOpenFile(driver, vm, src->path, O_RDONLY, + NULL, NULL)) == -1) + return -1; + + if (fstat(*ret_fd, ret_sb) < 0) { + virReportSystemError(errno, _("cannot stat file '%s'"), src->path); + VIR_FORCE_CLOSE(*ret_fd); + return -1; + } + } else { + if (virStorageFileInitAs(src, cfg->user, cfg->group) < 0) + return -1; + + if (virStorageFileStat(src, ret_sb) < 0) { + virStorageFileDeinit(src); + virReportSystemError(errno, _("failed to stat remote file '%s'"), + NULLSTR(src->path)); + return -1; + } + } + + return 0; +} + + +/** + * @src: storage source data + * @fd: file descriptor to close for local + * + * If local, then just close the file descriptor. + * else remote, then tear down the storage driver backend connection. + */ +static void +qemuDomainStorageCloseStat(virStorageSourcePtr src, + int *fd) +{ + if (virStorageSourceIsLocalStorage(src)) + VIR_FORCE_CLOSE(*fd); + else + virStorageFileDeinit(src); +} + + +/** + * @driver: qemu driver data + * @cfg: driver configuration data + * @vm: domain object + * @src: storage source data * * Refresh the capacity and allocation limits of a given storage source. * @@ -11575,35 +11643,19 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver, char *buf = NULL; ssize_t len; - if (virStorageSourceIsLocalStorage(src)) { - if ((fd = qemuOpenFile(driver, vm, src->path, O_RDONLY, - NULL, NULL)) == -1) - goto cleanup; - - if (fstat(fd, &sb) < 0) { - virReportSystemError(errno, - _("cannot stat file '%s'"), src->path); - goto cleanup; - } + if (qemuDomainStorageOpenStat(driver, cfg, vm, src, &fd, &sb) < 0) + goto cleanup; + if (virStorageSourceIsLocalStorage(src)) { if ((len = virFileReadHeaderFD(fd, VIR_STORAGE_MAX_HEADER, &buf)) < 0) { virReportSystemError(errno, _("cannot read header '%s'"), src->path); goto cleanup; } } else { - if (virStorageFileInitAs(src, cfg->user, cfg->group) < 0) - goto cleanup; - if ((len = virStorageFileReadHeader(src, VIR_STORAGE_MAX_HEADER, &buf)) < 0) goto cleanup; - - if (virStorageFileStat(src, &sb) < 0) { - virReportSystemError(errno, _("failed to stat remote file '%s'"), - NULLSTR(src->path)); - goto cleanup; - } } /* Get info for normal formats */ @@ -11671,8 +11723,7 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver, cleanup: VIR_FREE(buf); virStorageSourceFree(meta); - VIR_FORCE_CLOSE(fd); - virStorageFileDeinit(src); + qemuDomainStorageCloseStat(src, &fd); return ret; } -- 2.7.4

Commit id '8dc27259' introduced virStorageSourceUpdateBlockPhysicalSize in order to retrieve the physical size for a block backed source device since commit id '15fa84ac' changed to use the qemuMonitorGetAllBlockStatsInfo and qemuMonitorBlockStatsUpdateCapacity API's to (essentially) retrieve the "actual-size" from a 'query-block' operation for the source device. However, the code only was made functional for a BLOCK backing type and it neglected to use qemuOpenFile, instead using just open. After the open the block lseek would find the end of the block and set the physical value, close the fd and return. Since the code would return 0 immediately if the source device wasn't a BLOCK backed device, the physical would be displayed incorrectly, such as follows in domblkinfo for a file backed source device: Capacity: 1073741824 Allocation: 0 Physical: 0 This patch will modify the algorithm to get the physical size for other backing types and it will make use of the qemuDomainStorageOpenStat helper in order to open/stat the source file depending on its type. The qemuDomainGetStatsOneBlock will no longer inhibit printing errors, but it will still ignore them leaving the physical value set to 0. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/libvirt_private.syms | 2 +- src/qemu/qemu_driver.c | 25 +++++++++++++++---- src/util/virstoragefile.c | 61 ++++++++++++++++++++++++++++------------------- src/util/virstoragefile.h | 6 +++-- 4 files changed, 62 insertions(+), 32 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 43220e0..eaaa088 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2442,7 +2442,7 @@ virStorageSourceParseRBDColonString; virStorageSourcePoolDefFree; virStorageSourcePoolModeTypeFromString; virStorageSourcePoolModeTypeToString; -virStorageSourceUpdateBlockPhysicalSize; +virStorageSourceUpdatePhysicalSize; virStorageTypeFromString; virStorageTypeToString; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a744fda..6563a1e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11601,10 +11601,23 @@ qemuDomainStorageCloseStat(virStorageSourcePtr src, static int -qemuDomainStorageUpdatePhysical(virStorageSourcePtr src, - bool report) +qemuDomainStorageUpdatePhysical(virQEMUDriverPtr driver, + virQEMUDriverConfigPtr cfg, + virDomainObjPtr vm, + virStorageSourcePtr src) { - return virStorageSourceUpdateBlockPhysicalSize(src, report); + int ret; + int fd = -1; + struct stat sb; + + if (qemuDomainStorageOpenStat(driver, cfg, vm, src, &fd, &sb) < 0) + return -1; + + ret = virStorageSourceUpdatePhysicalSize(src, fd, &sb); + + qemuDomainStorageCloseStat(src, &fd); + + return ret; } @@ -11831,7 +11844,7 @@ qemuDomainGetBlockInfo(virDomainPtr dom, if (entry->physical) { info->physical = entry->physical; } else { - if (qemuDomainStorageUpdatePhysical(disk->src, true) < 0) + if (qemuDomainStorageUpdatePhysical(driver, cfg, vm, disk->src) < 0) goto endjob; info->physical = disk->src->physical; @@ -19386,9 +19399,11 @@ qemuDomainGetStatsOneBlock(virQEMUDriverPtr driver, QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx, "physical", entry->physical); } else { - if (qemuDomainStorageUpdatePhysical(src, false) == 0) { + if (qemuDomainStorageUpdatePhysical(driver, cfg, dom, src) == 0) { QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx, "physical", src->physical); + } else { + virResetLastError(); } } diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 1011bd0..809d825 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -24,7 +24,6 @@ #include <config.h> #include "virstoragefile.h" -#include <sys/stat.h> #include <unistd.h> #include <fcntl.h> #include <stdlib.h> @@ -3175,41 +3174,55 @@ virStorageSourceNewFromBacking(virStorageSourcePtr parent) /** - * @src: disk source definiton structure - * @report: report libvirt errors if set to true + * @src: disk source definition structure + * @fd: file descriptor + * @sb: stat buffer * - * Updates src->physical for block devices since qemu doesn't report the current - * size correctly for them. Returns 0 on success, -1 on error. + * Updates src->physical depending on the actual type of storage being used. + * + * Returns 0 on success, -1 on error. */ int -virStorageSourceUpdateBlockPhysicalSize(virStorageSourcePtr src, - bool report) +virStorageSourceUpdatePhysicalSize(virStorageSourcePtr src, + int fd, + struct stat const *sb) { - int fd = -1; off_t end; - int ret = -1; + virStorageType actual_type = virStorageSourceGetActualType(src); - if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_BLOCK) - return 0; + switch (actual_type) { + case VIR_STORAGE_TYPE_FILE: + case VIR_STORAGE_TYPE_NETWORK: + src->physical = sb->st_size; + break; - if ((fd = open(src->path, O_RDONLY)) < 0) { - if (report) - virReportSystemError(errno, _("failed to open block device '%s'"), + case VIR_STORAGE_TYPE_BLOCK: + if ((end = lseek(fd, 0, SEEK_END)) == (off_t) -1) { + virReportSystemError(errno, _("failed to seek to end of '%s'"), src->path); - return -1; - } + return -1; + } - if ((end = lseek(fd, 0, SEEK_END)) == (off_t) -1) { - if (report) - virReportSystemError(errno, - _("failed to seek to end of '%s'"), src->path); - } else { src->physical = end; - ret = 0; + break; + + case VIR_STORAGE_TYPE_DIR: + src->physical = 0; + break; + + /* We shouldn't get VOLUME, but the switch requires all cases */ + case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NONE: + case VIR_STORAGE_TYPE_LAST: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot retrieve physical for path '%s' type '%s'"), + NULLSTR(src->path), + virStorageTypeToString(actual_type)); + return -1; + break; } - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 3d09468..e38d01f 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -24,6 +24,8 @@ #ifndef __VIR_STORAGE_FILE_H__ # define __VIR_STORAGE_FILE_H__ +# include <sys/stat.h> + # include "virbitmap.h" # include "virseclabel.h" # include "virstorageencryption.h" @@ -355,8 +357,8 @@ bool virStorageSourceIsEmpty(virStorageSourcePtr src); bool virStorageSourceIsBlockLocal(const virStorageSource *src); void virStorageSourceFree(virStorageSourcePtr def); void virStorageSourceBackingStoreClear(virStorageSourcePtr def); -int virStorageSourceUpdateBlockPhysicalSize(virStorageSourcePtr src, - bool report); +int virStorageSourceUpdatePhysicalSize(virStorageSourcePtr src, + int fd, struct stat const *sb); virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent); virStorageSourcePtr virStorageSourceCopy(const virStorageSource *src, bool backingChain) -- 2.7.4

Instead of having duplicated code in qemuStorageLimitsRefresh and virStorageBackendUpdateVolTargetInfo to get capacity specific data about the storage backing source or volume -- create a common API to handle the details for both. As a side effect, virStorageFileProbeFormatFromBuf returns to being a local/static helper to virstoragefile.c For the QEMU code - if the probe is done, then the format is saved so as to avoid future such probes. For the storage backend code, there is no need to deal with the probe since we cannot call the new API if target->format == NONE. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/libvirt_private.syms | 2 +- src/qemu/qemu_driver.c | 29 +++------------------- src/storage/storage_backend.c | 8 +----- src/util/virstoragefile.c | 57 ++++++++++++++++++++++++++++++++++++++++++- src/util/virstoragefile.h | 6 ++--- 5 files changed, 65 insertions(+), 37 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7a78905..4b4a197 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2417,7 +2417,6 @@ virStorageFileGetSCSIKey; virStorageFileIsClusterFS; virStorageFileParseChainIndex; virStorageFileProbeFormat; -virStorageFileProbeFormatFromBuf; virStorageFileResize; virStorageIsFile; virStorageNetHostDefClear; @@ -2443,6 +2442,7 @@ virStorageSourcePoolDefFree; virStorageSourcePoolModeTypeFromString; virStorageSourcePoolModeTypeToString; virStorageSourceUpdateBackingSizes; +virStorageSourceUpdateCapacity; virStorageSourceUpdatePhysicalSize; virStorageTypeFromString; virStorageTypeToString; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3d20e30..f508872 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11657,9 +11657,7 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver, { int ret = -1; int fd = -1; - virStorageSourcePtr meta = NULL; struct stat sb; - int format; char *buf = NULL; ssize_t len; @@ -11681,27 +11679,8 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver, if (virStorageSourceUpdateBackingSizes(src, fd, &sb) < 0) goto cleanup; - /* Raw files: capacity is physical size. For all other files: if - * the metadata has a capacity, use that, otherwise fall back to - * physical size. */ - if (!(format = src->format)) { - if (!cfg->allowDiskFormatProbing) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("no disk format for %s and probing is disabled"), - src->path); - goto cleanup; - } - - if ((format = virStorageFileProbeFormatFromBuf(src->path, - buf, len)) < 0) - goto cleanup; - } - if (format == VIR_STORAGE_FILE_RAW) - src->capacity = src->physical; - else if ((meta = virStorageFileGetMetadataFromBuf(src->path, buf, - len, format, NULL))) - src->capacity = meta->capacity ? meta->capacity : src->physical; - else + if (virStorageSourceUpdateCapacity(src, buf, len, + cfg->allowDiskFormatProbing) < 0) goto cleanup; /* If guest is not using raw disk format and is on a host block @@ -11709,14 +11688,14 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver, * query the highest allocated extent from QEMU */ if (virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_BLOCK && - format != VIR_STORAGE_FILE_RAW && + src->format != VIR_STORAGE_FILE_RAW && S_ISBLK(sb.st_mode)) src->allocation = 0; ret = 0; + cleanup: VIR_FREE(buf); - virStorageSourceFree(meta); qemuDomainStorageCloseStat(src, &fd); return ret; } diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 0810ced..a433190 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1884,7 +1884,6 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, { int ret, fd = -1; struct stat sb; - virStorageSourcePtr meta = NULL; char *buf = NULL; ssize_t len = VIR_STORAGE_MAX_HEADER; @@ -1929,14 +1928,10 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, goto cleanup; } - if (!(meta = virStorageFileGetMetadataFromBuf(target->path, buf, len, target->format, - NULL))) { + if (virStorageSourceUpdateCapacity(target, buf, len, false) < 0) { ret = -1; goto cleanup; } - - if (meta->capacity) - target->capacity = meta->capacity; } if (withBlockVolFormat) { @@ -1946,7 +1941,6 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, } cleanup: - virStorageSourceFree(meta); VIR_FORCE_CLOSE(fd); VIR_FREE(buf); return ret; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index ab0f9bd..cf62ca9 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -797,7 +797,7 @@ virStorageIsRelative(const char *backing) } -int +static int virStorageFileProbeFormatFromBuf(const char *path, char *buf, size_t buflen) @@ -3289,6 +3289,61 @@ virStorageSourceUpdateBackingSizes(virStorageSourcePtr src, } +/** + * @src: disk source definition structure + * @buf: buffer to the storage file header + * @len: length of the storage file header + * @probe: allow probe + * + * Update the storage @src capacity. This may involve probing the storage + * @src in order to "see" if we can recognize what exists. + * + * Returns 0 on success, -1 on error. + */ +int +virStorageSourceUpdateCapacity(virStorageSourcePtr src, + char *buf, + ssize_t len, + bool probe) +{ + int ret = -1; + virStorageSourcePtr meta = NULL; + int format = src->format; + + /* Raw files: capacity is physical size. For all other files: if + * the metadata has a capacity, use that, otherwise fall back to + * physical size. */ + if (format == VIR_STORAGE_FILE_NONE) { + if (!probe) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("no disk format for %s and probing is disabled"), + src->path); + goto cleanup; + } + + if ((format = virStorageFileProbeFormatFromBuf(src->path, + buf, len)) < 0) + goto cleanup; + + src->format = format; + } + + if (format == VIR_STORAGE_FILE_RAW) + src->capacity = src->physical; + else if ((meta = virStorageFileGetMetadataFromBuf(src->path, buf, + len, format, NULL))) + src->capacity = meta->capacity ? meta->capacity : src->physical; + else + goto cleanup; + + ret = 0; + + cleanup: + virStorageSourceFree(meta); + return ret; +} + + static char * virStorageFileCanonicalizeFormatPath(char **components, size_t ncomponents, diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index b91045b..6d1aac7 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -284,9 +284,6 @@ 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, @@ -361,6 +358,9 @@ int virStorageSourceUpdatePhysicalSize(virStorageSourcePtr src, int fd, struct stat const *sb); int virStorageSourceUpdateBackingSizes(virStorageSourcePtr src, int fd, struct stat const *sb); +int virStorageSourceUpdateCapacity(virStorageSourcePtr src, + char *buf, ssize_t len, + bool probe); virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent); virStorageSourcePtr virStorageSourceCopy(const virStorageSource *src, -- 2.7.4