Re: [libvirt] [PATCHv2 09/16] storage: don't require caller to pre-allocate metadata struct

Requiring pre-allocation was an unusual idiom. It allowed iteration over the backing chain to use fewer mallocs, but made one-shot clients harder to read. Also, this makes it easier for a future patch to move away from opening fds on every iteration over the chain. * src/util/storage_file.h (virStorageFileGetMetadataFromFD): Alter signature. * src/util/storage_file.c (virStorageFileGetMetadataFromFD): Allocate return value. (virStorageFileGetMetadata): Update clients. * src/conf/domain_conf.c (virDomainDiskDefForeachPath): Likewise. * src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Likewise. * src/storage/storage_backend_fs.c (virStorageBackendProbeTarget): Likewise. --- src/conf/domain_conf.c | 11 ++++------ src/qemu/qemu_driver.c | 9 +------- src/storage/storage_backend_fs.c | 12 +++------- src/util/storage_file.c | 47 +++++++++++++++++++--------------------- src/util/storage_file.h | 7 +++--- 5 files changed, 33 insertions(+), 53 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index acf1904..a1d5a1a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14644,16 +14644,11 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, int ret = -1; size_t depth = 0; char *nextpath = NULL; - virStorageFileMetadata *meta; + virStorageFileMetadata *meta = NULL; if (!disk->src || disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) return 0; - if (VIR_ALLOC(meta) < 0) { - virReportOOMError(); - return ret; - } - if (disk->format > 0) { format = disk->format; } else { @@ -14696,7 +14691,7 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, } } - if (virStorageFileGetMetadataFromFD(path, fd, format, meta) < 0) { + if (!(meta = virStorageFileGetMetadataFromFD(path, fd, format))) { VIR_FORCE_CLOSE(fd); goto cleanup; } @@ -14730,6 +14725,8 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, /* Allow probing for image formats that are safe */ if (format == VIR_STORAGE_FILE_AUTO_SAFE) format = VIR_STORAGE_FILE_AUTO; + virStorageFileFreeMetadata(meta); + meta = NULL; } while (nextpath); ret = 0; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4d1c63c..0e7665e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9202,14 +9202,7 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, } } - if (VIR_ALLOC(meta) < 0) { - virReportOOMError(); - goto cleanup; - } - - if (virStorageFileGetMetadataFromFD(path, fd, - format, - meta) < 0) + if (!(meta = virStorageFileGetMetadataFromFD(path, fd, format))) goto cleanup; /* Get info for normal formats */ diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index db19b87..cecc2d2 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -68,12 +68,7 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target, { int fd = -1; int ret = -1; - virStorageFileMetadata *meta; - - if (VIR_ALLOC(meta) < 0) { - virReportOOMError(); - return ret; - } + virStorageFileMetadata *meta = NULL; *backingStore = NULL; *backingStoreFormat = VIR_STORAGE_FILE_AUTO; @@ -96,9 +91,8 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target, goto error; } - if (virStorageFileGetMetadataFromFD(target->path, fd, - target->format, - meta) < 0) { + if (!(meta = virStorageFileGetMetadataFromFD(target->path, fd, + target->format))) { ret = -1; goto error; } diff --git a/src/util/storage_file.c b/src/util/storage_file.c index 0f879bd..5387f1e 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -852,41 +852,43 @@ virStorageFileProbeFormat(const char *path) * backing store. Callers are advised against probing for the * backing store format in this case. * - * Caller MUST free @meta after use via virStorageFileFreeMetadata. + * Caller MUST free the result after use via virStorageFileFreeMetadata. */ -int +virStorageFileMetadataPtr virStorageFileGetMetadataFromFD(const char *path, int fd, - int format, - virStorageFileMetadata *meta) + int format) { + virStorageFileMetadata *meta = NULL; unsigned char *head = NULL; ssize_t len = STORAGE_MAX_HEAD; - int ret = -1; + virStorageFileMetadata *ret = NULL; struct stat sb; - memset(meta, 0, sizeof(*meta)); + if (VIR_ALLOC(meta) < 0) { + virReportOOMError(); + return NULL; + } if (fstat(fd, &sb) < 0) { virReportSystemError(errno, _("cannot stat file '%s'"), path); - return -1; + goto cleanup; } - /* No header to probe for directories */ - if (S_ISDIR(sb.st_mode)) { - return 0; - } + /* 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); - return -1; + goto cleanup; } if (VIR_ALLOC_N(head, len) < 0) { virReportOOMError(); - return -1; + goto cleanup; } if ((len = read(fd, head, len)) < 0) { @@ -904,9 +906,13 @@ virStorageFileGetMetadataFromFD(const char *path, goto cleanup; } - ret = virStorageFileGetMetadataFromBuf(format, path, head, len, meta); + if (virStorageFileGetMetadataFromBuf(format, path, head, len, meta) < 0) + goto cleanup; + ret = meta; + meta = NULL; cleanup: + virStorageFileFreeMetadata(meta); VIR_FREE(head); return ret; } @@ -934,21 +940,12 @@ virStorageFileGetMetadataRecurse(const char *path, int format, return NULL; } - if (VIR_ALLOC(ret) < 0) { - virReportOOMError(); - VIR_FORCE_CLOSE(fd); - return NULL; - } - if (virStorageFileGetMetadataFromFD(path, fd, format, ret) < 0) { - virStorageFileFreeMetadata(ret); - VIR_FORCE_CLOSE(fd); - return NULL; - } + ret = virStorageFileGetMetadataFromFD(path, fd, format); if (VIR_CLOSE(fd) < 0) virReportSystemError(errno, _("could not close file %s"), path); - if (ret->backingStoreIsFile) { + if (ret && ret->backingStoreIsFile) { if (ret->backingStoreFormat == VIR_STORAGE_FILE_AUTO && !allow_probe) ret->backingStoreFormat = VIR_STORAGE_FILE_RAW; else if (ret->backingStoreFormat == VIR_STORAGE_FILE_AUTO_SAFE) diff --git a/src/util/storage_file.h b/src/util/storage_file.h index 18dea6a..9a60451 100644 --- a/src/util/storage_file.h +++ b/src/util/storage_file.h @@ -73,10 +73,9 @@ virStorageFileMetadataPtr virStorageFileGetMetadata(const char *path, int format, uid_t uid, gid_t gid, bool allow_probe); -int virStorageFileGetMetadataFromFD(const char *path, - int fd, - int format, - virStorageFileMetadata *meta); +virStorageFileMetadataPtr virStorageFileGetMetadataFromFD(const char *path, + int fd, + int format); void virStorageFileFreeMetadata(virStorageFileMetadataPtr meta);