[libvirt] [PATCHv3 00/36] Network backed backing chains and block jobs
First 7 patches of v2 are already pushed. First ~6 patches of this series were already ACKed, but can't be pushed due to the freeze and rebase conflicts of changing order of the patches. Thus I'm reposting them. Peter Krempa (36): storage: backend: Add unique id retrieval API storage: Add API to check accessibility of storage volumes storage: Move virStorageFileGetMetadata to the storage driver storage: Determine the local storage type right away test: storage: Initialize storage source to correct type storage: backend: Add possibility to suppress errors from backend lookup storage: Switch metadata crawler to use storage driver to get unique path storage: Switch metadata crawler to use storage driver to read headers storage: Switch metadata crawler to use storage driver file access check storage: Add infrastructure to parse remote network backing names storage: Change to new backing store parser storage: Traverse backing chains of network disks util: string: Return element count from virStringSplit util: string: Add helper to free non-NULL terminated string arrays util: storagefile: Add helper to resolve "../", "./" and "////" in paths util: storage: Add helper to resolve relative path difference util: storagefile: Add canonicalization to virStorageFileSimplifyPath storage: gluster: Add backend to return unique storage file path qemu: json: Add format strings for optional command arguments tests: storagetest: Unify and reformat storage chain format string tests: virstoragetest: Remove "expBackingStore" field tests: virstoragetest: Fix output when hitting errors storage: Store relative path only for relatively backed storage tests: virstoragetest: Remove now unused pathAbs util: storage: Remove now redundant backingRelative from virStorageSource tests: virstoragetest: Don't test relative start of backing chains tests: virstoragetest: Remove unneeded relative test plumbing storage: Don't canonicalize paths unnecessarily storage: Don't store parent directory of an image explicitly qemu: monitor: Add argument for specifying backing name for block commit qemu: monitor: Add support for backing name specification for block-stream lib: Introduce flag VIR_DOMAIN_BLOCK_COMMIT_RELATIVE lib: Introduce flag VIR_DOMAIN_BLOCK_REBASE_RELATIVE qemu: caps: Add capability for change-backing-file command qemu: Add support for networked disks for block commit qemu: Add support for networked disks for block pull/block rebase cfg.mk | 2 +- include/libvirt/libvirt.h.in | 6 + src/Makefile.am | 2 + src/libvirt_private.syms | 7 +- src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_domain.c | 10 +- src/qemu/qemu_driver.c | 79 +++- src/qemu/qemu_migration.c | 6 +- src/qemu/qemu_monitor.c | 21 +- src/qemu/qemu_monitor.h | 4 +- src/qemu/qemu_monitor_json.c | 139 ++++-- src/qemu/qemu_monitor_json.h | 2 + src/security/virt-aa-helper.c | 2 + src/storage/storage_backend.c | 16 +- src/storage/storage_backend.h | 12 +- src/storage/storage_backend_fs.c | 62 +++ src/storage/storage_backend_gluster.c | 92 ++++ src/storage/storage_driver.c | 212 +++++++++ src/storage/storage_driver.h | 7 + src/util/virstoragefile.c | 822 +++++++++++++++++++++++----------- src/util/virstoragefile.h | 34 +- src/util/virstring.c | 44 +- src/util/virstring.h | 7 + tests/Makefile.am | 8 +- tests/qemumonitorjsontest.c | 2 +- tests/virstoragetest.c | 491 +++++++++++++------- tests/virstringtest.c | 14 +- tools/virsh-domain.c | 29 +- tools/virsh.pod | 10 +- 30 files changed, 1633 insertions(+), 512 deletions(-) -- 1.9.3
Different protocols have different means to uniquely identify a storage file. This patch implements a storage driver API to retrieve a unique string describing a volume. The current implementation works for local storage only and returns the canonical path of the volume. To add caching support the local filesystem driver now has a private structure holding the cached string, which is created only when it's initially accessed. This patch provides the implementation for local files only for start. --- Notes: Version 3: - changed name of variable from uid to canonpath - documented validity scope of the returned name src/storage/storage_backend.h | 3 +++ src/storage/storage_backend_fs.c | 49 ++++++++++++++++++++++++++++++++++++++++ src/storage/storage_driver.c | 30 ++++++++++++++++++++++++ src/storage/storage_driver.h | 1 + 4 files changed, 83 insertions(+) diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 6be5ca7..5d71cde 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -195,6 +195,8 @@ typedef ssize_t ssize_t max_len, char **buf); +typedef const char * +(*virStorageFileBackendGetUniqueIdentifier)(virStorageSourcePtr src); virStorageFileBackendPtr virStorageFileBackendForType(int type, int protocol); @@ -211,6 +213,7 @@ struct _virStorageFileBackend { virStorageFileBackendInit backendInit; virStorageFileBackendDeinit backendDeinit; virStorageFileBackendReadHeader storageFileReadHeader; + virStorageFileBackendGetUniqueIdentifier storageFileGetUniqueIdentifier; /* The following group of callbacks is expected to set errno * and return -1 on error. No libvirt error shall be reported */ diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 003c6df..aed07a6 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1333,6 +1333,14 @@ virStorageBackend virStorageBackendNetFileSystem = { }; +typedef struct _virStorageFileBackendFsPriv virStorageFileBackendFsPriv; +typedef virStorageFileBackendFsPriv *virStorageFileBackendFsPrivPtr; + +struct _virStorageFileBackendFsPriv { + char *canonpath; /* unique file identifier (canonical path) */ +}; + + static void virStorageFileBackendFileDeinit(virStorageSourcePtr src) { @@ -1340,16 +1348,27 @@ virStorageFileBackendFileDeinit(virStorageSourcePtr src) virStorageTypeToString(virStorageSourceGetActualType(src)), src->path); + virStorageFileBackendFsPrivPtr priv = src->drv->priv; + + VIR_FREE(priv->canonpath); + VIR_FREE(priv); } static int virStorageFileBackendFileInit(virStorageSourcePtr src) { + virStorageFileBackendFsPrivPtr priv = NULL; + VIR_DEBUG("initializing FS storage file %p (%s:%s)", src, virStorageTypeToString(virStorageSourceGetActualType(src)), src->path); + if (VIR_ALLOC(priv) < 0) + return -1; + + src->drv->priv = priv; + return 0; } @@ -1397,6 +1416,23 @@ virStorageFileBackendFileReadHeader(virStorageSourcePtr src, } +static const char * +virStorageFileBackendFileGetUniqueIdentifier(virStorageSourcePtr src) +{ + virStorageFileBackendFsPrivPtr priv = src->drv->priv; + + if (!priv->canonpath) { + if (!(priv->canonpath = canonicalize_file_name(src->path))) { + virReportSystemError(errno, _("can't canonicalize path '%s'"), + src->path); + return NULL; + } + } + + return priv->canonpath; +} + + virStorageFileBackend virStorageFileBackendFile = { .type = VIR_STORAGE_TYPE_FILE, @@ -1406,6 +1442,8 @@ virStorageFileBackend virStorageFileBackendFile = { .storageFileUnlink = virStorageFileBackendFileUnlink, .storageFileStat = virStorageFileBackendFileStat, .storageFileReadHeader = virStorageFileBackendFileReadHeader, + + .storageFileGetUniqueIdentifier = virStorageFileBackendFileGetUniqueIdentifier, }; @@ -1417,7 +1455,18 @@ virStorageFileBackend virStorageFileBackendBlock = { .storageFileStat = virStorageFileBackendFileStat, .storageFileReadHeader = virStorageFileBackendFileReadHeader, + + .storageFileGetUniqueIdentifier = virStorageFileBackendFileGetUniqueIdentifier, }; +virStorageFileBackend virStorageFileBackendDir = { + .type = VIR_STORAGE_TYPE_DIR, + + .backendInit = virStorageFileBackendFileInit, + .backendDeinit = virStorageFileBackendFileDeinit, + + .storageFileGetUniqueIdentifier = virStorageFileBackendFileGetUniqueIdentifier, +}; + #endif /* WITH_STORAGE_FS */ diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index cd6babe..0c779c5 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2987,3 +2987,33 @@ virStorageFileReadHeader(virStorageSourcePtr src, return ret; } + + +/* + * virStorageFileGetUniqueIdentifier: Get a unique string describing the volume + * + * @src: file structure pointing to the file + * + * Returns a string uniquely describing a single volume (canonical path). + * The string shall not be freed and is valid until the storage file is + * deinitialized. Returns NULL on error and sets a libvirt error code */ +const char * +virStorageFileGetUniqueIdentifier(virStorageSourcePtr src) +{ + if (!virStorageFileIsInitialized(src)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("storage file backend not initialized")); + return NULL; + } + + if (!src->drv->backend->storageFileGetUniqueIdentifier) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unique storage file identifier not implemented for " + "storage type %s (protocol: %s)'"), + virStorageTypeToString(src->type), + virStorageNetProtocolTypeToString(src->protocol)); + return NULL; + } + + return src->drv->backend->storageFileGetUniqueIdentifier(src); +} diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h index d67d74b..9280ef0 100644 --- a/src/storage/storage_driver.h +++ b/src/storage/storage_driver.h @@ -41,6 +41,7 @@ int virStorageFileStat(virStorageSourcePtr src, ssize_t virStorageFileReadHeader(virStorageSourcePtr src, ssize_t max_len, char **buf); +const char *virStorageFileGetUniqueIdentifier(virStorageSourcePtr src); int storageRegister(void); -- 1.9.3
On 05/30/2014 02:37 AM, Peter Krempa wrote:
Different protocols have different means to uniquely identify a storage file. This patch implements a storage driver API to retrieve a unique string describing a volume. The current implementation works for local storage only and returns the canonical path of the volume.
To add caching support the local filesystem driver now has a private structure holding the cached string, which is created only when it's initially accessed.
This patch provides the implementation for local files only for start. ---
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Add a storage driver API equivalent of the access() function. Implementations for the filesystem and gluster backends are provided. --- Notes: Version 3: - fixed typos - ACKed by Eric src/storage/storage_backend.h | 5 +++++ src/storage/storage_backend_fs.c | 13 +++++++++++++ src/storage/storage_backend_gluster.c | 11 +++++++++++ src/storage/storage_driver.c | 24 ++++++++++++++++++++++++ src/storage/storage_driver.h | 1 + 5 files changed, 54 insertions(+) diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 5d71cde..56643fb 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -198,6 +198,10 @@ typedef ssize_t typedef const char * (*virStorageFileBackendGetUniqueIdentifier)(virStorageSourcePtr src); +typedef int +(*virStorageFileBackendAccess)(virStorageSourcePtr src, + int mode); + virStorageFileBackendPtr virStorageFileBackendForType(int type, int protocol); @@ -220,6 +224,7 @@ struct _virStorageFileBackend { virStorageFileBackendCreate storageFileCreate; virStorageFileBackendUnlink storageFileUnlink; virStorageFileBackendStat storageFileStat; + virStorageFileBackendAccess storageFileAccess; }; #endif /* __VIR_STORAGE_BACKEND_H__ */ diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index aed07a6..414f2c7 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1433,6 +1433,15 @@ virStorageFileBackendFileGetUniqueIdentifier(virStorageSourcePtr src) } +static int +virStorageFileBackendFileAccess(virStorageSourcePtr src, + int mode) +{ + return virFileAccessibleAs(src->path, mode, + src->drv->uid, src->drv->gid); +} + + virStorageFileBackend virStorageFileBackendFile = { .type = VIR_STORAGE_TYPE_FILE, @@ -1442,6 +1451,7 @@ virStorageFileBackend virStorageFileBackendFile = { .storageFileUnlink = virStorageFileBackendFileUnlink, .storageFileStat = virStorageFileBackendFileStat, .storageFileReadHeader = virStorageFileBackendFileReadHeader, + .storageFileAccess = virStorageFileBackendFileAccess, .storageFileGetUniqueIdentifier = virStorageFileBackendFileGetUniqueIdentifier, }; @@ -1455,6 +1465,7 @@ virStorageFileBackend virStorageFileBackendBlock = { .storageFileStat = virStorageFileBackendFileStat, .storageFileReadHeader = virStorageFileBackendFileReadHeader, + .storageFileAccess = virStorageFileBackendFileAccess, .storageFileGetUniqueIdentifier = virStorageFileBackendFileGetUniqueIdentifier, }; @@ -1466,6 +1477,8 @@ virStorageFileBackend virStorageFileBackendDir = { .backendInit = virStorageFileBackendFileInit, .backendDeinit = virStorageFileBackendFileDeinit, + .storageFileAccess = virStorageFileBackendFileAccess, + .storageFileGetUniqueIdentifier = virStorageFileBackendFileGetUniqueIdentifier, }; diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index af6792f..3db4e66 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -703,6 +703,16 @@ virStorageFileBackendGlusterReadHeader(virStorageSourcePtr src, } +static int +virStorageFileBackendGlusterAccess(virStorageSourcePtr src, + int mode) +{ + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; + + return glfs_access(priv->vol, src->path, mode); +} + + virStorageFileBackend virStorageFileBackendGluster = { .type = VIR_STORAGE_TYPE_NETWORK, .protocol = VIR_STORAGE_NET_PROTOCOL_GLUSTER, @@ -713,4 +723,5 @@ virStorageFileBackend virStorageFileBackendGluster = { .storageFileUnlink = virStorageFileBackendGlusterUnlink, .storageFileStat = virStorageFileBackendGlusterStat, .storageFileReadHeader = virStorageFileBackendGlusterReadHeader, + .storageFileAccess = virStorageFileBackendGlusterAccess, }; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 0c779c5..f66c259 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -3017,3 +3017,27 @@ virStorageFileGetUniqueIdentifier(virStorageSourcePtr src) return src->drv->backend->storageFileGetUniqueIdentifier(src); } + + +/** + * virStorageFileAccess: Check accessibility of a storage file + * + * @src: storage file to check access permissions + * @mode: accessibility check options (see man 2 access) + * + * Returns 0 on success, -1 on error and sets errno. No libvirt + * error is reported. Returns -2 if the operation isn't supported + * by libvirt storage backend. + */ +int +virStorageFileAccess(virStorageSourcePtr src, + int mode) +{ + if (!virStorageFileIsInitialized(src) || + !src->drv->backend->storageFileAccess) { + errno = ENOSYS; + return -2; + } + + return src->drv->backend->storageFileAccess(src, mode); +} diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h index 9280ef0..5452df2 100644 --- a/src/storage/storage_driver.h +++ b/src/storage/storage_driver.h @@ -42,6 +42,7 @@ ssize_t virStorageFileReadHeader(virStorageSourcePtr src, ssize_t max_len, char **buf); const char *virStorageFileGetUniqueIdentifier(virStorageSourcePtr src); +int virStorageFileAccess(virStorageSourcePtr src, int mode); int storageRegister(void); -- 1.9.3
My future work will modify the metadata crawler function to use the storage driver file APIs to access the files instead of accessing them directly so that we will be able to request the metadata for remote files too. To avoid linking the storage driver to every helper file using the utils code, the backing chain traversal function needs to be moved to the storage driver source. Additionally the virt-aa-helper and virstoragetest programs need to be linked with the storage driver as a result of this change. --- Notes: Version 3: - fixed whitespace issue - ACKed by Eric cfg.mk | 2 +- src/Makefile.am | 2 + src/libvirt_private.syms | 2 +- src/qemu/qemu_domain.c | 2 + src/security/virt-aa-helper.c | 2 + src/storage/storage_driver.c | 233 ++++++++++++++++++++++++++++++++++++++++++ src/storage/storage_driver.h | 5 + src/util/virstoragefile.c | 233 +----------------------------------------- src/util/virstoragefile.h | 7 +- tests/Makefile.am | 8 +- tests/virstoragetest.c | 2 + 11 files changed, 259 insertions(+), 239 deletions(-) diff --git a/cfg.mk b/cfg.mk index 5ff2721..9e8fcec 100644 --- a/cfg.mk +++ b/cfg.mk @@ -774,7 +774,7 @@ sc_prohibit_cross_inclusion: access/ | conf/) safe="($$dir|conf|util)";; \ locking/) safe="($$dir|util|conf|rpc)";; \ cpu/| network/| node_device/| rpc/| security/| storage/) \ - safe="($$dir|util|conf)";; \ + safe="($$dir|util|conf|storage)";; \ xenapi/ | xenxs/ ) safe="($$dir|util|conf|xen)";; \ *) safe="($$dir|$(mid_dirs)|util)";; \ esac; \ diff --git a/src/Makefile.am b/src/Makefile.am index cfb7097..5028f0d 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -2565,8 +2565,10 @@ virt_aa_helper_LDFLAGS = \ $(PIE_LDFLAGS) \ $(NULL) virt_aa_helper_LDADD = \ + libvirt.la \ libvirt_conf.la \ libvirt_util.la \ + libvirt_driver_storage_impl.la \ ../gnulib/lib/libgnu.la if WITH_DTRACE_PROBES virt_aa_helper_LDADD += libvirt_probes.lo diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cb635cd..91f13a4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1863,9 +1863,9 @@ virStorageFileFeatureTypeToString; virStorageFileFormatTypeFromString; virStorageFileFormatTypeToString; virStorageFileGetLVMKey; -virStorageFileGetMetadata; virStorageFileGetMetadataFromBuf; virStorageFileGetMetadataFromFD; +virStorageFileGetMetadataFromFDInternal; virStorageFileGetSCSIKey; virStorageFileIsClusterFS; virStorageFileParseChainIndex; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 78cfdc6..502b7bd 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -40,6 +40,8 @@ #include "virstoragefile.h" #include "virstring.h" +#include "storage/storage_driver.h" + #include <sys/time.h> #include <fcntl.h> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 32fc04a..bf540b4 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -55,6 +55,8 @@ #include "virrandom.h" #include "virstring.h" +#include "storage/storage_driver.h" + #define VIR_FROM_THIS VIR_FROM_SECURITY static char *progname; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index f66c259..59f6fa8 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -49,6 +49,7 @@ #include "configmake.h" #include "virstring.h" #include "viraccessapicheck.h" +#include "dirname.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -3041,3 +3042,235 @@ virStorageFileAccess(virStorageSourcePtr src, return src->drv->backend->storageFileAccess(src, mode); } + + +/** + * Given a starting point START (a directory containing the original + * file, if the original file was opened via a relative path; ignored + * if NAME is absolute), determine the location of the backing file + * NAME (possibly relative), and compute the relative DIRECTORY + * (optional) and CANONICAL (mandatory) location of the backing file. + * Return 0 on success, negative on error. + */ +static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4) +virFindBackingFile(const char *start, const char *path, + char **directory, char **canonical) +{ + /* FIXME - when we eventually allow non-raw network devices, we + * must ensure that we handle backing files the same way as qemu. + * For a qcow2 top file of gluster://server/vol/img, qemu treats + * the relative backing file 'rel' as meaning + * 'gluster://server/vol/rel', while the backing file '/abs' is + * used as a local file. But we cannot canonicalize network + * devices via canonicalize_file_name(), because they are not part + * of the local file system. */ + char *combined = NULL; + int ret = -1; + + if (*path == '/') { + /* Safe to cast away const */ + combined = (char *)path; + } else if (virAsprintf(&combined, "%s/%s", start, path) < 0) { + goto cleanup; + } + + if (directory && !(*directory = mdir_name(combined))) { + virReportOOMError(); + goto cleanup; + } + + if (virFileAccessibleAs(combined, F_OK, geteuid(), getegid()) < 0) { + virReportSystemError(errno, + _("Cannot access backing file '%s'"), + combined); + goto cleanup; + } + + if (!(*canonical = canonicalize_file_name(combined))) { + virReportSystemError(errno, + _("Can't canonicalize path '%s'"), path); + goto cleanup; + } + + ret = 0; + + cleanup: + if (combined != path) + VIR_FREE(combined); + return ret; +} + + +/* Recursive workhorse for virStorageFileGetMetadata. */ +static int +virStorageFileGetMetadataRecurse(virStorageSourcePtr src, + const char *canonPath, + uid_t uid, gid_t gid, + bool allow_probe, + virHashTablePtr cycle) +{ + int fd; + int ret = -1; + virStorageSourcePtr backingStore = NULL; + int backingFormat; + + VIR_DEBUG("path=%s canonPath=%s dir=%s format=%d uid=%d gid=%d probe=%d", + src->path, canonPath, NULLSTR(src->relDir), src->format, + (int)uid, (int)gid, allow_probe); + + if (virHashLookup(cycle, canonPath)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("backing store for %s is self-referential"), + src->path); + return -1; + } + + if (virHashAddEntry(cycle, canonPath, (void *)1) < 0) + return -1; + + if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) { + if ((fd = virFileOpenAs(src->path, O_RDONLY, 0, uid, gid, 0)) < 0) { + virReportSystemError(-fd, _("Failed to open file '%s'"), + src->path); + return -1; + } + + if (virStorageFileGetMetadataFromFDInternal(src, fd, + &backingFormat) < 0) { + VIR_FORCE_CLOSE(fd); + return -1; + } + + if (VIR_CLOSE(fd) < 0) + VIR_WARN("could not close file %s", src->path); + } else { + /* TODO: currently we call this only for local storage */ + return 0; + } + + /* check whether we need to go deeper */ + if (!src->backingStoreRaw) + return 0; + + if (VIR_ALLOC(backingStore) < 0) + return -1; + + if (VIR_STRDUP(backingStore->relPath, src->backingStoreRaw) < 0) + goto cleanup; + + if (virStorageIsFile(src->backingStoreRaw)) { + backingStore->type = VIR_STORAGE_TYPE_FILE; + + if (virFindBackingFile(src->relDir, + src->backingStoreRaw, + &backingStore->relDir, + &backingStore->path) < 0) { + /* the backing file is (currently) unavailable, treat this + * file as standalone: + * backingStoreRaw is kept to mark broken image chains */ + VIR_WARN("Backing file '%s' of image '%s' is missing.", + src->backingStoreRaw, src->path); + ret = 0; + goto cleanup; + } + } else { + /* TODO: To satisfy the test case, copy the network URI as path. This + * will be removed later. */ + backingStore->type = VIR_STORAGE_TYPE_NETWORK; + + if (VIR_STRDUP(backingStore->path, src->backingStoreRaw) < 0) + goto cleanup; + } + + if (backingFormat == VIR_STORAGE_FILE_AUTO && !allow_probe) + backingStore->format = VIR_STORAGE_FILE_RAW; + else if (backingFormat == VIR_STORAGE_FILE_AUTO_SAFE) + backingStore->format = VIR_STORAGE_FILE_AUTO; + else + backingStore->format = backingFormat; + + if (virStorageFileGetMetadataRecurse(backingStore, + backingStore->path, + uid, gid, allow_probe, + cycle) < 0) { + /* if we fail somewhere midway, just accept and return a + * broken chain */ + ret = 0; + goto cleanup; + } + + src->backingStore = backingStore; + backingStore = NULL; + ret = 0; + + cleanup: + virStorageSourceFree(backingStore); + return ret; +} + + +/** + * virStorageFileGetMetadata: + * + * 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. Recurses through + * the entire chain. + * + * Open files using UID and GID (or pass -1 for the current user/group). + * Treat any backing files without explicit type as raw, unless ALLOW_PROBE. + * + * 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 result after use via virStorageSourceFree. + */ +int +virStorageFileGetMetadata(virStorageSourcePtr src, + uid_t uid, gid_t gid, + bool allow_probe) +{ + VIR_DEBUG("path=%s format=%d uid=%d gid=%d probe=%d", + src->path, src->format, (int)uid, (int)gid, allow_probe); + + virHashTablePtr cycle = NULL; + char *canonPath = NULL; + int ret = -1; + + if (!(cycle = virHashCreate(5, NULL))) + return -1; + + if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) { + if (!(canonPath = canonicalize_file_name(src->path))) { + virReportSystemError(errno, _("unable to resolve '%s'"), + src->path); + goto cleanup; + } + + if (!src->relPath && + VIR_STRDUP(src->relPath, src->path) < 0) + goto cleanup; + + if (!src->relDir && + !(src->relDir = mdir_name(src->path))) { + virReportOOMError(); + goto cleanup; + } + } else { + /* TODO: currently unimplemented for non-local storage */ + ret = 0; + goto cleanup; + } + + if (src->format <= VIR_STORAGE_FILE_NONE) + src->format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; + + ret = virStorageFileGetMetadataRecurse(src, canonPath, uid, gid, + allow_probe, cycle); + + cleanup: + VIR_FREE(canonPath); + virHashFree(cycle); + return ret; +} diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h index 5452df2..bd9e9ed 100644 --- a/src/storage/storage_driver.h +++ b/src/storage/storage_driver.h @@ -44,6 +44,11 @@ ssize_t virStorageFileReadHeader(virStorageSourcePtr src, const char *virStorageFileGetUniqueIdentifier(virStorageSourcePtr src); int virStorageFileAccess(virStorageSourcePtr src, int mode); +int virStorageFileGetMetadata(virStorageSourcePtr src, + uid_t uid, gid_t gid, + bool allow_probe) + ATTRIBUTE_NONNULL(1); + int storageRegister(void); #endif /* __VIR_STORAGE_DRIVER_H__ */ diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index c9b6187..4956808 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -28,7 +28,6 @@ #include <unistd.h> #include <fcntl.h> #include <stdlib.h> -#include "dirname.h" #include "viralloc.h" #include "virerror.h" #include "virlog.h" @@ -565,62 +564,6 @@ qedGetBackingStore(char **res, return BACKING_STORE_OK; } -/** - * Given a starting point START (a directory containing the original - * file, if the original file was opened via a relative path; ignored - * if NAME is absolute), determine the location of the backing file - * NAME (possibly relative), and compute the relative DIRECTORY - * (optional) and CANONICAL (mandatory) location of the backing file. - * Return 0 on success, negative on error. - */ -static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4) -virFindBackingFile(const char *start, const char *path, - char **directory, char **canonical) -{ - /* FIXME - when we eventually allow non-raw network devices, we - * must ensure that we handle backing files the same way as qemu. - * For a qcow2 top file of gluster://server/vol/img, qemu treats - * the relative backing file 'rel' as meaning - * 'gluster://server/vol/rel', while the backing file '/abs' is - * used as a local file. But we cannot canonicalize network - * devices via canonicalize_file_name(), because they are not part - * of the local file system. */ - char *combined = NULL; - int ret = -1; - - if (*path == '/') { - /* Safe to cast away const */ - combined = (char *)path; - } else if (virAsprintf(&combined, "%s/%s", start, path) < 0) { - goto cleanup; - } - - if (directory && !(*directory = mdir_name(combined))) { - virReportOOMError(); - goto cleanup; - } - - if (virFileAccessibleAs(combined, F_OK, geteuid(), getegid()) < 0) { - virReportSystemError(errno, - _("Cannot access backing file '%s'"), - combined); - goto cleanup; - } - - if (!(*canonical = canonicalize_file_name(combined))) { - virReportSystemError(errno, - _("Can't canonicalize path '%s'"), path); - goto cleanup; - } - - ret = 0; - - cleanup: - if (combined != path) - VIR_FREE(combined); - return ret; -} - static bool virStorageFileMatchesMagic(int format, @@ -1012,7 +955,7 @@ virStorageFileGetMetadataFromBuf(const char *path, /* Internal version that also supports a containing directory name. */ -static int +int virStorageFileGetMetadataFromFDInternal(virStorageSourcePtr meta, int fd, int *backingFormat) @@ -1111,180 +1054,6 @@ virStorageFileGetMetadataFromFD(const char *path, } -/* Recursive workhorse for virStorageFileGetMetadata. */ -static int -virStorageFileGetMetadataRecurse(virStorageSourcePtr src, - const char *canonPath, - uid_t uid, gid_t gid, - bool allow_probe, - virHashTablePtr cycle) -{ - int fd; - int ret = -1; - virStorageSourcePtr backingStore = NULL; - int backingFormat; - - VIR_DEBUG("path=%s canonPath=%s dir=%s format=%d uid=%d gid=%d probe=%d", - src->path, canonPath, NULLSTR(src->relDir), src->format, - (int)uid, (int)gid, allow_probe); - - if (virHashLookup(cycle, canonPath)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("backing store for %s is self-referential"), - src->path); - return -1; - } - - if (virHashAddEntry(cycle, canonPath, (void *)1) < 0) - return -1; - - if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) { - if ((fd = virFileOpenAs(src->path, O_RDONLY, 0, uid, gid, 0)) < 0) { - virReportSystemError(-fd, _("Failed to open file '%s'"), - src->path); - return -1; - } - - if (virStorageFileGetMetadataFromFDInternal(src, fd, - &backingFormat) < 0) { - VIR_FORCE_CLOSE(fd); - return -1; - } - - if (VIR_CLOSE(fd) < 0) - VIR_WARN("could not close file %s", src->path); - } else { - /* TODO: currently we call this only for local storage */ - return 0; - } - - /* check whether we need to go deeper */ - if (!src->backingStoreRaw) - return 0; - - if (VIR_ALLOC(backingStore) < 0) - return -1; - - if (VIR_STRDUP(backingStore->relPath, src->backingStoreRaw) < 0) - goto cleanup; - - if (virStorageIsFile(src->backingStoreRaw)) { - backingStore->type = VIR_STORAGE_TYPE_FILE; - - if (virFindBackingFile(src->relDir, - src->backingStoreRaw, - &backingStore->relDir, - &backingStore->path) < 0) { - /* the backing file is (currently) unavailable, treat this - * file as standalone: - * backingStoreRaw is kept to mark broken image chains */ - VIR_WARN("Backing file '%s' of image '%s' is missing.", - src->backingStoreRaw, src->path); - ret = 0; - goto cleanup; - } - } else { - /* TODO: To satisfy the test case, copy the network URI as path. This - * will be removed later. */ - backingStore->type = VIR_STORAGE_TYPE_NETWORK; - - if (VIR_STRDUP(backingStore->path, src->backingStoreRaw) < 0) - goto cleanup; - } - - if (backingFormat == VIR_STORAGE_FILE_AUTO && !allow_probe) - backingStore->format = VIR_STORAGE_FILE_RAW; - else if (backingFormat == VIR_STORAGE_FILE_AUTO_SAFE) - backingStore->format = VIR_STORAGE_FILE_AUTO; - else - backingStore->format = backingFormat; - - if (virStorageFileGetMetadataRecurse(backingStore, - backingStore->path, - uid, gid, allow_probe, - cycle) < 0) { - /* if we fail somewhere midway, just accept and return a - * broken chain */ - ret = 0; - goto cleanup; - } - - src->backingStore = backingStore; - backingStore = NULL; - ret = 0; - - cleanup: - virStorageSourceFree(backingStore); - return ret; -} - - -/** - * virStorageFileGetMetadata: - * - * 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. Recurses through - * the entire chain. - * - * Open files using UID and GID (or pass -1 for the current user/group). - * Treat any backing files without explicit type as raw, unless ALLOW_PROBE. - * - * 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 result after use via virStorageSourceFree. - */ -int -virStorageFileGetMetadata(virStorageSourcePtr src, - uid_t uid, gid_t gid, - bool allow_probe) -{ - VIR_DEBUG("path=%s format=%d uid=%d gid=%d probe=%d", - src->path, src->format, (int)uid, (int)gid, allow_probe); - - virHashTablePtr cycle = NULL; - char *canonPath = NULL; - int ret = -1; - - if (!(cycle = virHashCreate(5, NULL))) - return -1; - - if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) { - if (!(canonPath = canonicalize_file_name(src->path))) { - virReportSystemError(errno, _("unable to resolve '%s'"), - src->path); - goto cleanup; - } - - if (!src->relPath && - VIR_STRDUP(src->relPath, src->path) < 0) - goto cleanup; - - if (!src->relDir && - !(src->relDir = mdir_name(src->path))) { - virReportOOMError(); - goto cleanup; - } - } else { - /* TODO: currently unimplemented for non-local storage */ - ret = 0; - goto cleanup; - } - - if (src->format <= VIR_STORAGE_FILE_NONE) - src->format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; - - ret = virStorageFileGetMetadataRecurse(src, canonPath, uid, gid, - allow_probe, cycle); - - cleanup: - VIR_FREE(canonPath); - virHashFree(cycle); - return ret; -} - /** * virStorageFileChainCheckBroken * diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 158806b..9947203 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -265,10 +265,9 @@ struct _virStorageSource { int virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid); -int virStorageFileGetMetadata(virStorageSourcePtr src, - uid_t uid, gid_t gid, - bool allow_probe) - ATTRIBUTE_NONNULL(1); +int virStorageFileGetMetadataFromFDInternal(virStorageSourcePtr meta, + int fd, + int *backingFormat); virStorageSourcePtr virStorageFileGetMetadataFromFD(const char *path, int fd, int format, diff --git a/tests/Makefile.am b/tests/Makefile.am index 5ef8940..f9f2b84 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -869,7 +869,13 @@ virstringtest_LDADD = $(LDADDS) virstoragetest_SOURCES = \ virstoragetest.c testutils.h testutils.c -virstoragetest_LDADD = $(LDADDS) +virstoragetest_LDADD = $(LDADDS) \ + ../src/libvirt.la \ + ../src/libvirt_conf.la \ + ../src/libvirt_util.la \ + ../src/libvirt_driver_storage_impl.la \ + ../gnulib/lib/libgnu.la \ + $(NULL) viridentitytest_SOURCES = \ viridentitytest.c testutils.h testutils.c diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index d49098e..fb96c71 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -31,6 +31,8 @@ #include "virstring.h" #include "dirname.h" +#include "storage/storage_driver.h" + #define VIR_FROM_THIS VIR_FROM_NONE VIR_LOG_INIT("tests.storagetest"); -- 1.9.3
When walking the backing chain we previously set the storage type to _FILE and let the virStorageFileGetMetadataFromFDInternal update it to the correct type later on. This patch moves the actual storage type determination to the place where we parse the backing store name so that the code can later be switched to use virStorageFileReadHeader() directly. --- Notes: Version 3: - ACKed by Eric src/storage/storage_driver.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 59f6fa8..f92a553 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -3111,6 +3111,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, { int fd; int ret = -1; + struct stat st; virStorageSourcePtr backingStore = NULL; int backingFormat; @@ -3173,6 +3174,16 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, ret = 0; goto cleanup; } + + /* update the type for local storage */ + if (stat(backingStore->path, &st) == 0) { + if (S_ISDIR(st.st_mode)) { + backingStore->type = VIR_STORAGE_TYPE_DIR; + backingStore->format = VIR_STORAGE_FILE_DIR; + } else if (S_ISBLK(st.st_mode)) { + backingStore->type = VIR_STORAGE_TYPE_BLOCK; + } + } } else { /* TODO: To satisfy the test case, copy the network URI as path. This * will be removed later. */ -- 1.9.3
Stat the path of the storage file being tested to set the correct type into the virStorageSource. This will avoid breaking the test suite when inquiring metadata of directory paths in the next patches. --- Notes: Version 3: - ACKed by eric tests/virstoragetest.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index fb96c71..746bf63 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -98,6 +98,7 @@ testStorageFileGetMetadata(const char *path, uid_t uid, gid_t gid, bool allow_probe) { + struct stat st; virStorageSourcePtr ret = NULL; if (VIR_ALLOC(ret) < 0) @@ -106,6 +107,15 @@ testStorageFileGetMetadata(const char *path, ret->type = VIR_STORAGE_TYPE_FILE; ret->format = format; + if (stat(path, &st) == 0) { + if (S_ISDIR(st.st_mode)) { + ret->type = VIR_STORAGE_TYPE_DIR; + ret->format = VIR_STORAGE_FILE_DIR; + } else if (S_ISBLK(st.st_mode)) { + ret->type = VIR_STORAGE_TYPE_BLOCK; + } + } + if (VIR_STRDUP(ret->relPath, path) < 0) goto error; -- 1.9.3
Add a new function wrapper and tweak the storage file backend lookup function so that it can be used without reporting error. This will be useful in the metadata crawler code where we need silently break if metadata retrieval is not supported for the current storage type. --- Notes: Version 3: - ACKed by Eric src/storage/storage_backend.c | 16 ++++++++++++++-- src/storage/storage_backend.h | 4 +++- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index e016cc8..380ca58 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1173,8 +1173,9 @@ virStorageBackendForType(int type) virStorageFileBackendPtr -virStorageFileBackendForType(int type, - int protocol) +virStorageFileBackendForTypeInternal(int type, + int protocol, + bool report) { size_t i; @@ -1188,6 +1189,9 @@ virStorageFileBackendForType(int type, } } + if (!report) + return NULL; + if (type == VIR_STORAGE_TYPE_NETWORK) { virReportError(VIR_ERR_INTERNAL_ERROR, _("missing storage backend for network files " @@ -1203,6 +1207,14 @@ virStorageFileBackendForType(int type, } +virStorageFileBackendPtr +virStorageFileBackendForType(int type, + int protocol) +{ + return virStorageFileBackendForTypeInternal(type, protocol, true); +} + + struct diskType { int part_table_type; unsigned short offset; diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 56643fb..76c1afa 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -203,7 +203,9 @@ typedef int int mode); virStorageFileBackendPtr virStorageFileBackendForType(int type, int protocol); - +virStorageFileBackendPtr virStorageFileBackendForTypeInternal(int type, + int protocol, + bool report); struct _virStorageFileBackend { -- 1.9.3
Use the virStorageFileGetUniqueIdentifier() function to get a unique identifier regardless of the target storage type instead of relying on canonicalize_path(). A new function that checks whether we support a given image is introduced to avoid errors for unimplemented backends. --- Notes: Version 3: - fixed typo - ACKed by Eric src/storage/storage_driver.c | 77 +++++++++++++++++++++++++++++++------------- 1 file changed, 54 insertions(+), 23 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index f92a553..5c4188f 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2788,6 +2788,30 @@ virStorageFileIsInitialized(virStorageSourcePtr src) return !!src->drv; } + +static bool +virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src) +{ + int actualType = virStorageSourceGetActualType(src); + virStorageFileBackendPtr backend; + + if (!src) + return false; + + if (src->drv) { + backend = src->drv->backend; + } else { + if (!(backend = virStorageFileBackendForTypeInternal(actualType, + src->protocol, + false))) + return false; + } + + return backend->storageFileGetUniqueIdentifier && + backend->storageFileReadHeader && + backend->storageFileAccess; +} + void virStorageFileDeinit(virStorageSourcePtr src) { @@ -3104,7 +3128,6 @@ virFindBackingFile(const char *start, const char *path, /* Recursive workhorse for virStorageFileGetMetadata. */ static int virStorageFileGetMetadataRecurse(virStorageSourcePtr src, - const char *canonPath, uid_t uid, gid_t gid, bool allow_probe, virHashTablePtr cycle) @@ -3112,49 +3135,63 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, int fd; int ret = -1; struct stat st; + const char *uniqueName; virStorageSourcePtr backingStore = NULL; int backingFormat; - VIR_DEBUG("path=%s canonPath=%s dir=%s format=%d uid=%d gid=%d probe=%d", - src->path, canonPath, NULLSTR(src->relDir), src->format, + VIR_DEBUG("path=%s dir=%s format=%d uid=%d gid=%d probe=%d", + src->path, NULLSTR(src->relDir), src->format, (int)uid, (int)gid, allow_probe); - if (virHashLookup(cycle, canonPath)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("backing store for %s is self-referential"), - src->path); + /* exit if we can't load information about the current image */ + if (!virStorageFileSupportsBackingChainTraversal(src)) + return 0; + + if (virStorageFileInitAs(src, uid, gid) < 0) return -1; + + if (!(uniqueName = virStorageFileGetUniqueIdentifier(src))) + goto cleanup; + + if (virHashLookup(cycle, uniqueName)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("backing store for %s (%s) is self-referential"), + src->path, uniqueName); + goto cleanup; } - if (virHashAddEntry(cycle, canonPath, (void *)1) < 0) - return -1; + if (virHashAddEntry(cycle, uniqueName, (void *)1) < 0) + goto cleanup; if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) { if ((fd = virFileOpenAs(src->path, O_RDONLY, 0, uid, gid, 0)) < 0) { virReportSystemError(-fd, _("Failed to open file '%s'"), src->path); - return -1; + goto cleanup; } if (virStorageFileGetMetadataFromFDInternal(src, fd, &backingFormat) < 0) { VIR_FORCE_CLOSE(fd); - return -1; + goto cleanup; } if (VIR_CLOSE(fd) < 0) VIR_WARN("could not close file %s", src->path); } else { /* TODO: currently we call this only for local storage */ - return 0; + ret = 0; + goto cleanup; } /* check whether we need to go deeper */ - if (!src->backingStoreRaw) - return 0; + if (!src->backingStoreRaw) { + ret = 0; + goto cleanup; + } if (VIR_ALLOC(backingStore) < 0) - return -1; + goto cleanup; if (VIR_STRDUP(backingStore->relPath, src->backingStoreRaw) < 0) goto cleanup; @@ -3201,7 +3238,6 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, backingStore->format = backingFormat; if (virStorageFileGetMetadataRecurse(backingStore, - backingStore->path, uid, gid, allow_probe, cycle) < 0) { /* if we fail somewhere midway, just accept and return a @@ -3215,6 +3251,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, ret = 0; cleanup: + virStorageFileDeinit(src); virStorageSourceFree(backingStore); return ret; } @@ -3253,12 +3290,6 @@ virStorageFileGetMetadata(virStorageSourcePtr src, return -1; if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) { - if (!(canonPath = canonicalize_file_name(src->path))) { - virReportSystemError(errno, _("unable to resolve '%s'"), - src->path); - goto cleanup; - } - if (!src->relPath && VIR_STRDUP(src->relPath, src->path) < 0) goto cleanup; @@ -3277,7 +3308,7 @@ virStorageFileGetMetadata(virStorageSourcePtr src, if (src->format <= VIR_STORAGE_FILE_NONE) src->format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; - ret = virStorageFileGetMetadataRecurse(src, canonPath, uid, gid, + ret = virStorageFileGetMetadataRecurse(src, uid, gid, allow_probe, cycle); cleanup: -- 1.9.3
Peter Krempa wrote:
Use the virStorageFileGetUniqueIdentifier() function to get a unique identifier regardless of the target storage type instead of relying on canonicalize_path().
A new function that checks whether we support a given image is introduced to avoid errors for unimplemented backends. ---
Notes: Version 3: - fixed typo - ACKed by Eric
src/storage/storage_driver.c | 77 +++++++++++++++++++++++++++++++------------- 1 file changed, 54 insertions(+), 23 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index f92a553..5c4188f 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2788,6 +2788,30 @@ virStorageFileIsInitialized(virStorageSourcePtr src) return !!src->drv; }
+ +static bool +virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src) +{ + int actualType = virStorageSourceGetActualType(src); + virStorageFileBackendPtr backend; + + if (!src) + return false; + + if (src->drv) { + backend = src->drv->backend; + } else { + if (!(backend = virStorageFileBackendForTypeInternal(actualType, + src->protocol, + false))) + return false; + } + + return backend->storageFileGetUniqueIdentifier && + backend->storageFileReadHeader && + backend->storageFileAccess; +} + void virStorageFileDeinit(virStorageSourcePtr src) { @@ -3104,7 +3128,6 @@ virFindBackingFile(const char *start, const char *path, /* Recursive workhorse for virStorageFileGetMetadata. */ static int virStorageFileGetMetadataRecurse(virStorageSourcePtr src, - const char *canonPath, uid_t uid, gid_t gid, bool allow_probe, virHashTablePtr cycle) @@ -3112,49 +3135,63 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, int fd; int ret = -1; struct stat st; + const char *uniqueName; virStorageSourcePtr backingStore = NULL; int backingFormat;
- VIR_DEBUG("path=%s canonPath=%s dir=%s format=%d uid=%d gid=%d probe=%d", - src->path, canonPath, NULLSTR(src->relDir), src->format, + VIR_DEBUG("path=%s dir=%s format=%d uid=%d gid=%d probe=%d", + src->path, NULLSTR(src->relDir), src->format, (int)uid, (int)gid, allow_probe);
- if (virHashLookup(cycle, canonPath)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("backing store for %s is self-referential"), - src->path); + /* exit if we can't load information about the current image */ + if (!virStorageFileSupportsBackingChainTraversal(src)) + return 0;
After this change I get virstrogetest failing on FreeBSD, which doesn't support any of the file storage backends currently: Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 806c06400 (LWP 100061/virstoragetest)] 0x0000000000410088 in mymain () at virstoragetest.c:937 937 TEST_LOOKUP(3, "qcow2", chain->backingStore->path, chain->backingStore, Current language: auto; currently minimal (gdb) p chain $1 = 0x806c7b100 (gdb) p chain->backingStore $2 = 0x0 (gdb)
+ + if (virStorageFileInitAs(src, uid, gid) < 0) return -1; + + if (!(uniqueName = virStorageFileGetUniqueIdentifier(src))) + goto cleanup; + + if (virHashLookup(cycle, uniqueName)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("backing store for %s (%s) is self-referential"), + src->path, uniqueName); + goto cleanup; }
- if (virHashAddEntry(cycle, canonPath, (void *)1) < 0) - return -1; + if (virHashAddEntry(cycle, uniqueName, (void *)1) < 0) + goto cleanup;
if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) { if ((fd = virFileOpenAs(src->path, O_RDONLY, 0, uid, gid, 0)) < 0) { virReportSystemError(-fd, _("Failed to open file '%s'"), src->path); - return -1; + goto cleanup; }
if (virStorageFileGetMetadataFromFDInternal(src, fd, &backingFormat) < 0) { VIR_FORCE_CLOSE(fd); - return -1; + goto cleanup; }
if (VIR_CLOSE(fd) < 0) VIR_WARN("could not close file %s", src->path); } else { /* TODO: currently we call this only for local storage */ - return 0; + ret = 0; + goto cleanup; }
/* check whether we need to go deeper */ - if (!src->backingStoreRaw) - return 0; + if (!src->backingStoreRaw) { + ret = 0; + goto cleanup; + }
if (VIR_ALLOC(backingStore) < 0) - return -1; + goto cleanup;
if (VIR_STRDUP(backingStore->relPath, src->backingStoreRaw) < 0) goto cleanup; @@ -3201,7 +3238,6 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, backingStore->format = backingFormat;
if (virStorageFileGetMetadataRecurse(backingStore, - backingStore->path, uid, gid, allow_probe, cycle) < 0) { /* if we fail somewhere midway, just accept and return a @@ -3215,6 +3251,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, ret = 0;
cleanup: + virStorageFileDeinit(src); virStorageSourceFree(backingStore); return ret; } @@ -3253,12 +3290,6 @@ virStorageFileGetMetadata(virStorageSourcePtr src, return -1;
if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) { - if (!(canonPath = canonicalize_file_name(src->path))) { - virReportSystemError(errno, _("unable to resolve '%s'"), - src->path); - goto cleanup; - } - if (!src->relPath && VIR_STRDUP(src->relPath, src->path) < 0) goto cleanup; @@ -3277,7 +3308,7 @@ virStorageFileGetMetadata(virStorageSourcePtr src, if (src->format <= VIR_STORAGE_FILE_NONE) src->format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW;
- ret = virStorageFileGetMetadataRecurse(src, canonPath, uid, gid, + ret = virStorageFileGetMetadataRecurse(src, uid, gid, allow_probe, cycle);
cleanup: -- 1.9.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Roman Bogorodskiy
On 06/07/14 20:35, Roman Bogorodskiy wrote:
Peter Krempa wrote:
Use the virStorageFileGetUniqueIdentifier() function to get a unique identifier regardless of the target storage type instead of relying on canonicalize_path().
A new function that checks whether we support a given image is introduced to avoid errors for unimplemented backends. ---
Notes: Version 3: - fixed typo - ACKed by Eric
src/storage/storage_driver.c | 77 +++++++++++++++++++++++++++++++------------- 1 file changed, 54 insertions(+), 23 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index f92a553..5c4188f 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c
@@ -3112,49 +3135,63 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, int fd; int ret = -1; struct stat st; + const char *uniqueName; virStorageSourcePtr backingStore = NULL; int backingFormat;
- VIR_DEBUG("path=%s canonPath=%s dir=%s format=%d uid=%d gid=%d probe=%d", - src->path, canonPath, NULLSTR(src->relDir), src->format, + VIR_DEBUG("path=%s dir=%s format=%d uid=%d gid=%d probe=%d", + src->path, NULLSTR(src->relDir), src->format, (int)uid, (int)gid, allow_probe);
- if (virHashLookup(cycle, canonPath)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("backing store for %s is self-referential"), - src->path); + /* exit if we can't load information about the current image */ + if (!virStorageFileSupportsBackingChainTraversal(src)) + return 0;
After this change I get virstrogetest failing on FreeBSD, which doesn't support any of the file storage backends currently:
Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 806c06400 (LWP 100061/virstoragetest)] 0x0000000000410088 in mymain () at virstoragetest.c:937 937 TEST_LOOKUP(3, "qcow2", chain->backingStore->path, chain->backingStore, Current language: auto; currently minimal (gdb) p chain $1 = 0x806c7b100 (gdb) p chain->backingStore $2 = 0x0 (gdb)
Hmm, so FreeBSD doesn't currently compile the storage driver? We should probably look into enabling it. All the stuff that was done by the code was compiling just fine on FreeBSD previously so enabling just the local filesystem storage backend should work well. I'll have a look what that would include. Peter
Peter Krempa wrote:
On 06/07/14 20:35, Roman Bogorodskiy wrote:
Peter Krempa wrote:
Use the virStorageFileGetUniqueIdentifier() function to get a unique identifier regardless of the target storage type instead of relying on canonicalize_path().
A new function that checks whether we support a given image is introduced to avoid errors for unimplemented backends. ---
Notes: Version 3: - fixed typo - ACKed by Eric
src/storage/storage_driver.c | 77 +++++++++++++++++++++++++++++++------------- 1 file changed, 54 insertions(+), 23 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index f92a553..5c4188f 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c
@@ -3112,49 +3135,63 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, int fd; int ret = -1; struct stat st; + const char *uniqueName; virStorageSourcePtr backingStore = NULL; int backingFormat;
- VIR_DEBUG("path=%s canonPath=%s dir=%s format=%d uid=%d gid=%d probe=%d", - src->path, canonPath, NULLSTR(src->relDir), src->format, + VIR_DEBUG("path=%s dir=%s format=%d uid=%d gid=%d probe=%d", + src->path, NULLSTR(src->relDir), src->format, (int)uid, (int)gid, allow_probe);
- if (virHashLookup(cycle, canonPath)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("backing store for %s is self-referential"), - src->path); + /* exit if we can't load information about the current image */ + if (!virStorageFileSupportsBackingChainTraversal(src)) + return 0;
After this change I get virstrogetest failing on FreeBSD, which doesn't support any of the file storage backends currently:
Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 806c06400 (LWP 100061/virstoragetest)] 0x0000000000410088 in mymain () at virstoragetest.c:937 937 TEST_LOOKUP(3, "qcow2", chain->backingStore->path, chain->backingStore, Current language: auto; currently minimal (gdb) p chain $1 = 0x806c7b100 (gdb) p chain->backingStore $2 = 0x0 (gdb)
Hmm, so FreeBSD doesn't currently compile the storage driver? We should probably look into enabling it. All the stuff that was done by the code was compiling just fine on FreeBSD previously so enabling just the local filesystem storage backend should work well. I'll have a look what that would include.
virstorage.c test calls testStorageFileGetMetadata() which calls virStorageFileGetMetadata(). Inside of that, the call sequence is: virStorageFileGetMetadata -> virStorageFileGetMetadataRecurse virStorageFileGetMetadataRecurse then checks !virStorageFileSupportsBackingChainTraversal() and returns 0. virStorageFileSupportsBackingChainTraversal tries to initialise backend using virStorageFileBackendForTypeInternal(). virStorageFileBackendForTypeInternal loops through fileBackends which looks this way: static virStorageFileBackendPtr fileBackends[] = { #if WITH_STORAGE_FS &virStorageFileBackendFile, &virStorageFileBackendBlock, #endif #if WITH_STORAGE_GLUSTER &virStorageFileBackendGluster, #endif NULL }; FreeBSD doesn't support neither WITH_STORAGE_GLUSTER nor WITH_STORAGE_FS. So we end up having chain->backingStore == NULL. PS I may be wrong here as it's my first experience with the storage code and I haven't yet spend much time on it. Roman Bogorodskiy
On 06/09/14 16:07, Roman Bogorodskiy wrote:
Peter Krempa wrote:
On 06/07/14 20:35, Roman Bogorodskiy wrote:
Peter Krempa wrote:
Use the virStorageFileGetUniqueIdentifier() function to get a unique identifier regardless of the target storage type instead of relying on canonicalize_path().
A new function that checks whether we support a given image is introduced to avoid errors for unimplemented backends. ---
Notes: Version 3: - fixed typo - ACKed by Eric
src/storage/storage_driver.c | 77 +++++++++++++++++++++++++++++++------------- 1 file changed, 54 insertions(+), 23 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index f92a553..5c4188f 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c
@@ -3112,49 +3135,63 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, int fd; int ret = -1; struct stat st; + const char *uniqueName; virStorageSourcePtr backingStore = NULL; int backingFormat;
- VIR_DEBUG("path=%s canonPath=%s dir=%s format=%d uid=%d gid=%d probe=%d", - src->path, canonPath, NULLSTR(src->relDir), src->format, + VIR_DEBUG("path=%s dir=%s format=%d uid=%d gid=%d probe=%d", + src->path, NULLSTR(src->relDir), src->format, (int)uid, (int)gid, allow_probe);
- if (virHashLookup(cycle, canonPath)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("backing store for %s is self-referential"), - src->path); + /* exit if we can't load information about the current image */ + if (!virStorageFileSupportsBackingChainTraversal(src)) + return 0;
After this change I get virstrogetest failing on FreeBSD, which doesn't support any of the file storage backends currently:
Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 806c06400 (LWP 100061/virstoragetest)] 0x0000000000410088 in mymain () at virstoragetest.c:937 937 TEST_LOOKUP(3, "qcow2", chain->backingStore->path, chain->backingStore, Current language: auto; currently minimal (gdb) p chain $1 = 0x806c7b100 (gdb) p chain->backingStore $2 = 0x0 (gdb)
Hmm, so FreeBSD doesn't currently compile the storage driver? We should probably look into enabling it. All the stuff that was done by the code was compiling just fine on FreeBSD previously so enabling just the local filesystem storage backend should work well. I'll have a look what that would include.
virstorage.c test calls testStorageFileGetMetadata() which calls virStorageFileGetMetadata().
Inside of that, the call sequence is:
virStorageFileGetMetadata -> virStorageFileGetMetadataRecurse
virStorageFileGetMetadataRecurse then checks !virStorageFileSupportsBackingChainTraversal() and returns 0.
virStorageFileSupportsBackingChainTraversal tries to initialise backend using virStorageFileBackendForTypeInternal().
virStorageFileBackendForTypeInternal loops through fileBackends which looks this way:
static virStorageFileBackendPtr fileBackends[] = { #if WITH_STORAGE_FS &virStorageFileBackendFile, &virStorageFileBackendBlock, #endif #if WITH_STORAGE_GLUSTER &virStorageFileBackendGluster, #endif NULL };
FreeBSD doesn't support neither WITH_STORAGE_GLUSTER nor WITH_STORAGE_FS. So we end up having chain->backingStore == NULL.
PS I may be wrong here as it's my first experience with the storage code and I haven't yet spend much time on it.
Yes, the problem is that VIR_STORAGE_FS isn't enabled on freebsd. I don't know whether it's worth analyzing the code to find out if it can be enabled on freebsd or not. (the needed portions certainly can be enabled, but some of the other portions might be problematic. At any rate, I'm working on a refactor of the test that will allow to skip the test (and remove a few ugly pieces of code)
Roman Bogorodskiy
Peter
Peter Krempa wrote:
On 06/09/14 16:07, Roman Bogorodskiy wrote:
Peter Krempa wrote:
On 06/07/14 20:35, Roman Bogorodskiy wrote:
Peter Krempa wrote:
Use the virStorageFileGetUniqueIdentifier() function to get a unique identifier regardless of the target storage type instead of relying on canonicalize_path().
A new function that checks whether we support a given image is introduced to avoid errors for unimplemented backends. ---
Notes: Version 3: - fixed typo - ACKed by Eric
src/storage/storage_driver.c | 77 +++++++++++++++++++++++++++++++------------- 1 file changed, 54 insertions(+), 23 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index f92a553..5c4188f 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c
@@ -3112,49 +3135,63 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, int fd; int ret = -1; struct stat st; + const char *uniqueName; virStorageSourcePtr backingStore = NULL; int backingFormat;
- VIR_DEBUG("path=%s canonPath=%s dir=%s format=%d uid=%d gid=%d probe=%d", - src->path, canonPath, NULLSTR(src->relDir), src->format, + VIR_DEBUG("path=%s dir=%s format=%d uid=%d gid=%d probe=%d", + src->path, NULLSTR(src->relDir), src->format, (int)uid, (int)gid, allow_probe);
- if (virHashLookup(cycle, canonPath)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("backing store for %s is self-referential"), - src->path); + /* exit if we can't load information about the current image */ + if (!virStorageFileSupportsBackingChainTraversal(src)) + return 0;
After this change I get virstrogetest failing on FreeBSD, which doesn't support any of the file storage backends currently:
Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 806c06400 (LWP 100061/virstoragetest)] 0x0000000000410088 in mymain () at virstoragetest.c:937 937 TEST_LOOKUP(3, "qcow2", chain->backingStore->path, chain->backingStore, Current language: auto; currently minimal (gdb) p chain $1 = 0x806c7b100 (gdb) p chain->backingStore $2 = 0x0 (gdb)
Hmm, so FreeBSD doesn't currently compile the storage driver? We should probably look into enabling it. All the stuff that was done by the code was compiling just fine on FreeBSD previously so enabling just the local filesystem storage backend should work well. I'll have a look what that would include.
virstorage.c test calls testStorageFileGetMetadata() which calls virStorageFileGetMetadata().
Inside of that, the call sequence is:
virStorageFileGetMetadata -> virStorageFileGetMetadataRecurse
virStorageFileGetMetadataRecurse then checks !virStorageFileSupportsBackingChainTraversal() and returns 0.
virStorageFileSupportsBackingChainTraversal tries to initialise backend using virStorageFileBackendForTypeInternal().
virStorageFileBackendForTypeInternal loops through fileBackends which looks this way:
static virStorageFileBackendPtr fileBackends[] = { #if WITH_STORAGE_FS &virStorageFileBackendFile, &virStorageFileBackendBlock, #endif #if WITH_STORAGE_GLUSTER &virStorageFileBackendGluster, #endif NULL };
FreeBSD doesn't support neither WITH_STORAGE_GLUSTER nor WITH_STORAGE_FS. So we end up having chain->backingStore == NULL.
PS I may be wrong here as it's my first experience with the storage code and I haven't yet spend much time on it.
Yes, the problem is that VIR_STORAGE_FS isn't enabled on freebsd.
I don't know whether it's worth analyzing the code to find out if it can be enabled on freebsd or not. (the needed portions certainly can be enabled, but some of the other portions might be problematic.
Providing a fully functional virStorageBackendFileSystem backend will certainly require some effort, at least getmntent_r() part and mouting part needs to be re-implemnented for BSD. I'll probably take a look later.
At any rate, I'm working on a refactor of the test that will allow to skip the test (and remove a few ugly pieces of code)
Thanks! Roman Bogorodskiy
Use virStorageFileReadHeader() to read headers of storage files possibly on remote storage to retrieve the image metadata. The backend information is now parsed by virStorageFileGetMetadataInternal which is now exported from the util source and virStorageFileGetMetadataFromFDInternal now doesn't need to be exported. --- Notes: Version 3: - fixed typo - ACKed src/libvirt_private.syms | 2 +- src/storage/storage_driver.c | 26 +++++++------------------- src/util/virstoragefile.c | 5 ++--- src/util/virstoragefile.h | 9 ++++++--- 4 files changed, 16 insertions(+), 26 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 91f13a4..57312c3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1865,7 +1865,7 @@ virStorageFileFormatTypeToString; virStorageFileGetLVMKey; virStorageFileGetMetadataFromBuf; virStorageFileGetMetadataFromFD; -virStorageFileGetMetadataFromFDInternal; +virStorageFileGetMetadataInternal; virStorageFileGetSCSIKey; virStorageFileIsClusterFS; virStorageFileParseChainIndex; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 5c4188f..b074047 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -3132,10 +3132,11 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, bool allow_probe, virHashTablePtr cycle) { - int fd; int ret = -1; struct stat st; const char *uniqueName; + char *buf = NULL; + ssize_t headerLen; virStorageSourcePtr backingStore = NULL; int backingFormat; @@ -3163,26 +3164,13 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, if (virHashAddEntry(cycle, uniqueName, (void *)1) < 0) goto cleanup; - if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) { - if ((fd = virFileOpenAs(src->path, O_RDONLY, 0, uid, gid, 0)) < 0) { - virReportSystemError(-fd, _("Failed to open file '%s'"), - src->path); - goto cleanup; - } - - if (virStorageFileGetMetadataFromFDInternal(src, fd, - &backingFormat) < 0) { - VIR_FORCE_CLOSE(fd); - goto cleanup; - } + if ((headerLen = virStorageFileReadHeader(src, VIR_STORAGE_MAX_HEADER, + &buf)) < 0) + goto cleanup; - if (VIR_CLOSE(fd) < 0) - VIR_WARN("could not close file %s", src->path); - } else { - /* TODO: currently we call this only for local storage */ - ret = 0; + if (virStorageFileGetMetadataInternal(src, buf, headerLen, + &backingFormat) < 0) goto cleanup; - } /* check whether we need to go deeper */ if (!src->backingStoreRaw) { diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 4956808..a80131a 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -740,8 +740,7 @@ qcow2GetFeatures(virBitmapPtr *features, * with information about the file and its backing store. Return format * of the backing store as BACKING_FORMAT. PATH and FORMAT have to be * pre-populated in META */ -static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) -ATTRIBUTE_NONNULL(4) +int virStorageFileGetMetadataInternal(virStorageSourcePtr meta, char *buf, size_t len, @@ -955,7 +954,7 @@ virStorageFileGetMetadataFromBuf(const char *path, /* Internal version that also supports a containing directory name. */ -int +static int virStorageFileGetMetadataFromFDInternal(virStorageSourcePtr meta, int fd, int *backingFormat) diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 9947203..b516fbe 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -265,9 +265,12 @@ struct _virStorageSource { int virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid); -int virStorageFileGetMetadataFromFDInternal(virStorageSourcePtr meta, - int fd, - int *backingFormat); +int virStorageFileGetMetadataInternal(virStorageSourcePtr meta, + char *buf, + size_t len, + int *backingFormat) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4); + virStorageSourcePtr virStorageFileGetMetadataFromFD(const char *path, int fd, int format, -- 1.9.3
Use virStorageFileAccess() to to check whether the file is accessible in the main part of the metadata crawler. --- Notes: Version 3: - fixed typo - ACKed src/storage/storage_driver.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index b074047..787171d 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -3103,13 +3103,6 @@ virFindBackingFile(const char *start, const char *path, goto cleanup; } - if (virFileAccessibleAs(combined, F_OK, geteuid(), getegid()) < 0) { - virReportSystemError(errno, - _("Cannot access backing file '%s'"), - combined); - goto cleanup; - } - if (!(*canonical = canonicalize_file_name(combined))) { virReportSystemError(errno, _("Can't canonicalize path '%s'"), path); @@ -3151,6 +3144,13 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, if (virStorageFileInitAs(src, uid, gid) < 0) return -1; + if (virStorageFileAccess(src, F_OK) < 0) { + virReportSystemError(errno, + _("Cannot access backing file %s"), + src->path); + goto cleanup; + } + if (!(uniqueName = virStorageFileGetUniqueIdentifier(src))) goto cleanup; -- 1.9.3
Add parsers for relative and absolute backing names for local and remote storage files. This parser parses relative paths as relative to their parents and absolute paths according to the protocol or local access. For remote storage volumes, all URI based backing file names are supported and for the qemu colon syntax the NBD protocol is supported. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 364 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 4 + 3 files changed, 369 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 57312c3..e1702db 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1883,6 +1883,7 @@ virStorageSourceClear; virStorageSourceClearBackingStore; virStorageSourceFree; virStorageSourceGetActualType; +virStorageSourceNewFromBacking; virStorageSourcePoolDefFree; virStorageSourcePoolModeTypeFromString; virStorageSourcePoolModeTypeToString; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index a80131a..43b7a5a 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -38,6 +38,8 @@ #include "virendian.h" #include "virstring.h" #include "virutil.h" +#include "viruri.h" +#include "dirname.h" #if HAVE_SYS_SYSCALL_H # include <sys/syscall.h> #endif @@ -660,6 +662,19 @@ virStorageIsFile(const char *backing) } +static bool +virStorageIsRelative(const char *backing) +{ + if (backing[0] == '/') + return false; + + if (!virStorageIsFile(backing)) + return false; + + return true; +} + + static int virStorageFileProbeFormatFromBuf(const char *path, char *buf, @@ -1563,3 +1578,352 @@ virStorageSourceFree(virStorageSourcePtr def) virStorageSourceClear(def); VIR_FREE(def); } + + +static virStorageSourcePtr +virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent, + const char *rel) +{ + char *dirname = NULL; + const char *parentdir = ""; + virStorageSourcePtr ret; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + ret->backingRelative = true; + + /* XXX Once we get rid of the need to use canonical names in path, we will be + * able to use mdir_name on parent->path instead of using parent->relDir */ + if (STRNEQ(parent->relDir, "/")) + parentdir = parent->relDir; + + if (virAsprintf(&ret->path, "%s/%s", parentdir, rel) < 0) + goto error; + + if (virStorageSourceGetActualType(parent) != VIR_STORAGE_TYPE_NETWORK) { + ret->type = VIR_STORAGE_TYPE_FILE; + + /* XXX store the relative directory name for test's sake */ + if (!(ret->relDir = mdir_name(ret->path))) { + virReportOOMError(); + goto error; + } + + /* XXX we don't currently need to store the canonical path but the + * change would break the test suite. Get rid of this later */ + char *tmp = ret->path; + if (!(ret->path = canonicalize_file_name(tmp))) { + ret->path = tmp; + tmp = NULL; + } + VIR_FREE(tmp); + } else { + ret->type = VIR_STORAGE_TYPE_NETWORK; + + /* copy the host network part */ + ret->protocol = parent->protocol; + if (!(ret->hosts = virStorageNetHostDefCopy(parent->nhosts, parent->hosts))) + goto error; + ret->nhosts = parent->nhosts; + + if (VIR_STRDUP(ret->volume, parent->volume) < 0) + goto error; + + /* XXX store the relative directory name for test's sake */ + if (!(ret->relDir = mdir_name(ret->path))) { + virReportOOMError(); + goto error; + } + } + + cleanup: + VIR_FREE(dirname); + return ret; + + error: + virStorageSourceFree(ret); + ret = NULL; + goto cleanup; +} + + +static int +virStorageSourceParseBackingURI(virStorageSourcePtr src, + const char *path) +{ + virURIPtr uri = NULL; + char **scheme = NULL; + int ret = -1; + + if (!(uri = virURIParse(path))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to parse backing file location '%s'"), + path); + goto cleanup; + } + + if (!(scheme = virStringSplit(uri->scheme, "+", 2))) + goto cleanup; + + if (!scheme[0] || + (src->protocol = virStorageNetProtocolTypeFromString(scheme[0])) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid backing protocol '%s'"), + NULLSTR(scheme[0])); + goto cleanup; + } + + if (scheme[1] && + (src->hosts->transport = virStorageNetHostTransportTypeFromString(scheme[1])) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid protocol transport type '%s'"), + scheme[1]); + goto cleanup; + } + + /* handle socket stored as a query */ + if (uri->query) { + if (VIR_STRDUP(src->hosts->socket, STRSKIP(uri->query, "socket=")) < 0) + goto cleanup; + } + + /* XXX We currently don't support auth, so don't bother parsing it */ + + /* possibly skip the leading slash */ + if (VIR_STRDUP(src->path, + *uri->path == '/' ? uri->path + 1 : uri->path) < 0) + goto cleanup; + + if (src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER) { + char *tmp; + if (!(tmp = strchr(src->path, '/')) || + tmp == src->path) { + virReportError(VIR_ERR_XML_ERROR, + _("missing volume name or file name in " + "gluster source path '%s'"), src->path); + goto cleanup; + } + + src->volume = src->path; + + if (VIR_STRDUP(src->path, tmp) < 0) + goto cleanup; + + tmp[0] = '\0'; + } + + if (VIR_ALLOC(src->hosts) < 0) + goto cleanup; + + src->nhosts = 1; + + if (uri->port > 0) { + if (virAsprintf(&src->hosts->port, "%d", uri->port) < 0) + goto cleanup; + } + + if (VIR_STRDUP(src->hosts->name, uri->server) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virURIFree(uri); + virStringFreeList(scheme); + return ret; +} + + +static int +virStorageSourceParseBackingColon(virStorageSourcePtr src, + const char *path) +{ + char **backing = NULL; + int ret = -1; + + if (!(backing = virStringSplit(path, ":", 0))) + goto cleanup; + + if (!backing[0] || + (src->protocol = virStorageNetProtocolTypeFromString(backing[0])) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid backing protocol '%s'"), + NULLSTR(backing[0])); + goto cleanup; + } + + switch ((virStorageNetProtocol) src->protocol) { + case VIR_STORAGE_NET_PROTOCOL_NBD: + if (VIR_ALLOC_N(src->hosts, 1) < 0) + goto cleanup; + src->nhosts = 1; + src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP; + + /* format: [] denotes optional sections, uppercase are variable strings + * nbd:unix:/PATH/TO/SOCKET[:exportname=EXPORTNAME] + * nbd:HOSTNAME:PORT[:exportname=EXPORTNAME] + */ + if (!backing[1]) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing remote information in '%s' for protocol nbd"), + path); + goto cleanup; + } else if (STREQ(backing[1], "unix")) { + if (!backing[2]) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing unix socket path in nbd backing string %s"), + path); + goto cleanup; + } + + if (VIR_STRDUP(src->hosts->socket, backing[2]) < 0) + goto cleanup; + + } else { + if (!backing[1]) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing host name in nbd string '%s'"), + path); + goto cleanup; + } + + if (VIR_STRDUP(src->hosts->name, backing[1]) < 0) + goto cleanup; + + if (!backing[2]) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing port in nbd string '%s'"), + path); + goto cleanup; + } + + if (VIR_STRDUP(src->hosts->port, backing[2]) < 0) + goto cleanup; + } + + if (backing[3] && STRPREFIX(backing[3], "exportname=")) { + if (VIR_STRDUP(src->path, backing[3] + strlen("exportname=")) < 0) + goto cleanup; + } + break; + + case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: + case VIR_STORAGE_NET_PROTOCOL_RBD: + case VIR_STORAGE_NET_PROTOCOL_LAST: + case VIR_STORAGE_NET_PROTOCOL_NONE: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("backing store parser is not implemented for protocol %s"), + backing[0]); + goto cleanup; + + case VIR_STORAGE_NET_PROTOCOL_HTTP: + case VIR_STORAGE_NET_PROTOCOL_HTTPS: + case VIR_STORAGE_NET_PROTOCOL_FTP: + case VIR_STORAGE_NET_PROTOCOL_FTPS: + case VIR_STORAGE_NET_PROTOCOL_TFTP: + case VIR_STORAGE_NET_PROTOCOL_ISCSI: + case VIR_STORAGE_NET_PROTOCOL_GLUSTER: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("malformed backing store path for protocol %s"), + backing[0]); + goto cleanup; + } + + ret = 0; + + cleanup: + virStringFreeList(backing); + return ret; + +} + + +static virStorageSourcePtr +virStorageSourceNewFromBackingAbsolute(const char *path) +{ + virStorageSourcePtr ret; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + if (virStorageIsFile(path)) { + ret->type = VIR_STORAGE_TYPE_FILE; + + /* XXX store the relative directory name for test's sake */ + if (!(ret->relDir = mdir_name(path))) { + virReportOOMError(); + goto error; + } + + /* XXX we don't currently need to store the canonical path but the + * change would break the test suite. Get rid of this later */ + if (!(ret->path = canonicalize_file_name(path))) { + if (VIR_STRDUP(ret->path, path) < 0) + goto error; + } + } else { + ret->type = VIR_STORAGE_TYPE_NETWORK; + + /* handle URI formatted backing stores */ + if (strstr(path, "://")) { + if (virStorageSourceParseBackingURI(ret, path) < 0) + goto error; + } else { + if (virStorageSourceParseBackingColon(ret, path) < 0) + goto error; + } + + /* XXX fill relative path so that relative names work with network storage too */ + if (ret->path) { + if (!(ret->relDir = mdir_name(ret->path))) { + virReportOOMError(); + goto error; + } + } else { + if (VIR_STRDUP(ret->relDir, "") < 0) + goto error; + } + } + + return ret; + + error: + virStorageSourceFree(ret); + return NULL; +} + + +virStorageSourcePtr +virStorageSourceNewFromBacking(virStorageSourcePtr parent) +{ + struct stat st; + virStorageSourcePtr ret; + + if (virStorageIsRelative(parent->backingStoreRaw)) + ret = virStorageSourceNewFromBackingRelative(parent, + parent->backingStoreRaw); + else + ret = virStorageSourceNewFromBackingAbsolute(parent->backingStoreRaw); + + if (ret) { + if (VIR_STRDUP(ret->relPath, parent->backingStoreRaw) < 0) { + virStorageSourceFree(ret); + return NULL; + } + + /* possibly update local type */ + if (ret->type == VIR_STORAGE_TYPE_FILE) { + if (stat(ret->path, &st) == 0) { + if (S_ISDIR(st.st_mode)) { + ret->type = VIR_STORAGE_TYPE_DIR; + ret->format = VIR_STORAGE_FILE_DIR; + } else if (S_ISBLK(st.st_mode)) { + ret->type = VIR_STORAGE_TYPE_BLOCK; + } + } + } + } + + return ret; +} diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index b516fbe..34b3625 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -256,6 +256,8 @@ struct _virStorageSource { /* Name of the child backing store recorded in metadata of the * current file. */ char *backingStoreRaw; + /* is backing store identified as relative */ + bool backingRelative; }; @@ -321,5 +323,7 @@ void virStorageSourceClear(virStorageSourcePtr def); int virStorageSourceGetActualType(virStorageSourcePtr def); void virStorageSourceFree(virStorageSourcePtr def); void virStorageSourceClearBackingStore(virStorageSourcePtr def); +virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent); + #endif /* __VIR_STORAGE_FILE_H__ */ -- 1.9.3
On 05/30/2014 02:37 AM, Peter Krempa wrote:
Add parsers for relative and absolute backing names for local and remote storage files.
This parser parses relative paths as relative to their parents and absolute paths according to the protocol or local access.
For remote storage volumes, all URI based backing file names are supported and for the qemu colon syntax the NBD protocol is supported. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 364 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 4 + 3 files changed, 369 insertions(+)
Big patch, but no impact until later patches use it. Looks reasonable, so ACK (again, this series is post-release). Lots of FIXMEs, so I assume later patches in the series clean that up. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Use the new backing store parser in the backing chain crawler. This change needs one test change where information about the NBD image are now parsed differently. --- src/storage/storage_driver.c | 90 +------------------------------------------- tests/virstoragetest.c | 14 ++++--- 2 files changed, 9 insertions(+), 95 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 787171d..4d96070 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -3068,56 +3068,6 @@ virStorageFileAccess(virStorageSourcePtr src, } -/** - * Given a starting point START (a directory containing the original - * file, if the original file was opened via a relative path; ignored - * if NAME is absolute), determine the location of the backing file - * NAME (possibly relative), and compute the relative DIRECTORY - * (optional) and CANONICAL (mandatory) location of the backing file. - * Return 0 on success, negative on error. - */ -static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4) -virFindBackingFile(const char *start, const char *path, - char **directory, char **canonical) -{ - /* FIXME - when we eventually allow non-raw network devices, we - * must ensure that we handle backing files the same way as qemu. - * For a qcow2 top file of gluster://server/vol/img, qemu treats - * the relative backing file 'rel' as meaning - * 'gluster://server/vol/rel', while the backing file '/abs' is - * used as a local file. But we cannot canonicalize network - * devices via canonicalize_file_name(), because they are not part - * of the local file system. */ - char *combined = NULL; - int ret = -1; - - if (*path == '/') { - /* Safe to cast away const */ - combined = (char *)path; - } else if (virAsprintf(&combined, "%s/%s", start, path) < 0) { - goto cleanup; - } - - if (directory && !(*directory = mdir_name(combined))) { - virReportOOMError(); - goto cleanup; - } - - if (!(*canonical = canonicalize_file_name(combined))) { - virReportSystemError(errno, - _("Can't canonicalize path '%s'"), path); - goto cleanup; - } - - ret = 0; - - cleanup: - if (combined != path) - VIR_FREE(combined); - return ret; -} - - /* Recursive workhorse for virStorageFileGetMetadata. */ static int virStorageFileGetMetadataRecurse(virStorageSourcePtr src, @@ -3126,7 +3076,6 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, virHashTablePtr cycle) { int ret = -1; - struct stat st; const char *uniqueName; char *buf = NULL; ssize_t headerLen; @@ -3178,46 +3127,9 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, goto cleanup; } - if (VIR_ALLOC(backingStore) < 0) + if (!(backingStore = virStorageSourceNewFromBacking(src))) goto cleanup; - if (VIR_STRDUP(backingStore->relPath, src->backingStoreRaw) < 0) - goto cleanup; - - if (virStorageIsFile(src->backingStoreRaw)) { - backingStore->type = VIR_STORAGE_TYPE_FILE; - - if (virFindBackingFile(src->relDir, - src->backingStoreRaw, - &backingStore->relDir, - &backingStore->path) < 0) { - /* the backing file is (currently) unavailable, treat this - * file as standalone: - * backingStoreRaw is kept to mark broken image chains */ - VIR_WARN("Backing file '%s' of image '%s' is missing.", - src->backingStoreRaw, src->path); - ret = 0; - goto cleanup; - } - - /* update the type for local storage */ - if (stat(backingStore->path, &st) == 0) { - if (S_ISDIR(st.st_mode)) { - backingStore->type = VIR_STORAGE_TYPE_DIR; - backingStore->format = VIR_STORAGE_FILE_DIR; - } else if (S_ISBLK(st.st_mode)) { - backingStore->type = VIR_STORAGE_TYPE_BLOCK; - } - } - } else { - /* TODO: To satisfy the test case, copy the network URI as path. This - * will be removed later. */ - backingStore->type = VIR_STORAGE_TYPE_NETWORK; - - if (VIR_STRDUP(backingStore->path, src->backingStoreRaw) < 0) - goto cleanup; - } - if (backingFormat == VIR_STORAGE_FILE_AUTO && !allow_probe) backingStore->format = VIR_STORAGE_FILE_RAW; else if (backingFormat == VIR_STORAGE_FILE_AUTO_SAFE) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 746bf63..29297ef 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -730,20 +730,22 @@ mymain(void) /* 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", + "-F", "raw", "-b", "nbd:example.org:6000:exportname=blah", "qcow2", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; - qcow2.expBackingStore = "nbd:example.org:6000"; - qcow2.expBackingStoreRaw = "nbd:example.org:6000"; + qcow2.expBackingStore = "blah"; + qcow2.expBackingStoreRaw = "nbd:example.org:6000:exportname=blah"; /* Qcow2 file with backing protocol instead of file */ testFileData nbd = { - .pathRel = "nbd:example.org:6000", - .pathAbs = "nbd:example.org:6000", - .path = "nbd:example.org:6000", + .pathRel = "nbd:example.org:6000:exportname=blah", + .pathAbs = "nbd:example.org:6000:exportname=blah", + .path = "blah", .type = VIR_STORAGE_TYPE_NETWORK, .format = VIR_STORAGE_FILE_RAW, + .relDirRel = ".", + .relDirAbs = ".", }; TEST_CHAIN(11, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, (&qcow2, &nbd), EXP_PASS, -- 1.9.3
On 05/30/2014 02:37 AM, Peter Krempa wrote:
Use the new backing store parser in the backing chain crawler. This change needs one test change where information about the NBD image are now parsed differently. --- src/storage/storage_driver.c | 90 +------------------------------------------- tests/virstoragetest.c | 14 ++++--- 2 files changed, 9 insertions(+), 95 deletions(-)
Nice diffstat!
+++ b/tests/virstoragetest.c @@ -730,20 +730,22 @@ mymain(void) /* 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", + "-F", "raw", "-b", "nbd:example.org:6000:exportname=blah",
Nice increased test coverage. I'm glad I spent the time on the testsuite, it makes changes like this more reassuring. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Now we don't need to skip backing chain detection for remote disks. --- src/qemu/qemu_domain.c | 8 +++----- src/storage/storage_driver.c | 18 ++++++------------ 2 files changed, 9 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 502b7bd..1d6f7be 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2428,12 +2428,10 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, int ret = 0; uid_t uid; gid_t gid; - const char *src = virDomainDiskGetSource(disk); - int type = virDomainDiskGetType(disk); + int type = virStorageSourceGetActualType(&disk->src); - if (!src || - type == VIR_STORAGE_TYPE_NETWORK || - type == VIR_STORAGE_TYPE_VOLUME) + if (type != VIR_STORAGE_TYPE_NETWORK && + !disk->src.path) goto cleanup; if (disk->src.backingStore) { diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 4d96070..4f51517 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -3189,19 +3189,13 @@ virStorageFileGetMetadata(virStorageSourcePtr src, if (!(cycle = virHashCreate(5, NULL))) return -1; - if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) { - if (!src->relPath && - VIR_STRDUP(src->relPath, src->path) < 0) - goto cleanup; + if (!src->relPath && + VIR_STRDUP(src->relPath, src->path) < 0) + goto cleanup; - if (!src->relDir && - !(src->relDir = mdir_name(src->path))) { - virReportOOMError(); - goto cleanup; - } - } else { - /* TODO: currently unimplemented for non-local storage */ - ret = 0; + if (!src->relDir && + !(src->relDir = mdir_name(src->path))) { + virReportOOMError(); goto cleanup; } -- 1.9.3
On 05/30/2014 02:37 AM, Peter Krempa wrote:
Now we don't need to skip backing chain detection for remote disks. --- src/qemu/qemu_domain.c | 8 +++----- src/storage/storage_driver.c | 18 ++++++------------ 2 files changed, 9 insertions(+), 17 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
To allow using the array manipulation macros on the arrays returned by virStringSplit we need to know the count of the elements in the array. Modify virStringSplit to return this value, rename it and add a helper with the old name so that we don't need to update all the code. --- Notes: Version 3: - added test coverage src/libvirt_private.syms | 1 + src/util/virstring.c | 24 ++++++++++++++++++++---- src/util/virstring.h | 6 ++++++ tests/virstringtest.c | 14 +++++++++++++- 4 files changed, 40 insertions(+), 5 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e1702db..4c1f61c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1908,6 +1908,7 @@ virStringSearch; virStringSortCompare; virStringSortRevCompare; virStringSplit; +virStringSplitCount; virStrncpy; virStrndup; virStrToDouble; diff --git a/src/util/virstring.c b/src/util/virstring.c index 7a8430e..6dcc7a8 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -43,13 +43,15 @@ VIR_LOG_INIT("util.string"); */ /** - * virStringSplit: + * virStringSplitCount: * @string: a string to split * @delim: a string which specifies the places at which to split * the string. The delimiter is not included in any of the resulting * strings, unless @max_tokens is reached. * @max_tokens: the maximum number of pieces to split @string into. * If this is 0, the string is split completely. + * @tokcount: If provided, the value is set to the count of pieces the string + * was split to excluding the terminating NULL element. * * Splits a string into a maximum of @max_tokens pieces, using the given * @delim. If @max_tokens is reached, the remainder of @string is @@ -65,9 +67,11 @@ VIR_LOG_INIT("util.string"); * Return value: a newly-allocated NULL-terminated array of strings. Use * virStringFreeList() to free it. */ -char **virStringSplit(const char *string, - const char *delim, - size_t max_tokens) +char ** +virStringSplitCount(const char *string, + const char *delim, + size_t max_tokens, + size_t *tokcount) { char **tokens = NULL; size_t ntokens = 0; @@ -109,6 +113,9 @@ char **virStringSplit(const char *string, goto error; tokens[ntokens++] = NULL; + if (tokcount) + *tokcount = ntokens - 1; + return tokens; error: @@ -119,6 +126,15 @@ char **virStringSplit(const char *string, } +char ** +virStringSplit(const char *string, + const char *delim, + size_t max_tokens) +{ + return virStringSplitCount(string, delim, max_tokens, NULL); +} + + /** * virStringJoin: * @strings: a NULL-terminated array of strings to join diff --git a/src/util/virstring.h b/src/util/virstring.h index 1ed1046..0ab9d96 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -26,6 +26,12 @@ # include "internal.h" +char **virStringSplitCount(const char *string, + const char *delim, + size_t max_tokens, + size_t *tokcount) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + char **virStringSplit(const char *string, const char *delim, size_t max_tokens) diff --git a/tests/virstringtest.c b/tests/virstringtest.c index 1e330f9..291e505 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -53,11 +53,14 @@ static int testSplit(const void *args) { const struct testSplitData *data = args; char **got; + size_t ntokens; + size_t exptokens = 0; char **tmp1; const char **tmp2; int ret = -1; - if (!(got = virStringSplit(data->string, data->delim, data->max_tokens))) { + if (!(got = virStringSplitCount(data->string, data->delim, + data->max_tokens, &ntokens))) { VIR_DEBUG("Got no tokens at all"); return -1; } @@ -71,6 +74,7 @@ static int testSplit(const void *args) } tmp1++; tmp2++; + exptokens++; } if (*tmp1) { virFilePrintf(stderr, "Too many pieces returned\n"); @@ -81,6 +85,14 @@ static int testSplit(const void *args) goto cleanup; } + if (ntokens != exptokens) { + virFilePrintf(stderr, + "Returned token count (%zu) doesn't match " + "expected count (%zu)", + ntokens, exptokens); + goto cleanup; + } + ret = 0; cleanup: virStringFreeList(got); -- 1.9.3
On 05/30/2014 02:37 AM, Peter Krempa wrote:
To allow using the array manipulation macros on the arrays returned by virStringSplit we need to know the count of the elements in the array. Modify virStringSplit to return this value, rename it and add a helper with the old name so that we don't need to update all the code. ---
Notes: Version 3: - added test coverage
Thanks :)
src/libvirt_private.syms | 1 + src/util/virstring.c | 24 ++++++++++++++++++++---- src/util/virstring.h | 6 ++++++ tests/virstringtest.c | 14 +++++++++++++- 4 files changed, 40 insertions(+), 5 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/02/14 23:40, Eric Blake wrote:
On 05/30/2014 02:37 AM, Peter Krempa wrote:
To allow using the array manipulation macros on the arrays returned by virStringSplit we need to know the count of the elements in the array. Modify virStringSplit to return this value, rename it and add a helper with the old name so that we don't need to update all the code. ---
Notes: Version 3: - added test coverage
Thanks :)
src/libvirt_private.syms | 1 + src/util/virstring.c | 24 ++++++++++++++++++++---- src/util/virstring.h | 6 ++++++ tests/virstringtest.c | 14 +++++++++++++- 4 files changed, 40 insertions(+), 5 deletions(-)
ACK.
Thanks; I've pushed patches 1 through 13. Peter
Sometimes the length of the string list is known but the array isn't NULL terminated. Add helper to free the array in such cases. --- src/libvirt_private.syms | 1 + src/util/virstring.c | 20 ++++++++++++++++++++ src/util/virstring.h | 1 + 3 files changed, 22 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4c1f61c..4491262 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1901,6 +1901,7 @@ virStrcpy; virStrdup; virStringArrayHasString; virStringFreeList; +virStringFreeListCount; virStringJoin; virStringListLength; virStringReplace; diff --git a/src/util/virstring.c b/src/util/virstring.c index 6dcc7a8..35b99a5 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -187,6 +187,26 @@ void virStringFreeList(char **strings) } +/** + * virStringFreeListCount: + * @strings: array of strings to free + * @count: number of elements in the array + * + * Frees a string array of @count length. + */ +void +virStringFreeListCount(char **strings, + size_t count) +{ + size_t i; + + for (i = 0; i < count; i++) + VIR_FREE(strings[i]); + + VIR_FREE(strings); +} + + bool virStringArrayHasString(char **strings, const char *needle) { diff --git a/src/util/virstring.h b/src/util/virstring.h index 0ab9d96..42d68b5 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -42,6 +42,7 @@ char *virStringJoin(const char **strings, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); void virStringFreeList(char **strings); +void virStringFreeListCount(char **strings, size_t count); bool virStringArrayHasString(char **strings, const char *needle); -- 1.9.3
On 05/30/2014 02:37 AM, Peter Krempa wrote:
Sometimes the length of the string list is known but the array isn't NULL terminated. Add helper to free the array in such cases.
Can't this just be done with: array[count] = NULL; virStringFreeList(array); Furthermore, MOST of our allocation guarantees a NULL-terminated array (VIR_ALLOC_N for example). Where did you actually run into a situation where the tail wasn't already NULL?
--- src/libvirt_private.syms | 1 + src/util/virstring.c | 20 ++++++++++++++++++++ src/util/virstring.h | 1 + 3 files changed, 22 insertions(+)
I'm not quite sold on whether we need this. The code looks clean enough, but without seeing it used in context, I'm reserving judgment until later in the series. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
As gnulib's canonicalize_filename_mode isn't LGPL and relative snapshot resolution code will need to clean paths with relative path components this patch adds a libvirt's own implementation of that functionality and tests to make sure everything works well. --- src/util/virstoragefile.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 2 + tests/virstoragetest.c | 83 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 180 insertions(+) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 43b7a5a..a9c6a13 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -40,6 +40,7 @@ #include "virutil.h" #include "viruri.h" #include "dirname.h" +#include "virbuffer.h" #if HAVE_SYS_SYSCALL_H # include <sys/syscall.h> #endif @@ -1927,3 +1928,97 @@ virStorageSourceNewFromBacking(virStorageSourcePtr parent) return ret; } + + +static char * +virStorageFileExportPath(char **components, + size_t ncomponents, + bool beginSlash) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + size_t i; + char *ret = NULL; + + if (beginSlash) + virBufferAddLit(&buf, "/"); + + for (i = 0; i < ncomponents; i++) { + if (i != 0) + virBufferAddLit(&buf, "/"); + + virBufferAdd(&buf, components[i], -1); + } + + if (virBufferError(&buf) != 0) { + virReportOOMError(); + return NULL; + } + + /* if the output string is empty ... wel just return an empty string */ + if (!(ret = virBufferContentAndReset(&buf))) + ignore_value(VIR_STRDUP(ret, "")); + + return ret; +} + + +char * +virStorageFileSimplifyPath(const char *path, + bool allow_relative) +{ + bool beginSlash = false; + char **components = NULL; + size_t ncomponents = 0; + size_t i; + char *ret = NULL; + + /* special cases are "" and "//", return them as they are */ + if (STREQ(path, "") || STREQ(path, "//")) { + ignore_value(VIR_STRDUP(ret, path)); + goto cleanup; + } + + if (path[0] == '/') + beginSlash = true; + + if (!(components = virStringSplitCount(path, "/", 0, &ncomponents))) + goto cleanup; + + i = 0; + while (i < ncomponents) { + if (STREQ(components[i], "") || + STREQ(components[i], ".")) { + VIR_FREE(components[i]); + VIR_DELETE_ELEMENT(components, i, ncomponents); + continue; + } + + if (STREQ(components[i], "..")) { + if (allow_relative && !beginSlash && + (i == 0 || STREQ(components[i - 1], ".."))) { + i++; + continue; + } + + VIR_FREE(components[i]); + VIR_DELETE_ELEMENT(components, i, ncomponents); + + if (i != 0) { + VIR_FREE(components[i - 1]); + VIR_DELETE_ELEMENT(components, i - 1, ncomponents); + i--; + } + + continue; + } + + i++; + } + + ret = virStorageFileExportPath(components, ncomponents, beginSlash); + + cleanup: + virStringFreeListCount(components, ncomponents); + + return ret; +} diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 34b3625..3e9e5c8 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -325,5 +325,7 @@ void virStorageSourceFree(virStorageSourcePtr def); void virStorageSourceClearBackingStore(virStorageSourcePtr def); virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent); +char *virStorageFileSimplifyPath(const char *path, + bool allow_relative); #endif /* __VIR_STORAGE_FILE_H__ */ diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 29297ef..57f16ca 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -512,12 +512,61 @@ testStorageLookup(const void *args) return ret; } + +struct testPathSimplifyData +{ + const char *path; + const char *exp_abs; + const char *exp_rel; +}; + + +static int +testPathSimplify(const void *args) +{ + const struct testPathSimplifyData *data = args; + char *simple; + int ret = -1; + + if (!(simple = virStorageFileSimplifyPath(data->path, false))) { + fprintf(stderr, "path simplification returned NULL\n"); + goto cleanup; + } + + if (STRNEQ(simple, data->exp_abs)) { + fprintf(stderr, "simplify path (absolute) '%s': expected: '%s', got '%s'\n", + data->path, data->exp_abs, simple); + goto cleanup; + } + + VIR_FREE(simple); + + if (!(simple = virStorageFileSimplifyPath(data->path, true))) { + fprintf(stderr, "path simplification returned NULL\n"); + goto cleanup; + } + + if (STRNEQ(simple, data->exp_rel)) { + fprintf(stderr, "simplify path (relative) '%s': expected: '%s', got '%s'\n", + data->path, data->exp_rel, simple); + goto cleanup; + } + + ret = 0; + + cleanup: + VIR_FREE(simple); + return ret; +} + + static int mymain(void) { int ret; virCommandPtr cmd = NULL; struct testChainData data; + struct testPathSimplifyData data3; virStorageSourcePtr chain = NULL; /* Prep some files with qemu-img; if that is not found on PATH, or @@ -1016,6 +1065,40 @@ mymain(void) chain->backingStore->path); TEST_LOOKUP_TARGET(33, "vda", "vda[3]", 3, NULL, NULL, NULL); +#define TEST_SIMPLIFY(id, PATH, EXP_ABS, EXP_REL) \ + do { \ + data3.path = PATH; \ + data3.exp_abs = EXP_ABS; \ + data3.exp_rel = EXP_REL; \ + if (virtTestRun("Path simplify " #id, \ + testPathSimplify, &data3) < 0) \ + ret = -1; \ + } while (0) + + /* PATH, absolute simplification, relative simplification */ + TEST_SIMPLIFY(1, "/", "/", "/"); + TEST_SIMPLIFY(2, "/path", "/path", "/path"); + TEST_SIMPLIFY(3, "/path/to/blah", "/path/to/blah", "/path/to/blah"); + TEST_SIMPLIFY(4, "/path/", "/path", "/path"); + TEST_SIMPLIFY(5, "///////", "/", "/"); + TEST_SIMPLIFY(6, "//", "//", "//"); + TEST_SIMPLIFY(7, "", "", ""); + TEST_SIMPLIFY(8, "../", "", ".."); + TEST_SIMPLIFY(9, "../../", "", "../.."); + TEST_SIMPLIFY(10, "../../blah", "blah", "../../blah"); + TEST_SIMPLIFY(11, "/./././blah", "/blah", "/blah"); + TEST_SIMPLIFY(12, ".././../././../blah", "blah", "../../../blah"); + TEST_SIMPLIFY(13, "/././", "/", "/"); + TEST_SIMPLIFY(14, "./././", "", ""); + TEST_SIMPLIFY(15, "blah/../foo", "foo", "foo"); + TEST_SIMPLIFY(16, "foo/bar/../blah", "foo/blah", "foo/blah"); + TEST_SIMPLIFY(17, "foo/bar/.././blah", "foo/blah", "foo/blah"); + TEST_SIMPLIFY(18, "/path/to/foo/bar/../../../../../../../../baz", "/baz", "/baz"); + TEST_SIMPLIFY(19, "path/to/foo/bar/../../../../../../../../baz", "baz", "../../../../baz"); + TEST_SIMPLIFY(20, "path/to/foo/bar", "path/to/foo/bar", "path/to/foo/bar"); + TEST_SIMPLIFY(21, "some/path/to/image.qcow/../image2.qcow/../image3.qcow/", + "some/path/to/image3.qcow", "some/path/to/image3.qcow"); + cleanup: /* Final cleanup */ virStorageSourceFree(chain); -- 1.9.3
On 05/30/2014 02:37 AM, Peter Krempa wrote:
As gnulib's canonicalize_filename_mode isn't LGPL and relative snapshot resolution code will need to clean paths with relative path components
s/components/components,/
this patch adds a libvirt's own implementation of that functionality and
s/adds a/adds/
tests to make sure everything works well. --- src/util/virstoragefile.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 2 + tests/virstoragetest.c | 83 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 180 insertions(+)
+static char * +virStorageFileExportPath(char **components, + size_t ncomponents, + bool beginSlash) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + size_t i; + char *ret = NULL; + + if (beginSlash) + virBufferAddLit(&buf, "/"); + + for (i = 0; i < ncomponents; i++) { + if (i != 0) + virBufferAddLit(&buf, "/");
I find it slightly fewer lines of code to just blindly add a trailing '/' always...
+ + virBufferAdd(&buf, components[i], -1); + }
...then use virBufferTrim() to remove the last unneeded one. But that's aesthetic; what you have works.
+ + /* if the output string is empty ... wel just return an empty string */
Did you mean "well" or "we'll"? Either way, it still reads okay (and shorter) as: s/wel//
+virStorageFileSimplifyPath(const char *path, + bool allow_relative) +{ + bool beginSlash = false; + char **components = NULL; + size_t ncomponents = 0; + size_t i; + char *ret = NULL; + + /* special cases are "" and "//", return them as they are */ + if (STREQ(path, "") || STREQ(path, "//")) { + ignore_value(VIR_STRDUP(ret, path));
It's more than just "//" that is special; also special is anything with a leading double slash ("//foo" should not be simplified to "/foo", even though "/bar//foo" can be simplified. Other corner cases: "//foo//bar" should be simplified to "//foo/bar", "//../blah" can be simplified to "//blah"). Maybe this means checking if path[0]=='/' && path[1]=='/' && path[2]!='/'.
+++ b/tests/virstoragetest.c @@ -512,12 +512,61 @@ testStorageLookup(const void *args) return ret; }
+ +struct testPathSimplifyData +{ + const char *path; + const char *exp_abs; + const char *exp_rel; +}; +
Thanks; adding the test makes it more obvious what the code intends to do :)
static int mymain(void) { int ret; virCommandPtr cmd = NULL; struct testChainData data; + struct testPathSimplifyData data3;
data followed by data3 looks a bit odd :)
+ + /* PATH, absolute simplification, relative simplification */ + TEST_SIMPLIFY(1, "/", "/", "/"); + TEST_SIMPLIFY(2, "/path", "/path", "/path"); + TEST_SIMPLIFY(3, "/path/to/blah", "/path/to/blah", "/path/to/blah"); + TEST_SIMPLIFY(4, "/path/", "/path", "/path"); + TEST_SIMPLIFY(5, "///////", "/", "/"); + TEST_SIMPLIFY(6, "//", "//", "//"); + TEST_SIMPLIFY(7, "", "", ""); + TEST_SIMPLIFY(8, "../", "", ".."); + TEST_SIMPLIFY(9, "../../", "", "../.."); + TEST_SIMPLIFY(10, "../../blah", "blah", "../../blah"); + TEST_SIMPLIFY(11, "/./././blah", "/blah", "/blah"); + TEST_SIMPLIFY(12, ".././../././../blah", "blah", "../../../blah"); + TEST_SIMPLIFY(13, "/././", "/", "/"); + TEST_SIMPLIFY(14, "./././", "", "");
Turning "." into empty looks a bit odd (POSIX requires "" to fail while "." is the current directory). Not sure if that needs tweaking. Also, maybe it's worth a test of plain "." after test 7 (so that we are separating test of "." behavior from test 14's coverage of multiple "/." behavior).
+ TEST_SIMPLIFY(15, "blah/../foo", "foo", "foo");
This is not always true. It is wrong if blah/ is not a (symlink to a) directory; and if blah IS a symlink, it can still be wrong if it is a symlink to somewhere else in the hierarchy. While I'm in favor of simplifying /./ and a//b, I'm less certain about the benefits of reducing '..' without actually stat'ing the underlying filesystem.
+ TEST_SIMPLIFY(16, "foo/bar/../blah", "foo/blah", "foo/blah"); + TEST_SIMPLIFY(17, "foo/bar/.././blah", "foo/blah", "foo/blah"); + TEST_SIMPLIFY(18, "/path/to/foo/bar/../../../../../../../../baz", "/baz", "/baz"); + TEST_SIMPLIFY(19, "path/to/foo/bar/../../../../../../../../baz", "baz", "../../../../baz"); + TEST_SIMPLIFY(20, "path/to/foo/bar", "path/to/foo/bar", "path/to/foo/bar"); + TEST_SIMPLIFY(21, "some/path/to/image.qcow/../image2.qcow/../image3.qcow/", + "some/path/to/image3.qcow", "some/path/to/image3.qcow");
Yep, the more I look at this, the more I worry that simplifying '..' is wrong. :( -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
This patch introduces a function that will allow us to resolve a relative difference between two elements of a disk backing chain. This fucntion will be used to allow relative block commit and block pull where we need to specify the new relative name of the image to qemu. This patch also adds unit tests for the function to verify that it works correctly. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 45 +++++++++++++++++++++ src/util/virstoragefile.h | 4 ++ tests/virstoragetest.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 151 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4491262..8e00d8c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1866,6 +1866,7 @@ virStorageFileGetLVMKey; virStorageFileGetMetadataFromBuf; virStorageFileGetMetadataFromFD; virStorageFileGetMetadataInternal; +virStorageFileGetRelativeBackingPath; virStorageFileGetSCSIKey; virStorageFileIsClusterFS; virStorageFileParseChainIndex; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index a9c6a13..d8b4b54 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2022,3 +2022,48 @@ virStorageFileSimplifyPath(const char *path, return ret; } + + +int +virStorageFileGetRelativeBackingPath(virStorageSourcePtr from, + virStorageSourcePtr to, + char **relpath) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + virStorageSourcePtr next; + char *tmp = NULL; + char ret = -1; + + *relpath = NULL; + + for (next = from; next; next = next->backingStore) { + if (!next->backingRelative || !next->relPath) { + ret = 1; + goto cleanup; + } + + if (next != from) + virBufferAddLit(&buf, "/../"); + + virBufferAdd(&buf, next->relPath, -1); + + if (next == to) + break; + } + + if (next != to) + goto cleanup; + + if (!(tmp = virBufferContentAndReset(&buf))) + goto cleanup; + + if (!(*relpath = virStorageFileSimplifyPath(tmp, true))) + goto cleanup; + + ret = 0; + + cleanup: + virBufferFreeAndReset(&buf); + VIR_FREE(tmp); + return ret; +} diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 3e9e5c8..cbac30b 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -328,4 +328,8 @@ virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent); char *virStorageFileSimplifyPath(const char *path, bool allow_relative); +int virStorageFileGetRelativeBackingPath(virStorageSourcePtr from, + virStorageSourcePtr to, + char **relpath); + #endif /* __VIR_STORAGE_FILE_H__ */ diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 57f16ca..80d73ca 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -560,6 +560,85 @@ testPathSimplify(const void *args) } +virStorageSource backingchain[9]; + +static void +testPathRelativePrepare(void) +{ + size_t i; + + for (i = 0; i < ARRAY_CARDINALITY(backingchain) - 1; i++) { + backingchain[i].backingStore = &backingchain[i+1]; + } + + backingchain[0].relPath = (char *) "/path/to/some/img"; + backingchain[0].backingRelative = false; + + backingchain[1].relPath = (char *) "asdf"; + backingchain[1].backingRelative = true; + + backingchain[2].relPath = (char *) "test"; + backingchain[2].backingRelative = true; + + backingchain[3].relPath = (char *) "blah"; + backingchain[3].backingRelative = true; + + backingchain[4].relPath = (char *) "/path/to/some/other/img"; + backingchain[4].backingRelative = false; + + backingchain[5].relPath = (char *) "../relative/in/other/path"; + backingchain[5].backingRelative = true; + + backingchain[6].relPath = (char *) "test"; + backingchain[6].backingRelative = true; + + backingchain[7].relPath = (char *) "../../../../../below"; + backingchain[7].backingRelative = true; + + backingchain[8].relPath = (char *) "a/little/more/upwards"; + backingchain[8].backingRelative = true; +} + + +struct testPathRelativeBacking +{ + virStorageSourcePtr from; + virStorageSourcePtr to; + + const char *expect; +}; + +static int +testPathRelative(const void *args) +{ + const struct testPathRelativeBacking *data = args; + char *actual = NULL; + int ret = -1; + + if (virStorageFileGetRelativeBackingPath(data->from, + data->to, + &actual) < 0) { + fprintf(stderr, "relative backing path resolution failed\n"); + goto cleanup; + } + + if (STRNEQ_NULLABLE(data->expect, actual)) { + fprintf(stderr, "relative path resolution from '%s' to '%s': " + "expected '%s', got '%s'\n", + data->from->relPath, data->to->relPath, + NULLSTR(data->expect), NULLSTR(actual)); + goto cleanup; + } + + ret = 0; + + cleanup: + VIR_FREE(actual); + + return ret; +} + + static int mymain(void) { @@ -567,6 +646,7 @@ mymain(void) virCommandPtr cmd = NULL; struct testChainData data; struct testPathSimplifyData data3; + struct testPathRelativeBacking data4; virStorageSourcePtr chain = NULL; /* Prep some files with qemu-img; if that is not found on PATH, or @@ -1099,6 +1179,27 @@ mymain(void) TEST_SIMPLIFY(21, "some/path/to/image.qcow/../image2.qcow/../image3.qcow/", "some/path/to/image3.qcow", "some/path/to/image3.qcow"); +#define TEST_RELATIVE_BACKING(id, FROM, TO, EXPECT) \ + do { \ + data4.from = &FROM; \ + data4.to = &TO; \ + data4.expect = EXPECT; \ + if (virtTestRun("Path relative resolve " #id, \ + testPathRelative, &data4) < 0) \ + ret = -1; \ + } while (0) + + testPathRelativePrepare(); + + TEST_RELATIVE_BACKING(1, backingchain[0], backingchain[1], NULL); + TEST_RELATIVE_BACKING(2, backingchain[1], backingchain[2], "test"); + TEST_RELATIVE_BACKING(3, backingchain[2], backingchain[3], "blah"); + TEST_RELATIVE_BACKING(4, backingchain[1], backingchain[3], "blah"); + TEST_RELATIVE_BACKING(5, backingchain[1], backingchain[4], NULL); + TEST_RELATIVE_BACKING(6, backingchain[5], backingchain[6], "../relative/in/other/test"); + TEST_RELATIVE_BACKING(7, backingchain[5], backingchain[7], "../../../below"); + TEST_RELATIVE_BACKING(8, backingchain[5], backingchain[8], "../../../a/little/more/upwards"); + cleanup: /* Final cleanup */ virStorageSourceFree(chain); -- 1.9.3
On 05/30/2014 02:37 AM, Peter Krempa wrote:
This patch introduces a function that will allow us to resolve a relative difference between two elements of a disk backing chain. This fucntion will be used to allow relative block commit and block pull
s/fucntion/function/
where we need to specify the new relative name of the image to qemu.
This patch also adds unit tests for the function to verify that it works correctly. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 45 +++++++++++++++++++++ src/util/virstoragefile.h | 4 ++ tests/virstoragetest.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 151 insertions(+)
+int +virStorageFileGetRelativeBackingPath(virStorageSourcePtr from, + virStorageSourcePtr to, + char **relpath)
Missing documentation on what this function is intended to do.
+{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + virStorageSourcePtr next; + char *tmp = NULL; + char ret = -1; + + *relpath = NULL; + + for (next = from; next; next = next->backingStore) { + if (!next->backingRelative || !next->relPath) { + ret = 1; + goto cleanup; + } + + if (next != from) + virBufferAddLit(&buf, "/../"); + + virBufferAdd(&buf, next->relPath, -1); + + if (next == to) + break; + } + + if (next != to) + goto cleanup; + + if (!(tmp = virBufferContentAndReset(&buf))) + goto cleanup; + + if (!(*relpath = virStorageFileSimplifyPath(tmp, true)))
Ouch. Playing fast and loose with 'path/to/file/../otherfile' as a way to simplify to 'path/to/otherfile' is very risky. Instead of doing 'path/to/file'+'/../'+'otherfile', I'd feel better doing mdirname('path/to/file')+'/'+'otherfile' == 'path/to'+'/'+'otherfile'. Don't we already have a helper function in util/virfile.h that can concatenate a relative filename in relation to the containing directory of another filename? /me re-reads virFileBuildPath... nope, not quite what we need.
+virStorageSource backingchain[9]; + +static void +testPathRelativePrepare(void) +{ + size_t i; + + for (i = 0; i < ARRAY_CARDINALITY(backingchain) - 1; i++) { + backingchain[i].backingStore = &backingchain[i+1]; + } + + backingchain[0].relPath = (char *) "/path/to/some/img"; + backingchain[0].backingRelative = false; + + backingchain[1].relPath = (char *) "asdf"; + backingchain[1].backingRelative = true; + + backingchain[2].relPath = (char *) "test"; + backingchain[2].backingRelative = true; + + backingchain[3].relPath = (char *) "blah"; + backingchain[3].backingRelative = true;
1 through 3 are relative to the directory containing img in 0, but that is '/path/to/some/blah' and not a simplification of '/path/to/some/img/../blah' since 'img/..' doesn't resolve via POSIX functions.
+ + backingchain[4].relPath = (char *) "/path/to/some/other/img"; + backingchain[4].backingRelative = false;
As a non-relative image, the search for relative starts over again.
+ + backingchain[5].relPath = (char *) "../relative/in/other/path"; + backingchain[5].backingRelative = true;
Here, the answer has to be "/path/to/some/other/../relative/in/other/path", unless we did a stat test to ensure that '/path/to/some/other/..' resolves to the same location as '/path/to/some'.
+ + backingchain[6].relPath = (char *) "test"; + backingchain[6].backingRelative = true; + + backingchain[7].relPath = (char *) "../../../../../below"; + backingchain[7].backingRelative = true;
I see that you are trying to test that you can't escape past /...
+ + backingchain[8].relPath = (char *) "a/little/more/upwards"; + backingchain[8].backingRelative = true;
but that still doesn't make me feel good about simplifying .. without actual stat tests.
+ testPathRelativePrepare(); + + TEST_RELATIVE_BACKING(1, backingchain[0], backingchain[1], NULL);
I'm trying to wrap my head around this test and the expected results, and merely got confused. I need something that describes what we are doing visually, something like: Given the chain { "base" <- "intermediate" <- "active" }, we plan to commit "intermediate" into "base" and need to rewrite the backing file stored in "active" to point to "base". TEST_RELATIVE_BACKING(, active, intermediate, expected) then gives the expected string to write into active. Except that my formulation doesn't work with what your code expects, or I'm getting lost somewhere. backingchain[0] is the active image (living at "/path/to/some/file", and with backing element currently at the relative "asdf"); and we are committing backingchain[1] (file at "asdf", whose backing is currently "test"). Doesn't that mean that we want to determine a relative name for "test" that we can store in the metadata for "/path/to/some/file"? If so, isn't the answer "asdf", not NULL? I need more help understanding your goal here. :(
+ TEST_RELATIVE_BACKING(2, backingchain[1], backingchain[2], "test"); + TEST_RELATIVE_BACKING(3, backingchain[2], backingchain[3], "blah"); + TEST_RELATIVE_BACKING(4, backingchain[1], backingchain[3], "blah"); + TEST_RELATIVE_BACKING(5, backingchain[1], backingchain[4], NULL); + TEST_RELATIVE_BACKING(6, backingchain[5], backingchain[6], "../relative/in/other/test"); + TEST_RELATIVE_BACKING(7, backingchain[5], backingchain[7], "../../../below"); + TEST_RELATIVE_BACKING(8, backingchain[5], backingchain[8], "../../../a/little/more/upwards");
and to reiterate, I'm not sure we can elide any 'foo/../' pairs in our simplification, without actually stat'ing the local filesystem or using the equivalent libgfapi or other network driver calls. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The recently introduced virStorageFileSimplifyPath is good at resolving relative portions of a path. To add full path canonicalization capability we need to be able to resolve symlinks in the path too. This patch adds a callback to that function so that arbitrary storage systems can use this functionality. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 82 +++++++++++++++++++++++++++++++++++++++++++++-- src/util/virstoragefile.h | 9 ++++++ tests/virstoragetest.c | 80 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 170 insertions(+), 2 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8e00d8c..7de04ed 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1872,6 +1872,7 @@ virStorageFileIsClusterFS; virStorageFileParseChainIndex; virStorageFileProbeFormat; virStorageFileResize; +virStorageFileSimplifyPathInternal; virStorageIsFile; virStorageNetHostDefClear; virStorageNetHostDefCopy; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index d8b4b54..301452e 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1962,13 +1962,47 @@ virStorageFileExportPath(char **components, } +static int +virStorageFileExplodePath(const char *path, + size_t at, + char ***components, + size_t *ncomponents) +{ + char **tmp = NULL; + char **next; + size_t ntmp = 0; + int ret = -1; + + if (!(tmp = virStringSplitCount(path, "/", 0, &ntmp))) + goto cleanup; + + /* prepend */ + for (next = tmp; *next; next++) { + if (VIR_INSERT_ELEMENT(*components, at, *ncomponents, *next) < 0) + goto cleanup; + + at++; + } + + ret = 0; + + cleanup: + virStringFreeListCount(tmp, ntmp); + return ret; +} + + char * -virStorageFileSimplifyPath(const char *path, - bool allow_relative) +virStorageFileSimplifyPathInternal(const char *path, + bool allow_relative, + virStorageFileSimplifyPathReadlinkCallback cb, + void *cbdata) { bool beginSlash = false; char **components = NULL; size_t ncomponents = 0; + char *linkpath = NULL; + char *currentpath = NULL; size_t i; char *ret = NULL; @@ -2012,6 +2046,40 @@ virStorageFileSimplifyPath(const char *path, continue; } + /* read link and stuff */ + if (cb) { + int rc; + if (!(currentpath = virStorageFileExportPath(components, i + 1, + beginSlash))) + goto cleanup; + + if ((rc = cb(currentpath, &linkpath, cbdata)) < 0) + goto cleanup; + + if (rc == 0) { + if (linkpath[0] == '/') { + /* start from a clean slate */ + virStringFreeListCount(components, ncomponents); + ncomponents = 0; + components = NULL; + beginSlash = true; + i = 0; + } else { + VIR_FREE(components[i]); + VIR_DELETE_ELEMENT(components, i, ncomponents); + } + + if (virStorageFileExplodePath(linkpath, i, &components, + &ncomponents) < 0) + goto cleanup; + + VIR_FREE(linkpath); + VIR_FREE(currentpath); + + continue; + } + } + i++; } @@ -2019,11 +2087,21 @@ virStorageFileSimplifyPath(const char *path, cleanup: virStringFreeListCount(components, ncomponents); + VIR_FREE(linkpath); + VIR_FREE(currentpath); return ret; } +char * +virStorageFileSimplifyPath(const char *path, + bool allow_relative) +{ + return virStorageFileSimplifyPathInternal(path, allow_relative, NULL, NULL); +} + + int virStorageFileGetRelativeBackingPath(virStorageSourcePtr from, virStorageSourcePtr to, diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index cbac30b..70deaef 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -325,6 +325,15 @@ void virStorageSourceFree(virStorageSourcePtr def); void virStorageSourceClearBackingStore(virStorageSourcePtr def); virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent); +typedef int +(*virStorageFileSimplifyPathReadlinkCallback)(const char *path, + char **link, + void *data); +char *virStorageFileSimplifyPathInternal(const char *path, + bool allow_relative, + virStorageFileSimplifyPathReadlinkCallback cb, + void *cbdata); + char *virStorageFileSimplifyPath(const char *path, bool allow_relative); diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 80d73ca..771df0b 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -639,6 +639,72 @@ testPathRelative(const void *args) } +struct testPathCanonicalizeData +{ + const char *path; + const char *expect; +}; + +static const char *testPathCanonicalizeSymlinks[][2] = +{ + {"/path/blah", "/other/path/huzah"}, + {"/path/to/relative/symlink", "../../actual/file"}, +}; + +static int +testPathCanonicalizeReadlink(const char *path, + char **link, + void *data ATTRIBUTE_UNUSED) +{ + size_t i; + + *link = NULL; + + for (i = 0; i < ARRAY_CARDINALITY(testPathCanonicalizeSymlinks); i++) { + if (STREQ(path, testPathCanonicalizeSymlinks[i][0])) { + if (VIR_STRDUP(*link, testPathCanonicalizeSymlinks[i][1]) < 0) + return -1; + + return 0; + } + } + + return 1; +} + + +static int +testPathCanonicalize(const void *args) +{ + const struct testPathCanonicalizeData *data = args; + char *canon = NULL; + int ret = -1; + + if (!(canon = virStorageFileSimplifyPathInternal(data->path, + false, + testPathCanonicalizeReadlink, + NULL))) { + fprintf(stderr, "path canonicalization failed\n"); + goto cleanup; + } + + if (STRNEQ_NULLABLE(data->expect, canon)) { + fprintf(stderr, + "path canonicalization of '%s' failed: expected '%s' got '%s'\n", + data->path, data->expect, canon); + + goto cleanup; + } + + ret = 0; + + cleanup: + VIR_FREE(canon); + + return ret; +} + + static int mymain(void) { @@ -647,6 +713,7 @@ mymain(void) struct testChainData data; struct testPathSimplifyData data3; struct testPathRelativeBacking data4; + struct testPathCanonicalizeData data5; virStorageSourcePtr chain = NULL; /* Prep some files with qemu-img; if that is not found on PATH, or @@ -1200,6 +1267,19 @@ mymain(void) TEST_RELATIVE_BACKING(7, backingchain[5], backingchain[7], "../../../below"); TEST_RELATIVE_BACKING(8, backingchain[5], backingchain[8], "../../../a/little/more/upwards"); +#define TEST_PATH_CANONICALIZE(id, PATH, EXPECT) \ + do { \ + data5.path = PATH; \ + data5.expect = EXPECT; \ + if (virtTestRun("Path canonicalize " #id, \ + testPathCanonicalize, &data5) < 0) \ + ret = -1; \ + } while (0) + + TEST_PATH_CANONICALIZE(1, "/some/full/path", "/some/full/path"); + TEST_PATH_CANONICALIZE(2, "/path/blah", "/other/path/huzah"); + TEST_PATH_CANONICALIZE(3, "/path/to/relative/symlink", "/path/actual/file"); + cleanup: /* Final cleanup */ virStorageSourceFree(chain); -- 1.9.3
On 05/30/2014 02:37 AM, Peter Krempa wrote:
The recently introduced virStorageFileSimplifyPath is good at resolving relative portions of a path. To add full path canonicalization capability we need to be able to resolve symlinks in the path too.
This patch adds a callback to that function so that arbitrary storage systems can use this functionality. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 82 +++++++++++++++++++++++++++++++++++++++++++++-- src/util/virstoragefile.h | 9 ++++++ tests/virstoragetest.c | 80 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 170 insertions(+), 2 deletions(-)
+static int +virStorageFileExplodePath(const char *path, + size_t at, + char ***components, + size_t *ncomponents) +{ + char **tmp = NULL; + char **next; + size_t ntmp = 0; + int ret = -1; + + if (!(tmp = virStringSplitCount(path, "/", 0, &ntmp))) + goto cleanup; + + /* prepend */ + for (next = tmp; *next; next++) { + if (VIR_INSERT_ELEMENT(*components, at, *ncomponents, *next) < 0)
So this is the call that is not necessarily guaranteeing NULL termination...
+ goto cleanup; + + at++; + } + + ret = 0; + + cleanup: + virStringFreeListCount(tmp, ntmp);
...and therefore why you wanted to add this function from 14/36. Hmm, aren't you just taking the split array and reversing its order? Do you really need VIR_INSERT_ELEMENT for each array element, or could you just VIR_ALLOC_N an array already big enough and then do the assignment from 'n' to 'size - n' in a loop through all size entries, at which point you'd be guaranteed a NULL terminator and fewer intermediate malloc calls?
+ return ret; +} + + char * -virStorageFileSimplifyPath(const char *path, - bool allow_relative) +virStorageFileSimplifyPathInternal(const char *path, + bool allow_relative, + virStorageFileSimplifyPathReadlinkCallback cb, + void *cbdata) { bool beginSlash = false; char **components = NULL; size_t ncomponents = 0; + char *linkpath = NULL; + char *currentpath = NULL; size_t i; char *ret = NULL;
@@ -2012,6 +2046,40 @@ virStorageFileSimplifyPath(const char *path, continue; }
+ /* read link and stuff */ + if (cb) { + int rc; + if (!(currentpath = virStorageFileExportPath(components, i + 1, + beginSlash)))
My first thought when reading this in isolation was "Why are we changing the environment variable PATH to export it"? I'm not sure how much of your existing series is impacted, but I'm wondering if using "name" is better than "path" when referring to a file name (GNU Coding Standards recommends this nomenclature for a reason, after all, even if glibc's realpath() muddies the water by having a use of "path" for a non-PATH entity).
+ goto cleanup; + + if ((rc = cb(currentpath, &linkpath, cbdata)) < 0) + goto cleanup; + + if (rc == 0) { + if (linkpath[0] == '/') { + /* start from a clean slate */ + virStringFreeListCount(components, ncomponents); + ncomponents = 0; + components = NULL; + beginSlash = true; + i = 0; + } else { + VIR_FREE(components[i]); + VIR_DELETE_ELEMENT(components, i, ncomponents); + } + + if (virStorageFileExplodePath(linkpath, i, &components, + &ncomponents) < 0) + goto cleanup; + + VIR_FREE(linkpath); + VIR_FREE(currentpath); + + continue; + } + } +
I've run out of review time at the moment, I'll have to pick up here again later... -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/30/2014 02:37 AM, Peter Krempa wrote:
The recently introduced virStorageFileSimplifyPath is good at resolving relative portions of a path. To add full path canonicalization capability we need to be able to resolve symlinks in the path too.
This patch adds a callback to that function so that arbitrary storage systems can use this functionality. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 82 +++++++++++++++++++++++++++++++++++++++++++++-- src/util/virstoragefile.h | 9 ++++++ tests/virstoragetest.c | 80 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 170 insertions(+), 2 deletions(-)
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 80d73ca..771df0b 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -639,6 +639,72 @@ testPathRelative(const void *args) }
+struct testPathCanonicalizeData +{ + const char *path; + const char *expect; +}; + +static const char *testPathCanonicalizeSymlinks[][2] = +{ + {"/path/blah", "/other/path/huzah"}, + {"/path/to/relative/symlink", "../../actual/file"}, +};
Looks like a good way to test both types of symlinks.
+ + TEST_PATH_CANONICALIZE(1, "/some/full/path", "/some/full/path"); + TEST_PATH_CANONICALIZE(2, "/path/blah", "/other/path/huzah"); + TEST_PATH_CANONICALIZE(3, "/path/to/relative/symlink", "/path/actual/file"); +
You probably also want to test a symlink to directory in the middle of a longer name: for example, /path/blah/yippee should resolve to /other/path/huzah/yippee
cleanup: /* Final cleanup */ virStorageSourceFree(chain);
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Use virStorageFileSimplifyPathInternal to canonicalize gluster paths via a callback and use it for the unique volume path retrieval API. --- src/storage/storage_backend_gluster.c | 81 +++++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 3db4e66..1e86383 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -533,6 +533,7 @@ typedef virStorageFileBackendGlusterPriv *virStorageFileBackendGlusterPrivPtr; struct _virStorageFileBackendGlusterPriv { glfs_t *vol; + char *uid; }; @@ -547,6 +548,7 @@ virStorageFileBackendGlusterDeinit(virStorageSourcePtr src) if (priv->vol) glfs_fini(priv->vol); + VIR_FREE(priv->uid); VIR_FREE(priv); src->drv->priv = NULL; @@ -712,6 +714,81 @@ virStorageFileBackendGlusterAccess(virStorageSourcePtr src, return glfs_access(priv->vol, src->path, mode); } +static int +virStorageFileBackendGlusterReadlinkCallback(const char *path, + char **link, + void *data) +{ + virStorageFileBackendGlusterPrivPtr priv = data; + char *buf = NULL; + size_t bufsiz = 0; + ssize_t ret; + struct stat st; + + *link = NULL; + + if (glfs_stat(priv->vol, path, &st) < 0) { + virReportSystemError(errno, + _("failed to stat gluster path '%s'"), + path); + return -1; + } + + if (!S_ISLNK(st.st_mode)) + return 1; + + realloc: + if (VIR_EXPAND_N(buf, bufsiz, 256) < 0) + goto error; + + if ((ret = glfs_readlink(priv->vol, path, buf, bufsiz)) < 0) { + virReportSystemError(errno, + _("failed to read link of gluster file '%s'"), + path); + goto error; + } + + if (ret == bufsiz) + goto realloc; + + buf[ret] = '\0'; + + *link = buf; + + return 0; + + error: + VIR_FREE(buf); + return -1; +} + + +static const char * +virStorageFileBackendGlusterGetUniqueIdentifier(virStorageSourcePtr src) +{ + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; + char *canonPath = NULL; + + if (priv->uid) + return priv->uid; + + if (!(canonPath = virStorageFileSimplifyPathInternal(src->path, + false, + virStorageFileBackendGlusterReadlinkCallback, + priv))) + return NULL; + + ignore_value(virAsprintf(&priv->uid, "gluster://%s:%s/%s/%s", + src->hosts->name, + src->hosts->port, + src->volume, + canonPath)); + + VIR_FREE(canonPath); + + return priv->uid; +} + virStorageFileBackend virStorageFileBackendGluster = { .type = VIR_STORAGE_TYPE_NETWORK, @@ -724,4 +801,8 @@ virStorageFileBackend virStorageFileBackendGluster = { .storageFileStat = virStorageFileBackendGlusterStat, .storageFileReadHeader = virStorageFileBackendGlusterReadHeader, .storageFileAccess = virStorageFileBackendGlusterAccess, + + .storageFileGetUniqueIdentifier = virStorageFileBackendGlusterGetUniqueIdentifier, + + }; -- 1.9.3
On 05/30/2014 02:37 AM, Peter Krempa wrote:
Use virStorageFileSimplifyPathInternal to canonicalize gluster paths via a callback and use it for the unique volume path retrieval API. --- src/storage/storage_backend_gluster.c | 81 +++++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+)
diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 3db4e66..1e86383 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -533,6 +533,7 @@ typedef virStorageFileBackendGlusterPriv *virStorageFileBackendGlusterPrivPtr;
struct _virStorageFileBackendGlusterPriv { glfs_t *vol; + char *uid;
Once again, char *uid is misnamed (when I see uid, I think integral uid_t). Otherwise, seems to make sense. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
This patch adds option to specify that a json qemu command argument is optional without the need to use if's or ternary operators to pass the list. Additionally all the modifier characters are documented to avoid user confusion. --- src/qemu/qemu_monitor_json.c | 122 +++++++++++++++++++++++++++++-------------- 1 file changed, 84 insertions(+), 38 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 914f3ef..d78bd30 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -451,7 +451,28 @@ qemuMonitorJSONMakeCommandRaw(bool wrap, const char *cmdname, ...) goto error; } - /* Keys look like s:name the first letter is a type code */ + /* Keys look like s:name the first letter is a type code: + * Explanation of type codes: + * s: string value, must be non-null + * S: string value, signed, omitted if null + * + * i: signed integer value + * z: signed integer value, omitted if zero + * + * I: signed long integer value + * Z: integer value, signed, omitted if zero + * + * u: unsigned integer value + * p: unsigned integer value, omitted if zero + * + * U: unsigned long integer value (see below for quirks) + * P: unsigned long integer value, omitted if zero, + * + * d: double precision floating point number + * b: boolean value + * n: json null value + * a: json array + */ type = key[0]; key += 2; @@ -461,9 +482,13 @@ qemuMonitorJSONMakeCommandRaw(bool wrap, const char *cmdname, ...) /* This doesn't support maps, but no command uses those. */ switch (type) { + case 'S': case 's': { char *val = va_arg(args, char *); if (!val) { + if (type == 'S') + continue; + virReportError(VIR_ERR_INTERNAL_ERROR, _("argument key '%s' must not have null value"), key); @@ -471,18 +496,38 @@ qemuMonitorJSONMakeCommandRaw(bool wrap, const char *cmdname, ...) } ret = virJSONValueObjectAppendString(jargs, key, val); } break; + + case 'z': case 'i': { int val = va_arg(args, int); + + if (!val && type == 'z') + continue; + ret = virJSONValueObjectAppendNumberInt(jargs, key, val); } break; + + case 'p': case 'u': { unsigned int val = va_arg(args, unsigned int); + + if (!val && type == 'p') + continue; + ret = virJSONValueObjectAppendNumberUint(jargs, key, val); } break; + + case 'Z': case 'I': { long long val = va_arg(args, long long); + + if (!val && type == 'Z') + continue; + ret = virJSONValueObjectAppendNumberLong(jargs, key, val); } break; + + case 'P': case 'U': { /* qemu silently truncates numbers larger than LLONG_MAX, * so passing the full range of unsigned 64 bit integers @@ -490,23 +535,32 @@ qemuMonitorJSONMakeCommandRaw(bool wrap, const char *cmdname, ...) * instead. */ long long val = va_arg(args, long long); + + if (!val && type == 'P') + continue; + ret = virJSONValueObjectAppendNumberLong(jargs, key, val); } break; + case 'd': { double val = va_arg(args, double); ret = virJSONValueObjectAppendNumberDouble(jargs, key, val); } break; + case 'b': { int val = va_arg(args, int); ret = virJSONValueObjectAppendBoolean(jargs, key, val); } break; + case 'n': { ret = virJSONValueObjectAppendNull(jargs, key); } break; + case 'a': { virJSONValuePtr val = va_arg(args, virJSONValuePtr); ret = virJSONValueObjectAppend(jargs, key, val); } break; + default: virReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported data type '%c' for arg '%s'"), type, key - 2); @@ -2203,19 +2257,14 @@ int qemuMonitorJSONChangeMedia(qemuMonitorPtr mon, { int ret; virJSONValuePtr cmd; - if (format) - cmd = qemuMonitorJSONMakeCommand("change", - "s:device", dev_name, - "s:target", newmedia, - "s:arg", format, - NULL); - else - cmd = qemuMonitorJSONMakeCommand("change", - "s:device", dev_name, - "s:target", newmedia, - NULL); - virJSONValuePtr reply = NULL; + + cmd = qemuMonitorJSONMakeCommand("change", + "s:device", dev_name, + "s:target", newmedia, + "S:arg", format, + NULL); + if (!cmd) return -1; @@ -2781,8 +2830,7 @@ int qemuMonitorJSONGraphicsRelocate(qemuMonitorPtr mon, "s:hostname", hostname, "i:port", port, "i:tls-port", tlsPort, - (tlsSubject ? "s:cert-subject" : NULL), - (tlsSubject ? tlsSubject : NULL), + "S:cert-subject", tlsSubject, NULL); virJSONValuePtr reply = NULL; if (!cmd) @@ -2919,8 +2967,8 @@ qemuMonitorJSONAddFd(qemuMonitorPtr mon, int fdset, int fd, const char *name) int ret; virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("add-fd", "i:fdset-id", fdset, - name ? "s:opaque" : NULL, - name, NULL); + "S:opaque", name, + NULL); virJSONValuePtr reply = NULL; if (!cmd) return -1; @@ -3314,8 +3362,7 @@ qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions, "s:device", device, "s:snapshot-file", file, "s:format", format, - reuse ? "s:mode" : NULL, - reuse ? "existing" : NULL, + "S:mode", reuse ? "existing" : NULL, NULL); if (!cmd) return -1; @@ -3356,9 +3403,8 @@ qemuMonitorJSONDriveMirror(qemuMonitorPtr mon, "s:target", file, "U:speed", speed, "s:sync", shallow ? "top" : "full", - "s:mode", - reuse ? "existing" : "absolute-paths", - format ? "s:format" : NULL, format, + "s:mode", reuse ? "existing" : "absolute-paths", + "S:format", format, NULL); if (!cmd) return -1; @@ -3557,7 +3603,7 @@ int qemuMonitorJSONSendKey(qemuMonitorPtr mon, cmd = qemuMonitorJSONMakeCommand("send-key", "a:keys", keys, - holdtime ? "U:hold-time" : NULL, holdtime, + "P:hold-time", holdtime, NULL); if (!cmd) goto cleanup; @@ -3736,31 +3782,31 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon, switch (mode) { case BLOCK_JOB_ABORT: cmd_name = modern ? "block-job-cancel" : "block_job_cancel"; - cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device", device, NULL); + cmd = qemuMonitorJSONMakeCommand(cmd_name, + "s:device", device, + NULL); break; + case BLOCK_JOB_INFO: cmd_name = "query-block-jobs"; cmd = qemuMonitorJSONMakeCommand(cmd_name, NULL); break; + case BLOCK_JOB_SPEED: cmd_name = modern ? "block-job-set-speed" : "block_job_set_speed"; - cmd = qemuMonitorJSONMakeCommand(cmd_name, "s:device", device, - modern ? "U:speed" : "U:value", - speed, NULL); + cmd = qemuMonitorJSONMakeCommand(cmd_name, + "s:device", device, + modern ? "U:speed" : "U:value", speed, + NULL); break; + case BLOCK_JOB_PULL: cmd_name = modern ? "block-stream" : "block_stream"; - if (speed) - cmd = qemuMonitorJSONMakeCommand(cmd_name, - "s:device", device, - "U:speed", speed, - base ? "s:base" : NULL, base, - NULL); - else - cmd = qemuMonitorJSONMakeCommand(cmd_name, - "s:device", device, - base ? "s:base" : NULL, base, - NULL); + cmd = qemuMonitorJSONMakeCommand(cmd_name, + "s:device", device, + "P:speed", speed, + "S:base", base, + NULL); break; } -- 1.9.3
On 05/30/2014 02:37 AM, Peter Krempa wrote:
This patch adds option to specify that a json qemu command argument is optional without the need to use if's or ternary operators to pass the list. Additionally all the modifier characters are documented to avoid user confusion. --- src/qemu/qemu_monitor_json.c | 122 +++++++++++++++++++++++++++++-------------- 1 file changed, 84 insertions(+), 38 deletions(-)
The diffstat is misleading - the bulk of the addition is documentation, and the bulk of the deletions are compression taking advantage of the new feature. Overall, I like the patch!
+ * + * i: signed integer value + * z: signed integer value, omitted if zero + * + * I: signed long integer value + * Z: integer value, signed, omitted if zero
Looks more consistent as: "signed long integer value, omitted if zero"
+ * + * u: unsigned integer value + * p: unsigned integer value, omitted if zero + * + * U: unsigned long integer value (see below for quirks) + * P: unsigned long integer value, omitted if zero,
drop the trailing comma
+ * + * d: double precision floating point number + * b: boolean value
Is it worth a B for a boolean value that is omitted if false?
+ * n: json null value + * a: json array + */ type = key[0]; key += 2;
@@ -3557,7 +3603,7 @@ int qemuMonitorJSONSendKey(qemuMonitorPtr mon,
cmd = qemuMonitorJSONMakeCommand("send-key", "a:keys", keys, - holdtime ? "U:hold-time" : NULL, holdtime, + "P:hold-time", holdtime, NULL);
Fix the indentation while you are at it. ACK with nits addressed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/03/14 05:24, Eric Blake wrote:
On 05/30/2014 02:37 AM, Peter Krempa wrote:
This patch adds option to specify that a json qemu command argument is optional without the need to use if's or ternary operators to pass the list. Additionally all the modifier characters are documented to avoid user confusion. --- src/qemu/qemu_monitor_json.c | 122 +++++++++++++++++++++++++++++-------------- 1 file changed, 84 insertions(+), 38 deletions(-)
The diffstat is misleading - the bulk of the addition is documentation, and the bulk of the deletions are compression taking advantage of the new feature. Overall, I like the patch!
+ * + * i: signed integer value + * z: signed integer value, omitted if zero + * + * I: signed long integer value + * Z: integer value, signed, omitted if zero
Looks more consistent as: "signed long integer value, omitted if zero"
+ * + * u: unsigned integer value + * p: unsigned integer value, omitted if zero + * + * U: unsigned long integer value (see below for quirks) + * P: unsigned long integer value, omitted if zero,
drop the trailing comma
+ * + * d: double precision floating point number + * b: boolean value
Is it worth a B for a boolean value that is omitted if false?
I've added it as it's trivial. Hopefully someone will need it in the future.
+ * n: json null value + * a: json array + */ type = key[0]; key += 2;
@@ -3557,7 +3603,7 @@ int qemuMonitorJSONSendKey(qemuMonitorPtr mon,
cmd = qemuMonitorJSONMakeCommand("send-key", "a:keys", keys, - holdtime ? "U:hold-time" : NULL, holdtime, + "P:hold-time", holdtime, NULL);
Fix the indentation while you are at it.
ACK with nits addressed.
I've fixed the docs and formatting and pushed. Peter
All the fields crammed into two lines weren't easy to parse by human eyes. Split up the format string into lines and put it into a central variable so that changes in two places aren't necessary. --- tests/virstoragetest.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 771df0b..3c892d5 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -312,6 +312,18 @@ struct testChainData unsigned int flags; }; + +static const char testStorageChainFormat[] = + "store: %s\n" + "backingStoreRaw: %s\n" + "capacity: %lld\n" + "encryption: %d\n" + "relPath:%s\n" + "path:%s\n" + "relDir:%s\n" + "type:%d\n" + "format:%d\n"; + static int testStorageChain(const void *args) { @@ -373,8 +385,7 @@ testStorageChain(const void *args) expRelDir = isAbs ? data->files[i]->relDirAbs : data->files[i]->relDirRel; if (virAsprintf(&expect, - "store:%s\nraw:%s\nother:%lld %d\n" - "relPath:%s\npath:%s\nrelDir:%s\ntype:%d %d\n", + testStorageChainFormat, NULLSTR(data->files[i]->expBackingStore), NULLSTR(data->files[i]->expBackingStoreRaw), data->files[i]->expCapacity, @@ -385,15 +396,16 @@ testStorageChain(const void *args) data->files[i]->type, data->files[i]->format) < 0 || virAsprintf(&actual, - "store:%s\nraw:%s\nother:%lld %d\n" - "relPath:%s\npath:%s\nrelDir:%s\ntype:%d %d\n", + testStorageChainFormat, NULLSTR(elt->backingStore ? elt->backingStore->path : NULL), NULLSTR(elt->backingStoreRaw), - elt->capacity, !!elt->encryption, + elt->capacity, + !!elt->encryption, NULLSTR(elt->relPath), NULLSTR(elt->path), NULLSTR(elt->relDir), - elt->type, elt->format) < 0) { + elt->type, + elt->format) < 0) { VIR_FREE(expect); VIR_FREE(actual); goto cleanup; -- 1.9.3
On 05/30/2014 02:37 AM, Peter Krempa wrote:
All the fields crammed into two lines weren't easy to parse by human eyes. Split up the format string into lines and put it into a central variable so that changes in two places aren't necessary. --- tests/virstoragetest.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/03/14 05:34, Eric Blake wrote:
On 05/30/2014 02:37 AM, Peter Krempa wrote:
All the fields crammed into two lines weren't easy to parse by human eyes. Split up the format string into lines and put it into a central variable so that changes in two places aren't necessary. --- tests/virstoragetest.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)
ACK.
Pushed; Thanks. Peter
Now that we changed ordering of the stored metadata so that the backing store is described by the child element the test should reflect this change too. Remove the expected backing store field as it's actually described by the next element in the backing chain, so there's no need for duplication. --- tests/virstoragetest.c | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 3c892d5..03f8552 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -282,7 +282,6 @@ testPrepImages(void) typedef struct _testFileData testFileData; struct _testFileData { - const char *expBackingStore; const char *expBackingStoreRaw; unsigned long long expCapacity; bool expEncrypted; @@ -314,12 +313,11 @@ struct testChainData static const char testStorageChainFormat[] = - "store: %s\n" + "path:%s\n" "backingStoreRaw: %s\n" "capacity: %lld\n" "encryption: %d\n" "relPath:%s\n" - "path:%s\n" "relDir:%s\n" "type:%d\n" "format:%d\n"; @@ -386,23 +384,21 @@ testStorageChain(const void *args) : data->files[i]->relDirRel; if (virAsprintf(&expect, testStorageChainFormat, - NULLSTR(data->files[i]->expBackingStore), + NULLSTR(data->files[i]->path), NULLSTR(data->files[i]->expBackingStoreRaw), data->files[i]->expCapacity, data->files[i]->expEncrypted, NULLSTR(expPath), - NULLSTR(data->files[i]->path), NULLSTR(expRelDir), data->files[i]->type, data->files[i]->format) < 0 || virAsprintf(&actual, testStorageChainFormat, - NULLSTR(elt->backingStore ? elt->backingStore->path : NULL), + NULLSTR(elt->path), NULLSTR(elt->backingStoreRaw), elt->capacity, !!elt->encryption, NULLSTR(elt->relPath), - NULLSTR(elt->path), NULLSTR(elt->relDir), elt->type, elt->format) < 0) { @@ -793,7 +789,6 @@ mymain(void) /* Qcow2 file with relative raw backing, format provided */ raw.pathAbs = "raw"; testFileData qcow2 = { - .expBackingStore = canonraw, .expBackingStoreRaw = "raw", .expCapacity = 1024, .pathRel = "qcow2", @@ -849,7 +844,6 @@ mymain(void) /* Wrapped file access */ testFileData wrap = { - .expBackingStore = canonqcow2, .expBackingStoreRaw = absqcow2, .expCapacity = 1024, .pathRel = "wrap", @@ -885,7 +879,6 @@ mymain(void) /* Qcow2 file with raw as absolute backing, backing format omitted */ testFileData wrap_as_raw = { - .expBackingStore = canonqcow2, .expBackingStoreRaw = absqcow2, .expCapacity = 1024, .pathRel = "wrap", @@ -909,7 +902,6 @@ mymain(void) "qcow2", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; - qcow2.expBackingStore = NULL; qcow2.expBackingStoreRaw = datadir "/bogus"; qcow2.pathRel = "qcow2"; qcow2.relDirRel = "."; @@ -942,7 +934,6 @@ mymain(void) "qcow2", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; - qcow2.expBackingStore = "blah"; qcow2.expBackingStoreRaw = "nbd:example.org:6000:exportname=blah"; /* Qcow2 file with backing protocol instead of file */ @@ -963,7 +954,6 @@ mymain(void) /* qed file */ testFileData qed = { - .expBackingStore = canonraw, .expBackingStoreRaw = absraw, .expCapacity = 1024, .pathRel = "qed", @@ -1028,7 +1018,6 @@ mymain(void) /* Behavior of symlinks to qcow2 with relative backing files */ testFileData link1 = { - .expBackingStore = canonraw, .expBackingStoreRaw = "../raw", .expCapacity = 1024, .pathRel = "../sub/link1", @@ -1040,7 +1029,6 @@ mymain(void) .format = VIR_STORAGE_FILE_QCOW2, }; testFileData link2 = { - .expBackingStore = canonqcow2, .expBackingStoreRaw = "../sub/link1", .expCapacity = 1024, .pathRel = "sub/link2", @@ -1068,7 +1056,6 @@ mymain(void) "-F", "qcow2", "-b", "qcow2", "qcow2", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; - qcow2.expBackingStore = NULL; qcow2.expBackingStoreRaw = "qcow2"; /* Behavior of an infinite loop chain */ -- 1.9.3
When the test is failing but the debug output isn't enabled the resulting line would look ugly like and would not contain the actual difference. TEST: virstoragetest .................chain member 1!chain member 1!chain member 1! Store the member index in the actual checked string to hide this problem --- tests/virstoragetest.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 03f8552..e1adce9 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -313,6 +313,7 @@ struct testChainData static const char testStorageChainFormat[] = + "chain member: %zu\n" "path:%s\n" "backingStoreRaw: %s\n" "capacity: %lld\n" @@ -383,7 +384,7 @@ testStorageChain(const void *args) expRelDir = isAbs ? data->files[i]->relDirAbs : data->files[i]->relDirRel; if (virAsprintf(&expect, - testStorageChainFormat, + testStorageChainFormat, i, NULLSTR(data->files[i]->path), NULLSTR(data->files[i]->expBackingStoreRaw), data->files[i]->expCapacity, @@ -393,7 +394,7 @@ testStorageChain(const void *args) data->files[i]->type, data->files[i]->format) < 0 || virAsprintf(&actual, - testStorageChainFormat, + testStorageChainFormat, i, NULLSTR(elt->path), NULLSTR(elt->backingStoreRaw), elt->capacity, @@ -407,7 +408,6 @@ testStorageChain(const void *args) goto cleanup; } if (STRNEQ(expect, actual)) { - fprintf(stderr, "chain member %zu", i); virtTestDifference(stderr, expect, actual); VIR_FREE(expect); VIR_FREE(actual); -- 1.9.3
On 05/30/2014 02:37 AM, Peter Krempa wrote:
When the test is failing but the debug output isn't enabled the resulting line would look ugly like and would not contain the actual difference.
TEST: virstoragetest .................chain member 1!chain member 1!chain member 1!
Store the member index in the actual checked string to hide this problem --- tests/virstoragetest.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
ACK. Thanks for cleaning up after me; I stuck in the chain member as a debug item a while back, and forgot to remove it (or formalize it). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Due to various refactors and compatibility with the virstoragetest the relPath field of the virStorageSource structure was always filled either with the relative name or the full path in case of abslutely backed storage. Return it's original purpose to store only the relative name of the disk if it is backed relatively and tweak the tests. --- src/storage/storage_driver.c | 4 ---- src/util/virstoragefile.c | 21 +++++++++------------ src/util/virstoragefile.h | 4 ++-- tests/virstoragetest.c | 25 +++---------------------- 4 files changed, 14 insertions(+), 40 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 4f51517..9ce3b62 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -3189,10 +3189,6 @@ virStorageFileGetMetadata(virStorageSourcePtr src, if (!(cycle = virHashCreate(5, NULL))) return -1; - if (!src->relPath && - VIR_STRDUP(src->relPath, src->path) < 0) - goto cleanup; - if (!src->relDir && !(src->relDir = mdir_name(src->path))) { virReportOOMError(); diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 301452e..b33bc1a 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -764,11 +764,11 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta, { int ret = -1; - VIR_DEBUG("relPath=%s, buf=%p, len=%zu, meta->format=%d", - meta->relPath, buf, len, meta->format); + VIR_DEBUG("path=%s, buf=%p, len=%zu, meta->format=%d", + meta->path, buf, len, meta->format); if (meta->format == VIR_STORAGE_FILE_AUTO) - meta->format = virStorageFileProbeFormatFromBuf(meta->relPath, buf, len); + meta->format = virStorageFileProbeFormatFromBuf(meta->path, buf, len); if (meta->format <= VIR_STORAGE_FILE_NONE || meta->format >= VIR_STORAGE_FILE_LAST) { @@ -907,9 +907,6 @@ virStorageFileMetadataNew(const char *path, ret->format = format; ret->type = VIR_STORAGE_TYPE_FILE; - if (VIR_STRDUP(ret->relPath, path) < 0) - goto error; - if (VIR_STRDUP(ret->path, path) < 0) goto error; @@ -1375,7 +1372,8 @@ virStorageFileChainLookup(virStorageSourcePtr chain, if (idx == i) break; } else { - if (STREQ_NULLABLE(name, chain->relPath)) + if (STREQ_NULLABLE(name, chain->relPath) || + STREQ(name, chain->path)) break; if (nameIsFile && (chain->type == VIR_STORAGE_TYPE_FILE || chain->type == VIR_STORAGE_TYPE_BLOCK)) { @@ -1594,6 +1592,10 @@ virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent, ret->backingRelative = true; + /* store relative name */ + if (VIR_STRDUP(ret->relPath, parent->backingStoreRaw) < 0) + goto error; + /* XXX Once we get rid of the need to use canonical names in path, we will be * able to use mdir_name on parent->path instead of using parent->relDir */ if (STRNEQ(parent->relDir, "/")) @@ -1908,11 +1910,6 @@ virStorageSourceNewFromBacking(virStorageSourcePtr parent) ret = virStorageSourceNewFromBackingAbsolute(parent->backingStoreRaw); if (ret) { - if (VIR_STRDUP(ret->relPath, parent->backingStoreRaw) < 0) { - virStorageSourceFree(ret); - return NULL; - } - /* possibly update local type */ if (ret->type == VIR_STORAGE_TYPE_FILE) { if (stat(ret->path, &st) == 0) { diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 70deaef..1f2ae53 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -247,8 +247,8 @@ struct _virStorageSource { virStorageDriverDataPtr drv; /* metadata about storage image which need separate fields */ - /* Name of the current file as spelled by the user (top level) or - * metadata of the overlay (if this is a backing store). */ + /* Relative path of the backing image from the parent NULL if + * backed by absolute path */ char *relPath; /* Directory to start from if backingStoreRaw is a relative file * name. */ diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index e1adce9..5fd2530 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -116,9 +116,6 @@ testStorageFileGetMetadata(const char *path, } } - if (VIR_STRDUP(ret->relPath, path) < 0) - goto error; - if (!(ret->relDir = mdir_name(path))) { virReportOOMError(); goto error; @@ -371,7 +368,6 @@ testStorageChain(const void *args) while (elt) { char *expect = NULL; char *actual = NULL; - const char *expPath; const char *expRelDir; if (i == data->nfiles) { @@ -379,8 +375,6 @@ testStorageChain(const void *args) goto cleanup; } - expPath = isAbs ? data->files[i]->pathAbs - : data->files[i]->pathRel; expRelDir = isAbs ? data->files[i]->relDirAbs : data->files[i]->relDirRel; if (virAsprintf(&expect, @@ -389,7 +383,7 @@ testStorageChain(const void *args) NULLSTR(data->files[i]->expBackingStoreRaw), data->files[i]->expCapacity, data->files[i]->expEncrypted, - NULLSTR(expPath), + NULLSTR(data->files[i]->pathRel), NULLSTR(expRelDir), data->files[i]->type, data->files[i]->format) < 0 || @@ -767,7 +761,6 @@ mymain(void) /* Raw image, whether with right format or no specified format */ testFileData raw = { - .pathRel = "raw", .pathAbs = canonraw, .path = canonraw, .relDirRel = ".", @@ -788,10 +781,10 @@ mymain(void) /* Qcow2 file with relative raw backing, format provided */ raw.pathAbs = "raw"; + raw.pathRel = "raw"; testFileData qcow2 = { .expBackingStoreRaw = "raw", .expCapacity = 1024, - .pathRel = "qcow2", .pathAbs = canonqcow2, .path = canonqcow2, .relDirRel = ".", @@ -800,7 +793,6 @@ mymain(void) .format = VIR_STORAGE_FILE_QCOW2, }; testFileData qcow2_as_raw = { - .pathRel = "qcow2", .pathAbs = canonqcow2, .path = canonqcow2, .relDirRel = ".", @@ -826,7 +818,7 @@ mymain(void) if (virCommandRun(cmd, NULL) < 0) ret = -1; qcow2.expBackingStoreRaw = absraw; - raw.pathRel = absraw; + raw.pathRel = NULL; raw.pathAbs = absraw; raw.relDirRel = datadir; @@ -846,7 +838,6 @@ mymain(void) testFileData wrap = { .expBackingStoreRaw = absqcow2, .expCapacity = 1024, - .pathRel = "wrap", .pathAbs = abswrap, .path = canonwrap, .relDirRel = ".", @@ -854,7 +845,6 @@ mymain(void) .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; - qcow2.pathRel = absqcow2; qcow2.relDirRel = datadir; TEST_CHAIN(7, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2, (&wrap, &qcow2, &raw), EXP_PASS, @@ -874,14 +864,12 @@ mymain(void) "-b", absqcow2, "wrap", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; - qcow2_as_raw.pathRel = absqcow2; qcow2_as_raw.relDirRel = datadir; /* Qcow2 file with raw as absolute backing, backing format omitted */ testFileData wrap_as_raw = { .expBackingStoreRaw = absqcow2, .expCapacity = 1024, - .pathRel = "wrap", .pathAbs = abswrap, .path = canonwrap, .relDirRel = ".", @@ -903,7 +891,6 @@ mymain(void) if (virCommandRun(cmd, NULL) < 0) ret = -1; qcow2.expBackingStoreRaw = datadir "/bogus"; - qcow2.pathRel = "qcow2"; qcow2.relDirRel = "."; /* Qcow2 file with missing backing file but specified type */ @@ -938,7 +925,6 @@ mymain(void) /* Qcow2 file with backing protocol instead of file */ testFileData nbd = { - .pathRel = "nbd:example.org:6000:exportname=blah", .pathAbs = "nbd:example.org:6000:exportname=blah", .path = "blah", .type = VIR_STORAGE_TYPE_NETWORK, @@ -956,7 +942,6 @@ mymain(void) testFileData qed = { .expBackingStoreRaw = absraw, .expCapacity = 1024, - .pathRel = "qed", .pathAbs = absqed, .path = canonqed, .relDirRel = ".", @@ -965,7 +950,6 @@ mymain(void) .format = VIR_STORAGE_FILE_QED, }; testFileData qed_as_raw = { - .pathRel = "qed", .pathAbs = absqed, .path = canonqed, .relDirRel = ".", @@ -981,7 +965,6 @@ mymain(void) /* directory */ testFileData dir = { - .pathRel = "dir", .pathAbs = absdir, .path = canondir, .relDirRel = ".", @@ -1031,7 +1014,6 @@ mymain(void) testFileData link2 = { .expBackingStoreRaw = "../sub/link1", .expCapacity = 1024, - .pathRel = "sub/link2", .pathAbs = abslink2, .path = canonwrap, .relDirRel = "sub", @@ -1078,7 +1060,6 @@ mymain(void) if (virCommandRun(cmd, NULL) < 0) ret = -1; qcow2.expBackingStoreRaw = "wrap"; - qcow2.pathRel = absqcow2; qcow2.relDirRel = datadir; /* Behavior of an infinite loop chain */ -- 1.9.3
Separately remove the now unused variable. --- tests/virstoragetest.c | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 5fd2530..af565c9 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -283,7 +283,6 @@ struct _testFileData unsigned long long expCapacity; bool expEncrypted; const char *pathRel; - const char *pathAbs; const char *path; const char *relDirRel; const char *relDirAbs; @@ -761,7 +760,6 @@ mymain(void) /* Raw image, whether with right format or no specified format */ testFileData raw = { - .pathAbs = canonraw, .path = canonraw, .relDirRel = ".", .relDirAbs = datadir, @@ -780,12 +778,10 @@ mymain(void) (&raw), ALLOW_PROBE | EXP_PASS); /* Qcow2 file with relative raw backing, format provided */ - raw.pathAbs = "raw"; raw.pathRel = "raw"; testFileData qcow2 = { .expBackingStoreRaw = "raw", .expCapacity = 1024, - .pathAbs = canonqcow2, .path = canonqcow2, .relDirRel = ".", .relDirAbs = datadir, @@ -793,7 +789,6 @@ mymain(void) .format = VIR_STORAGE_FILE_QCOW2, }; testFileData qcow2_as_raw = { - .pathAbs = canonqcow2, .path = canonqcow2, .relDirRel = ".", .relDirAbs = datadir, @@ -819,7 +814,6 @@ mymain(void) ret = -1; qcow2.expBackingStoreRaw = absraw; raw.pathRel = NULL; - raw.pathAbs = absraw; raw.relDirRel = datadir; /* Qcow2 file with raw as absolute backing, backing format provided */ @@ -838,7 +832,6 @@ mymain(void) testFileData wrap = { .expBackingStoreRaw = absqcow2, .expCapacity = 1024, - .pathAbs = abswrap, .path = canonwrap, .relDirRel = ".", .relDirAbs = datadir, @@ -870,7 +863,6 @@ mymain(void) testFileData wrap_as_raw = { .expBackingStoreRaw = absqcow2, .expCapacity = 1024, - .pathAbs = abswrap, .path = canonwrap, .relDirRel = ".", .relDirAbs = datadir, @@ -925,7 +917,6 @@ mymain(void) /* Qcow2 file with backing protocol instead of file */ testFileData nbd = { - .pathAbs = "nbd:example.org:6000:exportname=blah", .path = "blah", .type = VIR_STORAGE_TYPE_NETWORK, .format = VIR_STORAGE_FILE_RAW, @@ -942,7 +933,6 @@ mymain(void) testFileData qed = { .expBackingStoreRaw = absraw, .expCapacity = 1024, - .pathAbs = absqed, .path = canonqed, .relDirRel = ".", .relDirAbs = datadir, @@ -950,7 +940,6 @@ mymain(void) .format = VIR_STORAGE_FILE_QED, }; testFileData qed_as_raw = { - .pathAbs = absqed, .path = canonqed, .relDirRel = ".", .relDirAbs = datadir, @@ -965,7 +954,6 @@ mymain(void) /* directory */ testFileData dir = { - .pathAbs = absdir, .path = canondir, .relDirRel = ".", .relDirAbs = datadir, @@ -1004,7 +992,6 @@ mymain(void) .expBackingStoreRaw = "../raw", .expCapacity = 1024, .pathRel = "../sub/link1", - .pathAbs = "../sub/link1", .path = canonqcow2, .relDirRel = "sub/../sub", .relDirAbs = datadir "/sub/../sub", @@ -1014,7 +1001,6 @@ mymain(void) testFileData link2 = { .expBackingStoreRaw = "../sub/link1", .expCapacity = 1024, - .pathAbs = abslink2, .path = canonwrap, .relDirRel = "sub", .relDirAbs = datadir "/sub", @@ -1022,7 +1008,6 @@ mymain(void) .format = VIR_STORAGE_FILE_QCOW2, }; raw.pathRel = "../raw"; - raw.pathAbs = "../raw"; raw.relDirRel = "sub/../sub/.."; raw.relDirAbs = datadir "/sub/../sub/.."; TEST_CHAIN(15, "sub/link2", abslink2, VIR_STORAGE_FILE_QCOW2, -- 1.9.3
Now that we store only relative names in virStorageSource's member relPath the backingRelative member is obsolete. Remove it and adapt the code to the removal. --- src/util/virstoragefile.c | 4 +--- src/util/virstoragefile.h | 2 -- tests/virstoragetest.c | 23 +++++++++++------------ 3 files changed, 12 insertions(+), 17 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index b33bc1a..6482689 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1590,8 +1590,6 @@ virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent, if (VIR_ALLOC(ret) < 0) return NULL; - ret->backingRelative = true; - /* store relative name */ if (VIR_STRDUP(ret->relPath, parent->backingStoreRaw) < 0) goto error; @@ -2112,7 +2110,7 @@ virStorageFileGetRelativeBackingPath(virStorageSourcePtr from, *relpath = NULL; for (next = from; next; next = next->backingStore) { - if (!next->backingRelative || !next->relPath) { + if (!next->relPath) { ret = 1; goto cleanup; } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 1f2ae53..37f4e66 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -256,8 +256,6 @@ struct _virStorageSource { /* Name of the child backing store recorded in metadata of the * current file. */ char *backingStoreRaw; - /* is backing store identified as relative */ - bool backingRelative; }; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index af565c9..a787cc9 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -570,34 +570,33 @@ testPathRelativePrepare(void) for (i = 0; i < ARRAY_CARDINALITY(backingchain) - 1; i++) { backingchain[i].backingStore = &backingchain[i+1]; + backingchain[i].relPath = NULL; } - backingchain[0].relPath = (char *) "/path/to/some/img"; - backingchain[0].backingRelative = false; + backingchain[0].path = (char *) "/path/to/disk"; + backingchain[1].path = (char *) "/path/to/asdf"; backingchain[1].relPath = (char *) "asdf"; - backingchain[1].backingRelative = true; + backingchain[2].path = (char *) "/path/to/test"; backingchain[2].relPath = (char *) "test"; - backingchain[2].backingRelative = true; + backingchain[3].path = (char *) "/path/to/blah"; backingchain[3].relPath = (char *) "blah"; - backingchain[3].backingRelative = true; - backingchain[4].relPath = (char *) "/path/to/some/other/img"; - backingchain[4].backingRelative = false; + backingchain[4].path = (char *) "/path/to/some/other/img"; + backingchain[5].path = (char *) "/path/to/some/other/relative/in/other/path"; backingchain[5].relPath = (char *) "../relative/in/other/path"; - backingchain[5].backingRelative = true; + backingchain[6].path = (char *) "/path/to/some/other/relative/in/other/test"; backingchain[6].relPath = (char *) "test"; - backingchain[6].backingRelative = true; + backingchain[7].path = (char *) "/path/to/below"; backingchain[7].relPath = (char *) "../../../../../below"; - backingchain[7].backingRelative = true; + backingchain[8].path = (char *) "/path/to/a/little/more/upwards"; backingchain[8].relPath = (char *) "a/little/more/upwards"; - backingchain[8].backingRelative = true; } @@ -626,7 +625,7 @@ testPathRelative(const void *args) if (STRNEQ_NULLABLE(data->expect, actual)) { fprintf(stderr, "relative path resolution from '%s' to '%s': " "expected '%s', got '%s'\n", - data->from->relPath, data->to->relPath, + data->from->path, data->to->path, NULLSTR(data->expect), NULLSTR(actual)); goto cleanup; } -- 1.9.3
libvirt always uses an absolute path to address the top image of an image chain. Our storage test tests also the relative path which won't ever be used. Additionally it makes the test more complicated. --- tests/virstoragetest.c | 79 +++++++++++++------------------------------------- 1 file changed, 20 insertions(+), 59 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index a787cc9..002df68 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -739,17 +739,12 @@ mymain(void) #define VIR_FLATTEN_2(...) __VA_ARGS__ #define VIR_FLATTEN_1(_1) VIR_FLATTEN_2 _1 -#define TEST_CHAIN(id, relstart, absstart, format, chain1, flags1, \ - chain2, flags2, chain3, flags3, chain4, flags4) \ +#define TEST_CHAIN(id, path, format, chain1, flags1, chain2, flags2) \ do { \ - TEST_ONE_CHAIN(#id "a", relstart, format, flags1, \ + TEST_ONE_CHAIN(#id "a", path, format, flags1 | ABS_START, \ VIR_FLATTEN_1(chain1)); \ - TEST_ONE_CHAIN(#id "b", relstart, format, flags2, \ + TEST_ONE_CHAIN(#id "b", path, format, flags2 | ABS_START, \ VIR_FLATTEN_1(chain2)); \ - TEST_ONE_CHAIN(#id "c", absstart, format, flags3 | ABS_START,\ - VIR_FLATTEN_1(chain3)); \ - TEST_ONE_CHAIN(#id "d", absstart, format, flags4 | ABS_START,\ - VIR_FLATTEN_1(chain4)); \ } while (0) /* The actual tests, in several groups. */ @@ -765,14 +760,10 @@ mymain(void) .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_RAW, }; - TEST_CHAIN(1, "raw", absraw, VIR_STORAGE_FILE_RAW, - (&raw), EXP_PASS, - (&raw), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(1, absraw, VIR_STORAGE_FILE_RAW, (&raw), EXP_PASS, (&raw), ALLOW_PROBE | EXP_PASS); - TEST_CHAIN(2, "raw", absraw, VIR_STORAGE_FILE_AUTO, - (&raw), EXP_PASS, - (&raw), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(2, absraw, VIR_STORAGE_FILE_AUTO, (&raw), EXP_PASS, (&raw), ALLOW_PROBE | EXP_PASS); @@ -794,14 +785,10 @@ mymain(void) .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_RAW, }; - TEST_CHAIN(3, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2, &raw), EXP_PASS, - (&qcow2, &raw), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(3, absqcow2, VIR_STORAGE_FILE_QCOW2, (&qcow2, &raw), EXP_PASS, (&qcow2, &raw), ALLOW_PROBE | EXP_PASS); - TEST_CHAIN(4, "qcow2", absqcow2, VIR_STORAGE_FILE_AUTO, - (&qcow2_as_raw), EXP_PASS, - (&qcow2, &raw), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(4, absqcow2, VIR_STORAGE_FILE_AUTO, (&qcow2_as_raw), EXP_PASS, (&qcow2, &raw), ALLOW_PROBE | EXP_PASS); @@ -816,14 +803,10 @@ mymain(void) raw.relDirRel = datadir; /* Qcow2 file with raw as absolute backing, backing format provided */ - TEST_CHAIN(5, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2, &raw), EXP_PASS, - (&qcow2, &raw), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(5, absqcow2, VIR_STORAGE_FILE_QCOW2, (&qcow2, &raw), EXP_PASS, (&qcow2, &raw), ALLOW_PROBE | EXP_PASS); - TEST_CHAIN(6, "qcow2", absqcow2, VIR_STORAGE_FILE_AUTO, - (&qcow2_as_raw), EXP_PASS, - (&qcow2, &raw), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(6, absqcow2, VIR_STORAGE_FILE_AUTO, (&qcow2_as_raw), EXP_PASS, (&qcow2, &raw), ALLOW_PROBE | EXP_PASS); @@ -838,9 +821,7 @@ mymain(void) .format = VIR_STORAGE_FILE_QCOW2, }; qcow2.relDirRel = datadir; - TEST_CHAIN(7, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2, - (&wrap, &qcow2, &raw), EXP_PASS, - (&wrap, &qcow2, &raw), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(7, abswrap, VIR_STORAGE_FILE_QCOW2, (&wrap, &qcow2, &raw), EXP_PASS, (&wrap, &qcow2, &raw), ALLOW_PROBE | EXP_PASS); @@ -868,9 +849,7 @@ mymain(void) .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; - TEST_CHAIN(8, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2, - (&wrap_as_raw, &qcow2_as_raw), EXP_PASS, - (&wrap, &qcow2, &raw), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(8, abswrap, VIR_STORAGE_FILE_QCOW2, (&wrap_as_raw, &qcow2_as_raw), EXP_PASS, (&wrap, &qcow2, &raw), ALLOW_PROBE | EXP_PASS); @@ -885,9 +864,7 @@ mymain(void) qcow2.relDirRel = "."; /* Qcow2 file with missing backing file but specified type */ - TEST_CHAIN(9, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2), EXP_WARN, - (&qcow2), ALLOW_PROBE | EXP_WARN, + TEST_CHAIN(9, absqcow2, VIR_STORAGE_FILE_QCOW2, (&qcow2), EXP_WARN, (&qcow2), ALLOW_PROBE | EXP_WARN); @@ -899,9 +876,7 @@ mymain(void) ret = -1; /* Qcow2 file with missing backing file and no specified type */ - TEST_CHAIN(10, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2), EXP_WARN, - (&qcow2), ALLOW_PROBE | EXP_WARN, + TEST_CHAIN(10, absqcow2, VIR_STORAGE_FILE_QCOW2, (&qcow2), EXP_WARN, (&qcow2), ALLOW_PROBE | EXP_WARN); @@ -922,9 +897,7 @@ mymain(void) .relDirRel = ".", .relDirAbs = ".", }; - TEST_CHAIN(11, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2, &nbd), EXP_PASS, - (&qcow2, &nbd), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(11, absqcow2, VIR_STORAGE_FILE_QCOW2, (&qcow2, &nbd), EXP_PASS, (&qcow2, &nbd), ALLOW_PROBE | EXP_PASS); @@ -945,9 +918,7 @@ mymain(void) .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_RAW, }; - TEST_CHAIN(12, "qed", absqed, VIR_STORAGE_FILE_AUTO, - (&qed_as_raw), EXP_PASS, - (&qed, &raw), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(12, absqed, VIR_STORAGE_FILE_AUTO, (&qed_as_raw), EXP_PASS, (&qed, &raw), ALLOW_PROBE | EXP_PASS); @@ -959,14 +930,10 @@ mymain(void) .type = VIR_STORAGE_TYPE_DIR, .format = VIR_STORAGE_FILE_DIR, }; - TEST_CHAIN(13, "dir", absdir, VIR_STORAGE_FILE_AUTO, - (&dir), EXP_PASS, - (&dir), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(13, absdir, VIR_STORAGE_FILE_AUTO, (&dir), EXP_PASS, (&dir), ALLOW_PROBE | EXP_PASS); - TEST_CHAIN(14, "dir", absdir, VIR_STORAGE_FILE_DIR, - (&dir), EXP_PASS, - (&dir), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(14, absdir, VIR_STORAGE_FILE_DIR, (&dir), EXP_PASS, (&dir), ALLOW_PROBE | EXP_PASS); @@ -1009,9 +976,7 @@ mymain(void) raw.pathRel = "../raw"; raw.relDirRel = "sub/../sub/.."; raw.relDirAbs = datadir "/sub/../sub/.."; - TEST_CHAIN(15, "sub/link2", abslink2, VIR_STORAGE_FILE_QCOW2, - (&link2, &link1, &raw), EXP_PASS, - (&link2, &link1, &raw), ALLOW_PROBE | EXP_PASS, + TEST_CHAIN(15, abslink2, VIR_STORAGE_FILE_QCOW2, (&link2, &link1, &raw), EXP_PASS, (&link2, &link1, &raw), ALLOW_PROBE | EXP_PASS); #endif @@ -1025,9 +990,7 @@ mymain(void) qcow2.expBackingStoreRaw = "qcow2"; /* Behavior of an infinite loop chain */ - TEST_CHAIN(16, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - (&qcow2), EXP_WARN, - (&qcow2), ALLOW_PROBE | EXP_WARN, + TEST_CHAIN(16, absqcow2, VIR_STORAGE_FILE_QCOW2, (&qcow2), EXP_WARN, (&qcow2), ALLOW_PROBE | EXP_WARN); @@ -1047,9 +1010,7 @@ mymain(void) qcow2.relDirRel = datadir; /* Behavior of an infinite loop chain */ - TEST_CHAIN(17, "wrap", abswrap, VIR_STORAGE_FILE_QCOW2, - (&wrap, &qcow2), EXP_WARN, - (&wrap, &qcow2), ALLOW_PROBE | EXP_WARN, + TEST_CHAIN(17, abswrap, VIR_STORAGE_FILE_QCOW2, (&wrap, &qcow2), EXP_WARN, (&wrap, &qcow2), ALLOW_PROBE | EXP_WARN); -- 1.9.3
After we don't test relative paths, remove even more unnecessary cruft from the test code. --- tests/virstoragetest.c | 61 +++++++++++++++----------------------------------- 1 file changed, 18 insertions(+), 43 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 002df68..3c262e2 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -284,8 +284,7 @@ struct _testFileData bool expEncrypted; const char *pathRel; const char *path; - const char *relDirRel; - const char *relDirAbs; + const char *relDir; int type; int format; }; @@ -295,7 +294,6 @@ enum { EXP_FAIL = 1, EXP_WARN = 2, ALLOW_PROBE = 4, - ABS_START = 8, }; struct testChainData @@ -328,7 +326,6 @@ testStorageChain(const void *args) virStorageSourcePtr elt; size_t i = 0; char *broken = NULL; - bool isAbs = !!(data->flags & ABS_START); meta = testStorageFileGetMetadata(data->start, data->format, -1, -1, (data->flags & ALLOW_PROBE) != 0); @@ -367,15 +364,12 @@ testStorageChain(const void *args) while (elt) { char *expect = NULL; char *actual = NULL; - const char *expRelDir; if (i == data->nfiles) { fprintf(stderr, "probed chain was too long\n"); goto cleanup; } - expRelDir = isAbs ? data->files[i]->relDirAbs - : data->files[i]->relDirRel; if (virAsprintf(&expect, testStorageChainFormat, i, NULLSTR(data->files[i]->path), @@ -383,7 +377,7 @@ testStorageChain(const void *args) data->files[i]->expCapacity, data->files[i]->expEncrypted, NULLSTR(data->files[i]->pathRel), - NULLSTR(expRelDir), + NULLSTR(data->files[i]->relDir), data->files[i]->type, data->files[i]->format) < 0 || virAsprintf(&actual, @@ -739,12 +733,10 @@ mymain(void) #define VIR_FLATTEN_2(...) __VA_ARGS__ #define VIR_FLATTEN_1(_1) VIR_FLATTEN_2 _1 -#define TEST_CHAIN(id, path, format, chain1, flags1, chain2, flags2) \ - do { \ - TEST_ONE_CHAIN(#id "a", path, format, flags1 | ABS_START, \ - VIR_FLATTEN_1(chain1)); \ - TEST_ONE_CHAIN(#id "b", path, format, flags2 | ABS_START, \ - VIR_FLATTEN_1(chain2)); \ +#define TEST_CHAIN(id, path, format, chain1, flags1, chain2, flags2) \ + do { \ + TEST_ONE_CHAIN(#id "a", path, format, flags1, VIR_FLATTEN_1(chain1)); \ + TEST_ONE_CHAIN(#id "b", path, format, flags2, VIR_FLATTEN_1(chain2)); \ } while (0) /* The actual tests, in several groups. */ @@ -755,8 +747,7 @@ mymain(void) /* Raw image, whether with right format or no specified format */ testFileData raw = { .path = canonraw, - .relDirRel = ".", - .relDirAbs = datadir, + .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_RAW, }; @@ -773,15 +764,13 @@ mymain(void) .expBackingStoreRaw = "raw", .expCapacity = 1024, .path = canonqcow2, - .relDirRel = ".", - .relDirAbs = datadir, + .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; testFileData qcow2_as_raw = { .path = canonqcow2, - .relDirRel = ".", - .relDirAbs = datadir, + .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_RAW, }; @@ -800,7 +789,6 @@ mymain(void) ret = -1; qcow2.expBackingStoreRaw = absraw; raw.pathRel = NULL; - raw.relDirRel = datadir; /* Qcow2 file with raw as absolute backing, backing format provided */ TEST_CHAIN(5, absqcow2, VIR_STORAGE_FILE_QCOW2, @@ -815,12 +803,10 @@ mymain(void) .expBackingStoreRaw = absqcow2, .expCapacity = 1024, .path = canonwrap, - .relDirRel = ".", - .relDirAbs = datadir, + .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; - qcow2.relDirRel = datadir; TEST_CHAIN(7, abswrap, VIR_STORAGE_FILE_QCOW2, (&wrap, &qcow2, &raw), EXP_PASS, (&wrap, &qcow2, &raw), ALLOW_PROBE | EXP_PASS); @@ -837,15 +823,13 @@ mymain(void) "-b", absqcow2, "wrap", NULL); if (virCommandRun(cmd, NULL) < 0) ret = -1; - qcow2_as_raw.relDirRel = datadir; /* Qcow2 file with raw as absolute backing, backing format omitted */ testFileData wrap_as_raw = { .expBackingStoreRaw = absqcow2, .expCapacity = 1024, .path = canonwrap, - .relDirRel = ".", - .relDirAbs = datadir, + .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; @@ -861,7 +845,6 @@ mymain(void) if (virCommandRun(cmd, NULL) < 0) ret = -1; qcow2.expBackingStoreRaw = datadir "/bogus"; - qcow2.relDirRel = "."; /* Qcow2 file with missing backing file but specified type */ TEST_CHAIN(9, absqcow2, VIR_STORAGE_FILE_QCOW2, @@ -894,8 +877,7 @@ mymain(void) .path = "blah", .type = VIR_STORAGE_TYPE_NETWORK, .format = VIR_STORAGE_FILE_RAW, - .relDirRel = ".", - .relDirAbs = ".", + .relDir = ".", }; TEST_CHAIN(11, absqcow2, VIR_STORAGE_FILE_QCOW2, (&qcow2, &nbd), EXP_PASS, @@ -906,15 +888,13 @@ mymain(void) .expBackingStoreRaw = absraw, .expCapacity = 1024, .path = canonqed, - .relDirRel = ".", - .relDirAbs = datadir, + .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QED, }; testFileData qed_as_raw = { .path = canonqed, - .relDirRel = ".", - .relDirAbs = datadir, + .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_RAW, }; @@ -925,8 +905,7 @@ mymain(void) /* directory */ testFileData dir = { .path = canondir, - .relDirRel = ".", - .relDirAbs = datadir, + .relDir = datadir, .type = VIR_STORAGE_TYPE_DIR, .format = VIR_STORAGE_FILE_DIR, }; @@ -959,8 +938,7 @@ mymain(void) .expCapacity = 1024, .pathRel = "../sub/link1", .path = canonqcow2, - .relDirRel = "sub/../sub", - .relDirAbs = datadir "/sub/../sub", + .relDir = datadir "/sub/../sub", .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; @@ -968,14 +946,12 @@ mymain(void) .expBackingStoreRaw = "../sub/link1", .expCapacity = 1024, .path = canonwrap, - .relDirRel = "sub", - .relDirAbs = datadir "/sub", + .relDir = datadir "/sub", .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; raw.pathRel = "../raw"; - raw.relDirRel = "sub/../sub/.."; - raw.relDirAbs = datadir "/sub/../sub/.."; + raw.relDir = datadir "/sub/../sub/.."; TEST_CHAIN(15, abslink2, VIR_STORAGE_FILE_QCOW2, (&link2, &link1, &raw), EXP_PASS, (&link2, &link1, &raw), ALLOW_PROBE | EXP_PASS); @@ -1007,7 +983,6 @@ mymain(void) if (virCommandRun(cmd, NULL) < 0) ret = -1; qcow2.expBackingStoreRaw = "wrap"; - qcow2.relDirRel = datadir; /* Behavior of an infinite loop chain */ TEST_CHAIN(17, abswrap, VIR_STORAGE_FILE_QCOW2, -- 1.9.3
Store backing chain paths as non-cannonical. The cannonicalization step will be already taken. This will allow to avoid storing unnecessary amounts of data. --- src/util/virstoragefile.c | 33 ++++++--------------------------- tests/virstoragetest.c | 10 +++++----- 2 files changed, 11 insertions(+), 32 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 6482689..02b4c73 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1043,25 +1043,17 @@ virStorageFileGetMetadataFromFD(const char *path, int *backingFormat) { - virStorageSourcePtr ret = NULL; - char *canonPath = NULL; - - if (!(canonPath = canonicalize_file_name(path))) { - virReportSystemError(errno, _("unable to resolve '%s'"), path); - goto cleanup; - } + virStorageSourcePtr ret; - if (!(ret = virStorageFileMetadataNew(canonPath, format))) - goto cleanup; + if (!(ret = virStorageFileMetadataNew(path, format))) + return NULL; if (virStorageFileGetMetadataFromFDInternal(ret, fd, backingFormat) < 0) { virStorageSourceFree(ret); - ret = NULL; + return NULL; } - cleanup: - VIR_FREE(canonPath); return ret; } @@ -1610,15 +1602,6 @@ virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent, virReportOOMError(); goto error; } - - /* XXX we don't currently need to store the canonical path but the - * change would break the test suite. Get rid of this later */ - char *tmp = ret->path; - if (!(ret->path = canonicalize_file_name(tmp))) { - ret->path = tmp; - tmp = NULL; - } - VIR_FREE(tmp); } else { ret->type = VIR_STORAGE_TYPE_NETWORK; @@ -1857,12 +1840,8 @@ virStorageSourceNewFromBackingAbsolute(const char *path) goto error; } - /* XXX we don't currently need to store the canonical path but the - * change would break the test suite. Get rid of this later */ - if (!(ret->path = canonicalize_file_name(path))) { - if (VIR_STRDUP(ret->path, path) < 0) - goto error; - } + if (VIR_STRDUP(ret->path, path) < 0) + goto error; } else { ret->type = VIR_STORAGE_TYPE_NETWORK; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 3c262e2..5ce42d3 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -121,10 +121,8 @@ testStorageFileGetMetadata(const char *path, goto error; } - if (!(ret->path = canonicalize_file_name(path))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "failed to resolve '%s'", path); + if (VIR_STRDUP(ret->path, path) < 0) goto error; - } if (virStorageFileGetMetadata(ret, uid, gid, allow_probe) < 0) goto error; @@ -937,7 +935,7 @@ mymain(void) .expBackingStoreRaw = "../raw", .expCapacity = 1024, .pathRel = "../sub/link1", - .path = canonqcow2, + .path = datadir "/sub/../sub/link1", .relDir = datadir "/sub/../sub", .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, @@ -945,11 +943,13 @@ mymain(void) testFileData link2 = { .expBackingStoreRaw = "../sub/link1", .expCapacity = 1024, - .path = canonwrap, + .path = abslink2, .relDir = datadir "/sub", .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; + + raw.path = datadir "/sub/../sub/../raw"; raw.pathRel = "../raw"; raw.relDir = datadir "/sub/../sub/.."; TEST_CHAIN(15, abslink2, VIR_STORAGE_FILE_QCOW2, -- 1.9.3
On 05/30/2014 02:37 AM, Peter Krempa wrote:
Store backing chain paths as non-cannonical. The cannonicalization step
s/cannon/canon/ twice
will be already taken. This will allow to avoid storing unnecessary amounts of data. --- src/util/virstoragefile.c | 33 ++++++--------------------------- tests/virstoragetest.c | 10 +++++----- 2 files changed, 11 insertions(+), 32 deletions(-)
[I'll review the technical content tomorrow when it's not quite so late for me...] -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The parent directory doesn't necessarily need to be stored after we don't mangle the path stored in the image. Remove it and tweak the code to avoid using it. --- src/storage/storage_driver.c | 11 ++----- src/util/virstoragefile.c | 68 ++++++++++++++++++-------------------------- src/util/virstoragefile.h | 3 -- tests/virstoragetest.c | 21 -------------- 4 files changed, 30 insertions(+), 73 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 9ce3b62..1c8ad1e 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -3082,8 +3082,8 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, virStorageSourcePtr backingStore = NULL; int backingFormat; - VIR_DEBUG("path=%s dir=%s format=%d uid=%d gid=%d probe=%d", - src->path, NULLSTR(src->relDir), src->format, + VIR_DEBUG("path=%s format=%d uid=%d gid=%d probe=%d", + src->path, src->format, (int)uid, (int)gid, allow_probe); /* exit if we can't load information about the current image */ @@ -3189,19 +3189,12 @@ virStorageFileGetMetadata(virStorageSourcePtr src, if (!(cycle = virHashCreate(5, NULL))) return -1; - if (!src->relDir && - !(src->relDir = mdir_name(src->path))) { - virReportOOMError(); - goto cleanup; - } - if (src->format <= VIR_STORAGE_FILE_NONE) src->format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; ret = virStorageFileGetMetadataRecurse(src, uid, gid, allow_probe, cycle); - cleanup: VIR_FREE(canonPath); virHashFree(cycle); return ret; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 02b4c73..2a45e58 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1337,9 +1337,10 @@ virStorageFileChainLookup(virStorageSourcePtr chain, unsigned int idx, const char **parent) { + virStorageSourcePtr prev = NULL; const char *start = chain->path; const char *tmp; - const char *parentDir = "."; + char *parentDir = NULL; bool nameIsFile = virStorageIsFile(name); size_t i; @@ -1369,8 +1370,20 @@ virStorageFileChainLookup(virStorageSourcePtr chain, break; if (nameIsFile && (chain->type == VIR_STORAGE_TYPE_FILE || chain->type == VIR_STORAGE_TYPE_BLOCK)) { + if (prev) { + if (!(parentDir = mdir_name(prev->path))) { + virReportOOMError(); + goto error; + } + } else { + if (VIR_STRDUP(parentDir, ".") < 0) + goto error; + } + int result = virFileRelLinkPointsTo(parentDir, name, chain->path); + + VIR_FREE(parentDir); if (result < 0) goto error; if (result > 0) @@ -1378,7 +1391,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain, } } *parent = chain->path; - parentDir = chain->relDir; + prev = chain; chain = chain->backingStore; i++; } @@ -1517,7 +1530,6 @@ virStorageSourceClearBackingStore(virStorageSourcePtr def) return; VIR_FREE(def->relPath); - VIR_FREE(def->relDir); VIR_FREE(def->backingStoreRaw); /* recursively free backing chain */ @@ -1576,7 +1588,6 @@ virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent, const char *rel) { char *dirname = NULL; - const char *parentdir = ""; virStorageSourcePtr ret; if (VIR_ALLOC(ret) < 0) @@ -1586,23 +1597,20 @@ virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent, if (VIR_STRDUP(ret->relPath, parent->backingStoreRaw) < 0) goto error; - /* XXX Once we get rid of the need to use canonical names in path, we will be - * able to use mdir_name on parent->path instead of using parent->relDir */ - if (STRNEQ(parent->relDir, "/")) - parentdir = parent->relDir; - - if (virAsprintf(&ret->path, "%s/%s", parentdir, rel) < 0) + if (!(dirname = mdir_name(parent->path))) { + virReportOOMError(); goto error; + } - if (virStorageSourceGetActualType(parent) != VIR_STORAGE_TYPE_NETWORK) { - ret->type = VIR_STORAGE_TYPE_FILE; - - /* XXX store the relative directory name for test's sake */ - if (!(ret->relDir = mdir_name(ret->path))) { - virReportOOMError(); + if (STRNEQ(dirname, "/")) { + if (virAsprintf(&ret->path, "%s/%s", dirname, rel) < 0) goto error; - } } else { + if (virAsprintf(&ret->path, "/%s", rel) < 0) + goto error; + } + + if (virStorageSourceGetActualType(parent) == VIR_STORAGE_TYPE_NETWORK) { ret->type = VIR_STORAGE_TYPE_NETWORK; /* copy the host network part */ @@ -1613,12 +1621,9 @@ virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent, if (VIR_STRDUP(ret->volume, parent->volume) < 0) goto error; - - /* XXX store the relative directory name for test's sake */ - if (!(ret->relDir = mdir_name(ret->path))) { - virReportOOMError(); - goto error; - } + } else { + /* set the type to _FILE, the caller shall update it to the actual type */ + ret->type = VIR_STORAGE_TYPE_FILE; } cleanup: @@ -1834,12 +1839,6 @@ virStorageSourceNewFromBackingAbsolute(const char *path) if (virStorageIsFile(path)) { ret->type = VIR_STORAGE_TYPE_FILE; - /* XXX store the relative directory name for test's sake */ - if (!(ret->relDir = mdir_name(path))) { - virReportOOMError(); - goto error; - } - if (VIR_STRDUP(ret->path, path) < 0) goto error; } else { @@ -1853,17 +1852,6 @@ virStorageSourceNewFromBackingAbsolute(const char *path) if (virStorageSourceParseBackingColon(ret, path) < 0) goto error; } - - /* XXX fill relative path so that relative names work with network storage too */ - if (ret->path) { - if (!(ret->relDir = mdir_name(ret->path))) { - virReportOOMError(); - goto error; - } - } else { - if (VIR_STRDUP(ret->relDir, "") < 0) - goto error; - } } return ret; diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 37f4e66..a83e168 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -250,9 +250,6 @@ struct _virStorageSource { /* Relative path of the backing image from the parent NULL if * backed by absolute path */ char *relPath; - /* Directory to start from if backingStoreRaw is a relative file - * name. */ - char *relDir; /* Name of the child backing store recorded in metadata of the * current file. */ char *backingStoreRaw; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 5ce42d3..a09648c 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -116,11 +116,6 @@ testStorageFileGetMetadata(const char *path, } } - if (!(ret->relDir = mdir_name(path))) { - virReportOOMError(); - goto error; - } - if (VIR_STRDUP(ret->path, path) < 0) goto error; @@ -282,7 +277,6 @@ struct _testFileData bool expEncrypted; const char *pathRel; const char *path; - const char *relDir; int type; int format; }; @@ -311,7 +305,6 @@ static const char testStorageChainFormat[] = "capacity: %lld\n" "encryption: %d\n" "relPath:%s\n" - "relDir:%s\n" "type:%d\n" "format:%d\n"; @@ -375,7 +368,6 @@ testStorageChain(const void *args) data->files[i]->expCapacity, data->files[i]->expEncrypted, NULLSTR(data->files[i]->pathRel), - NULLSTR(data->files[i]->relDir), data->files[i]->type, data->files[i]->format) < 0 || virAsprintf(&actual, @@ -385,7 +377,6 @@ testStorageChain(const void *args) elt->capacity, !!elt->encryption, NULLSTR(elt->relPath), - NULLSTR(elt->relDir), elt->type, elt->format) < 0) { VIR_FREE(expect); @@ -745,7 +736,6 @@ mymain(void) /* Raw image, whether with right format or no specified format */ testFileData raw = { .path = canonraw, - .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_RAW, }; @@ -762,13 +752,11 @@ mymain(void) .expBackingStoreRaw = "raw", .expCapacity = 1024, .path = canonqcow2, - .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; testFileData qcow2_as_raw = { .path = canonqcow2, - .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_RAW, }; @@ -801,7 +789,6 @@ mymain(void) .expBackingStoreRaw = absqcow2, .expCapacity = 1024, .path = canonwrap, - .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; @@ -827,7 +814,6 @@ mymain(void) .expBackingStoreRaw = absqcow2, .expCapacity = 1024, .path = canonwrap, - .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; @@ -875,7 +861,6 @@ mymain(void) .path = "blah", .type = VIR_STORAGE_TYPE_NETWORK, .format = VIR_STORAGE_FILE_RAW, - .relDir = ".", }; TEST_CHAIN(11, absqcow2, VIR_STORAGE_FILE_QCOW2, (&qcow2, &nbd), EXP_PASS, @@ -886,13 +871,11 @@ mymain(void) .expBackingStoreRaw = absraw, .expCapacity = 1024, .path = canonqed, - .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QED, }; testFileData qed_as_raw = { .path = canonqed, - .relDir = datadir, .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_RAW, }; @@ -903,7 +886,6 @@ mymain(void) /* directory */ testFileData dir = { .path = canondir, - .relDir = datadir, .type = VIR_STORAGE_TYPE_DIR, .format = VIR_STORAGE_FILE_DIR, }; @@ -936,7 +918,6 @@ mymain(void) .expCapacity = 1024, .pathRel = "../sub/link1", .path = datadir "/sub/../sub/link1", - .relDir = datadir "/sub/../sub", .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; @@ -944,14 +925,12 @@ mymain(void) .expBackingStoreRaw = "../sub/link1", .expCapacity = 1024, .path = abslink2, - .relDir = datadir "/sub", .type = VIR_STORAGE_TYPE_FILE, .format = VIR_STORAGE_FILE_QCOW2, }; raw.path = datadir "/sub/../sub/../raw"; raw.pathRel = "../raw"; - raw.relDir = datadir "/sub/../sub/.."; TEST_CHAIN(15, abslink2, VIR_STORAGE_FILE_QCOW2, (&link2, &link1, &raw), EXP_PASS, (&link2, &link1, &raw), ALLOW_PROBE | EXP_PASS); -- 1.9.3
To allow changing the name that is recorded in the overlay of the TOP image used in a block commit operation, we need to specify the backing name to qemu. This is done via the "backing-file" attribute to the block-commit commad. --- src/qemu/qemu_driver.c | 1 + src/qemu/qemu_monitor.c | 9 ++++++--- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 2 ++ src/qemu/qemu_monitor_json.h | 1 + tests/qemumonitorjsontest.c | 2 +- 6 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 59185c6..0619156 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15529,6 +15529,7 @@ qemuDomainBlockCommit(virDomainPtr dom, ret = qemuMonitorBlockCommit(priv->mon, device, top && !topIndex ? top : topSource->path, base && !baseIndex ? base : baseSource->path, + NULL, bandwidth); qemuDomainObjExitMonitor(driver, vm); diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 912bea1..590081c 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3234,13 +3234,15 @@ qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr actions) int qemuMonitorBlockCommit(qemuMonitorPtr mon, const char *device, const char *top, const char *base, + const char *backingName, unsigned long bandwidth) { int ret = -1; unsigned long long speed; - VIR_DEBUG("mon=%p, device=%s, top=%s, base=%s, bandwidth=%ld", - mon, device, top, base, bandwidth); + VIR_DEBUG("mon=%p, device=%s, top=%s, base=%s, backingName=%s, " + "bandwidth=%ld", + mon, device, top, base, backingName, bandwidth); /* Convert bandwidth MiB to bytes - unfortunately the JSON QMP protocol is * limited to LLONG_MAX also for unsigned values */ @@ -3254,7 +3256,8 @@ qemuMonitorBlockCommit(qemuMonitorPtr mon, const char *device, speed <<= 20; if (mon->json) - ret = qemuMonitorJSONBlockCommit(mon, device, top, base, speed); + ret = qemuMonitorJSONBlockCommit(mon, device, top, base, + backingName, speed); else virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("block-commit requires JSON monitor")); diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index de724bf..2cac1b3 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -662,6 +662,7 @@ int qemuMonitorBlockCommit(qemuMonitorPtr mon, const char *device, const char *top, const char *base, + const char *backingName, unsigned long bandwidth) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index d78bd30..8d69b96 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3451,6 +3451,7 @@ qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions) int qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, const char *device, const char *top, const char *base, + const char *backingName, unsigned long long speed) { int ret = -1; @@ -3462,6 +3463,7 @@ qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, const char *device, "U:speed", speed, "s:top", top, "s:base", base, + "S:backing-file", backingName, NULL); if (!cmd) return -1; diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 5dda0ba..c30803f 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -261,6 +261,7 @@ int qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, const char *device, const char *top, const char *base, + const char *backingName, unsigned long long bandwidth) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 47d7481..e618c67 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1164,7 +1164,7 @@ GEN_TEST_FUNC(qemuMonitorJSONAddDevice, "some_dummy_devicestr") GEN_TEST_FUNC(qemuMonitorJSONSetDrivePassphrase, "vda", "secret_passhprase") GEN_TEST_FUNC(qemuMonitorJSONDriveMirror, "vdb", "/foo/bar", NULL, 1024, VIR_DOMAIN_BLOCK_REBASE_SHALLOW | VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT) -GEN_TEST_FUNC(qemuMonitorJSONBlockCommit, "vdb", "/foo/bar1", "/foo/bar2", 1024) +GEN_TEST_FUNC(qemuMonitorJSONBlockCommit, "vdb", "/foo/bar1", "/foo/bar2", NULL, 1024) GEN_TEST_FUNC(qemuMonitorJSONDrivePivot, "vdb", NULL, NULL) GEN_TEST_FUNC(qemuMonitorJSONScreendump, "/foo/bar") GEN_TEST_FUNC(qemuMonitorJSONOpenGraphics, "spice", "spicefd", false) -- 1.9.3
On 05/30/2014 02:37 AM, Peter Krempa wrote:
To allow changing the name that is recorded in the overlay of the TOP image used in a block commit operation, we need to specify the backing name to qemu. This is done via the "backing-file" attribute to the block-commit commad. --- src/qemu/qemu_driver.c | 1 + src/qemu/qemu_monitor.c | 9 ++++++--- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 2 ++ src/qemu/qemu_monitor_json.h | 1 + tests/qemumonitorjsontest.c | 2 +- 6 files changed, 12 insertions(+), 4 deletions(-)
We need a capability in qemu_capabilities.[ch] first; that way we can give better error messages if qemu is too old to support use of this feature. ACK to this patch; it will be the caller qemu_driver that needs to pay attention to the capability. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/30/2014 02:37 AM, Peter Krempa wrote:
To allow changing the name that is recorded in the overlay of the TOP image used in a block commit operation, we need to specify the backing name to qemu. This is done via the "backing-file" attribute to the block-commit commad.
Oops, missed this on first review. s/commad/command/
--- src/qemu/qemu_driver.c | 1 + src/qemu/qemu_monitor.c | 9 ++++++--- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 2 ++ src/qemu/qemu_monitor_json.h | 1 + tests/qemumonitorjsontest.c | 2 +- 6 files changed, 12 insertions(+), 4 deletions(-)
- VIR_DEBUG("mon=%p, device=%s, top=%s, base=%s, bandwidth=%ld", - mon, device, top, base, bandwidth); + VIR_DEBUG("mon=%p, device=%s, top=%s, base=%s, backingName=%s, " + "bandwidth=%ld", + mon, device, top, base, backingName, bandwidth);
NULLSTR(backingName) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
To allow changing the name that is recorded in the top of the current image chain used in a block pull/rebase operation, we need to specify the backing name to qemu. This is done via the "backing-file" attribute to the block-stream commad. --- src/qemu/qemu_driver.c | 8 ++++---- src/qemu/qemu_migration.c | 6 +++--- src/qemu/qemu_monitor.c | 12 +++++++----- src/qemu/qemu_monitor.h | 3 ++- src/qemu/qemu_monitor_json.c | 15 +++++++++++++++ src/qemu/qemu_monitor_json.h | 1 + 6 files changed, 32 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0619156..72e92cc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14834,7 +14834,7 @@ qemuDomainBlockPivot(virConnectPtr conn, /* Probe the status, if needed. */ if (!disk->mirroring) { qemuDomainObjEnterMonitor(driver, vm); - rc = qemuMonitorBlockJob(priv->mon, device, NULL, 0, &info, + rc = qemuMonitorBlockJob(priv->mon, device, NULL, NULL, 0, &info, BLOCK_JOB_INFO, true); qemuDomainObjExitMonitor(driver, vm); if (rc < 0) @@ -15051,7 +15051,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorBlockJob(priv->mon, device, baseIndex ? baseSource->path : base, - bandwidth, info, mode, async); + NULL, bandwidth, info, mode, async); qemuDomainObjExitMonitor(driver, vm); if (ret < 0) goto endjob; @@ -15091,8 +15091,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, virDomainBlockJobInfo dummy; qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorBlockJob(priv->mon, device, NULL, 0, &dummy, - BLOCK_JOB_INFO, async); + ret = qemuMonitorBlockJob(priv->mon, device, NULL, NULL, 0, + &dummy, BLOCK_JOB_INFO, async); qemuDomainObjExitMonitor(driver, vm); if (ret <= 0) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 5754f73..327679d 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1308,7 +1308,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, _("canceled by client")); goto error; } - mon_ret = qemuMonitorBlockJob(priv->mon, diskAlias, NULL, 0, + mon_ret = qemuMonitorBlockJob(priv->mon, diskAlias, NULL, NULL, 0, &info, BLOCK_JOB_INFO, true); qemuDomainObjExitMonitor(driver, vm); @@ -1360,7 +1360,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, continue; if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT) == 0) { - if (qemuMonitorBlockJob(priv->mon, diskAlias, NULL, 0, + if (qemuMonitorBlockJob(priv->mon, diskAlias, NULL, NULL, 0, NULL, BLOCK_JOB_ABORT, true) < 0) { VIR_WARN("Unable to cancel block-job on '%s'", diskAlias); } @@ -1426,7 +1426,7 @@ qemuMigrationCancelDriveMirror(qemuMigrationCookiePtr mig, QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) goto cleanup; - if (qemuMonitorBlockJob(priv->mon, diskAlias, NULL, 0, + if (qemuMonitorBlockJob(priv->mon, diskAlias, NULL, NULL, 0, NULL, BLOCK_JOB_ABORT, true) < 0) VIR_WARN("Unable to stop block job on %s", diskAlias); qemuDomainObjExitMonitor(driver, vm); diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 590081c..df5dc3a 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3354,6 +3354,7 @@ int qemuMonitorScreendump(qemuMonitorPtr mon, int qemuMonitorBlockJob(qemuMonitorPtr mon, const char *device, const char *base, + const char *backingName, unsigned long bandwidth, virDomainBlockJobInfoPtr info, qemuMonitorBlockJobCmd mode, @@ -3362,9 +3363,10 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon, int ret = -1; unsigned long long speed; - VIR_DEBUG("mon=%p, device=%s, base=%s, bandwidth=%luM, info=%p, mode=%o, " - "modern=%d", mon, device, NULLSTR(base), bandwidth, info, mode, - modern); + VIR_DEBUG("mon=%p, device=%s, base=%s, backingName=%s, bandwidth=%luM, " + "info=%p, mode=%o, modern=%d", + mon, device, NULLSTR(base), NULLSTR(backingName), + bandwidth, info, mode, modern); /* Convert bandwidth MiB to bytes - unfortunately the JSON QMP protocol is * limited to LLONG_MAX also for unsigned values */ @@ -3378,8 +3380,8 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon, speed <<= 20; if (mon->json) - ret = qemuMonitorJSONBlockJob(mon, device, base, speed, info, mode, - modern); + ret = qemuMonitorJSONBlockJob(mon, device, base, backingName, + speed, info, mode, modern); else virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("block jobs require JSON monitor")); diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 2cac1b3..67d00d7 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -691,7 +691,8 @@ typedef enum { int qemuMonitorBlockJob(qemuMonitorPtr mon, const char *device, - const char *back, + const char *base, + const char *backingName, unsigned long bandwidth, virDomainBlockJobInfoPtr info, qemuMonitorBlockJobCmd mode, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 8d69b96..b18be52 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3759,6 +3759,7 @@ int qemuMonitorJSONBlockJob(qemuMonitorPtr mon, const char *device, const char *base, + const char *backingName, unsigned long long speed, virDomainBlockJobInfoPtr info, qemuMonitorBlockJobCmd mode, @@ -3774,6 +3775,19 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon, _("only modern block pull supports base: %s"), base); return -1; } + + if (backingName && mode != BLOCK_JOB_PULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("backing name is supported only for block pull")); + return -1; + } + + if (backingName && !base) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("backing name requires a base image")); + return -1; + } + if (speed && mode == BLOCK_JOB_PULL && !modern) { virReportError(VIR_ERR_INTERNAL_ERROR, _("only modern block pull supports speed: %llu"), @@ -3808,6 +3822,7 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon, "s:device", device, "P:speed", speed, "S:base", base, + "S:backing-file", backingName, NULL); break; } diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index c30803f..434d702 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -284,6 +284,7 @@ int qemuMonitorJSONScreendump(qemuMonitorPtr mon, int qemuMonitorJSONBlockJob(qemuMonitorPtr mon, const char *device, const char *base, + const char *backingName, unsigned long long speed, virDomainBlockJobInfoPtr info, qemuMonitorBlockJobCmd mode, -- 1.9.3
On 05/30/2014 02:37 AM, Peter Krempa wrote:
To allow changing the name that is recorded in the top of the current image chain used in a block pull/rebase operation, we need to specify the backing name to qemu. This is done via the "backing-file" attribute to the block-stream commad.
s/commad/command/
--- src/qemu/qemu_driver.c | 8 ++++---- src/qemu/qemu_migration.c | 6 +++--- src/qemu/qemu_monitor.c | 12 +++++++----- src/qemu/qemu_monitor.h | 3 ++- src/qemu/qemu_monitor_json.c | 15 +++++++++++++++ src/qemu/qemu_monitor_json.h | 1 + 6 files changed, 32 insertions(+), 13 deletions(-)
Again, I think the qemu_capability.[ch] change is needed as a separate commit; but you can get away with the same capability for both block-commit and block-pull since both commands are learning the feature in the same qemu series. Not sure if the capability needs to come first, or if you have it later in the series. This patch is okay (although this and 30/36 should probably wait until Jeff's series is actually committed into qemu.git).
@@ -3759,6 +3759,7 @@ int qemuMonitorJSONBlockJob(qemuMonitorPtr mon,
+ + if (backingName && mode != BLOCK_JOB_PULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("backing name is supported only for block pull")); + return -1; + }
[side note - I honestly don't know why we tried to cram so much into qemuMonitorJSONBlockJob through the entire stack; it might have been simpler to have one qemu_monitor.h entry point per QMP command, rather than trying to multiplex. But it may not be worth the refactor now] If these were separate functions instead of multiplexed through a mode argument, then you wouldn't need a check like this (since the check will never fail unless we introduce a bug in qemu_driver.c). ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Introduce flag for the block commit API to allow the commit operation to leave the chain relatively addressed. Also adds a virsh switch to enable this behavior. --- include/libvirt/libvirt.h.in | 4 ++++ tools/virsh-domain.c | 7 +++++++ tools/virsh.pod | 6 ++++-- 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 260c971..2df7fc8 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2588,6 +2588,10 @@ typedef enum { VIR_DOMAIN_BLOCK_COMMIT_DELETE = 1 << 1, /* Delete any files that are now invalid after their contents have been committed */ + VIR_DOMAIN_BLOCK_COMMIT_RELATIVE = 1 << 2, /* try to keep the backing chain + relative if the components + removed by the commit are + already relative */ } virDomainBlockCommitFlags; int virDomainBlockCommit(virDomainPtr dom, const char *disk, const char *base, diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 84a6706..5c3a142 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1492,6 +1492,8 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, flags |= VIR_DOMAIN_BLOCK_COMMIT_SHALLOW; if (vshCommandOptBool(cmd, "delete")) flags |= VIR_DOMAIN_BLOCK_COMMIT_DELETE; + if (vshCommandOptBool(cmd, "keep-relative")) + flags |= VIR_DOMAIN_BLOCK_COMMIT_RELATIVE; ret = virDomainBlockCommit(dom, path, base, top, bandwidth, flags); break; case VSH_CMD_BLOCK_JOB_COPY: @@ -1612,6 +1614,11 @@ static const vshCmdOptDef opts_block_commit[] = { .type = VSH_OT_BOOL, .help = N_("with --wait, don't wait for cancel to finish") }, + {.name = "keep-relative", + .type = VSH_OT_BOOL, + .help = N_("keep the backing chain relative if it was relatively " + "referenced if it was before") + }, {.name = NULL} }; diff --git a/tools/virsh.pod b/tools/virsh.pod index 02671b4..03b94a5 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -767,7 +767,7 @@ address of virtual interface (such as I<detach-interface> or I<domif-setlink>) will accept the MAC address printed by this command. =item B<blockcommit> I<domain> I<path> [I<bandwidth>] -{[I<base>] | [I<--shallow>]} [I<top>] [I<--delete>] +{[I<base>] | [I<--shallow>]} [I<top>] [I<--delete>] [I<--keep-relative>] [I<--wait> [I<--verbose>] [I<--timeout> B<seconds>] [I<--async>]] Reduce the length of a backing image chain, by committing changes at the @@ -779,7 +779,9 @@ I<--shallow> can be used instead of I<base> to specify the immediate backing file of the resulting top image to be committed. The files being committed are rendered invalid, possibly as soon as the operation starts; using the I<--delete> flag will remove these files at the successful -completion of the commit operation. +completion of the commit operation. Using the I<--keep-relative> flag +will try to keep the backing chain names relative (if they were +relative before). By default, this command returns as soon as possible, and data for the entire disk is committed in the background; the progress of the -- 1.9.3
On 05/30/2014 02:37 AM, Peter Krempa wrote:
Introduce flag for the block commit API to allow the commit operation to leave the chain relatively addressed. Also adds a virsh switch to enable this behavior. --- include/libvirt/libvirt.h.in | 4 ++++ tools/virsh-domain.c | 7 +++++++ tools/virsh.pod | 6 ++++-- 3 files changed, 15 insertions(+), 2 deletions(-)
}, + {.name = "keep-relative", + .type = VSH_OT_BOOL, + .help = N_("keep the backing chain relative if it was relatively " + "referenced if it was before")
s/if it was before/before/ I'd also like to see the flag mentioned in the doc text of libvirt.c. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Introduce flag for the block rebase API to allow the rebase operation to leave the chain relatively addressed. Also adds a virsh switch to enable this behavior. --- include/libvirt/libvirt.h.in | 2 ++ tools/virsh-domain.c | 22 +++++++++++++++++++--- tools/virsh.pod | 4 ++++ 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 2df7fc8..a4fe175 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2571,6 +2571,8 @@ typedef enum { file for a copy */ VIR_DOMAIN_BLOCK_REBASE_COPY_RAW = 1 << 2, /* Make destination file raw */ VIR_DOMAIN_BLOCK_REBASE_COPY = 1 << 3, /* Start a copy job */ + VIR_DOMAIN_BLOCK_REBASE_RELATIVE = 1 << 4, /* Keep backing chain relative + if possible */ } virDomainBlockRebaseFlags; int virDomainBlockRebase(virDomainPtr dom, const char *disk, diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5c3a142..74473a8 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1479,10 +1479,14 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, case VSH_CMD_BLOCK_JOB_PULL: if (vshCommandOptStringReq(ctl, cmd, "base", &base) < 0) goto cleanup; - if (base) - ret = virDomainBlockRebase(dom, path, base, bandwidth, 0); - else + if (base) { + if (vshCommandOptBool(cmd, "keep-relative")) + flags |= VIR_DOMAIN_BLOCK_REBASE_RELATIVE; + + ret = virDomainBlockRebase(dom, path, base, bandwidth, flags); + } else { ret = virDomainBlockPull(dom, path, bandwidth, 0); + } break; case VSH_CMD_BLOCK_JOB_COMMIT: if (vshCommandOptStringReq(ctl, cmd, "base", &base) < 0 || @@ -2071,6 +2075,11 @@ static const vshCmdOptDef opts_block_pull[] = { .type = VSH_OT_BOOL, .help = N_("with --wait, don't wait for cancel to finish") }, + {.name = "keep-relative", + .type = VSH_OT_BOOL, + .help = N_("keep the backing chain relative if it was relatively " + "referenced if it was before") + }, {.name = NULL} }; @@ -2091,6 +2100,13 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) bool quit = false; int abort_flags = 0; + if (vshCommandOptBool(cmd, "keep-relative") && + !vshCommandOptBool(cmd, "base")) { + vshError(ctl, "%s", _("--keep-relative is supported only with partial " + "pull operations with --base specified")); + return false; + } + if (blocking) { if (vshCommandOptTimeoutToMs(ctl, cmd, &timeout) < 0) return false; diff --git a/tools/virsh.pod b/tools/virsh.pod index 03b94a5..e252d62 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -844,6 +844,7 @@ I<bandwidth> specifies copying bandwidth limit in MiB/s. =item B<blockpull> I<domain> I<path> [I<bandwidth>] [I<base>] [I<--wait> [I<--verbose>] [I<--timeout> B<seconds>] [I<--async>]] +[I<--keep-relative>] Populate a disk from its backing image chain. By default, this command flattens the entire chain; but if I<base> is specified, containing the @@ -863,6 +864,9 @@ is triggered, I<--async> will return control to the user as fast as possible, otherwise the command may continue to block a little while longer until the job is done cleaning up. +Using the I<--keep-relative> flag will try to keep the backing chain names +relative (if they were relative before). + I<path> specifies fully-qualified path of the disk; it corresponds to a unique target name (<target dev='name'/>) or source file (<source file='name'/>) for one of the disk devices attached to I<domain> (see -- 1.9.3
On 05/30/2014 02:37 AM, Peter Krempa wrote:
Introduce flag for the block rebase API to allow the rebase operation to leave the chain relatively addressed. Also adds a virsh switch to enable this behavior. --- include/libvirt/libvirt.h.in | 2 ++ tools/virsh-domain.c | 22 +++++++++++++++++++--- tools/virsh.pod | 4 ++++ 3 files changed, 25 insertions(+), 3 deletions(-)
Again, missing doc changes in libvirt.c mentioning the flag.
- else + if (base) { + if (vshCommandOptBool(cmd, "keep-relative")) + flags |= VIR_DOMAIN_BLOCK_REBASE_RELATIVE;
Why not parse this flag unconditionally, to check that the code has a sane error path if the flag is present but base was not specified? In other words, filtering at the virsh level is too high when it comes to validating lower levels.
+ {.name = "keep-relative", + .type = VSH_OT_BOOL, + .help = N_("keep the backing chain relative if it was relatively " + "referenced if it was before")
s/if it was before/before/
@@ -2091,6 +2100,13 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) bool quit = false; int abort_flags = 0;
+ if (vshCommandOptBool(cmd, "keep-relative") && + !vshCommandOptBool(cmd, "base")) { + vshError(ctl, "%s", _("--keep-relative is supported only with partial " + "pull operations with --base specified"));
Again, let the lower level flag this, to prove that error message is sane. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
This command allows to change the backing file name recorded in the metadata of a qcow (or other) image. The capability also notifies that the "block-stream" and "block-commit" commands understand the "backing-file" attribute. --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + 2 files changed, 3 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 08c3d04..05613e5 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -256,6 +256,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "usb-kbd", /* 165 */ "host-pci-multidomain", "msg-timestamp", + "change-backing-file", ); @@ -1382,6 +1383,7 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = { { "blockdev-snapshot-sync", QEMU_CAPS_DISK_SNAPSHOT }, { "add-fd", QEMU_CAPS_ADD_FD }, { "nbd-server-start", QEMU_CAPS_NBD_SERVER }, + { "change-backing-file", QEMU_CAPS_CHANGE_BACKING_FILE }, }; struct virQEMUCapsStringFlags virQEMUCapsEvents[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index d755caa..f6bca5d 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -206,6 +206,7 @@ enum virQEMUCapsFlags { QEMU_CAPS_DEVICE_USB_KBD = 165, /* -device usb-kbd */ QEMU_CAPS_HOST_PCI_MULTIDOMAIN = 166, /* support domain > 0 in host pci address */ QEMU_CAPS_MSG_TIMESTAMP = 167, /* -msg timestamp */ + QEMU_CAPS_CHANGE_BACKING_FILE = 168, /* change namen of backing file in metadata */ QEMU_CAPS_LAST, /* this must always be the last item */ }; -- 1.9.3
On 05/30/2014 02:37 AM, Peter Krempa wrote:
This command allows to change the backing file name recorded in the metadata of a qcow (or other) image. The capability also notifies that the "block-stream" and "block-commit" commands understand the "backing-file" attribute.
It might be better to rebase this ahead of 30 and 31. Also, where does the qemu_driver code actually honor the new flags from 32 and 33 in comparison to whether this capability bit is set?
QEMU_CAPS_HOST_PCI_MULTIDOMAIN = 166, /* support domain > 0 in host pci address */ QEMU_CAPS_MSG_TIMESTAMP = 167, /* -msg timestamp */ + QEMU_CAPS_CHANGE_BACKING_FILE = 168, /* change namen of backing file in metadata */
s/namen/name/ Wait for Jeff's series to actually hit qemu.git before pushing this one. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Now that we are able to select images from the backing chain via indexed access we should also convert possible network sources to qemu-compatible strings before passing them to qemu. --- src/qemu/qemu_driver.c | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 72e92cc..424be4a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15425,8 +15425,12 @@ qemuDomainBlockCommit(virDomainPtr dom, unsigned int baseIndex = 0; const char *top_parent = NULL; bool clean_access = false; + char *topPath = NULL; + char *basePath = NULL; + char *backingPath = NULL; - virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW, -1); + virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW | + VIR_DOMAIN_BLOCK_COMMIT_RELATIVE, -1); if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; @@ -15520,6 +15524,26 @@ qemuDomainBlockCommit(virDomainPtr dom, VIR_DISK_CHAIN_READ_WRITE) < 0)) goto endjob; + if (qemuGetDriveSourceString(topSource, NULL, &topPath) < 0) + goto endjob; + + if (qemuGetDriveSourceString(baseSource, NULL, &basePath) < 0) + goto endjob; + + if (flags & VIR_DOMAIN_BLOCK_COMMIT_RELATIVE) { + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CHANGE_BACKING_FILE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this qemu doesn't support relative blockpull")); + goto endjob; + } + + if (virStorageFileGetRelativeBackingPath(topSource, baseSource, + &backingPath) < 0) + goto endjob; + + /* XXX should we error out? */ + } + /* Start the commit operation. Pass the user's original spelling, * if any, through to qemu, since qemu may behave differently * depending on whether the input was specified as relative or @@ -15527,9 +15551,7 @@ qemuDomainBlockCommit(virDomainPtr dom, * thing if the user specified a relative name). */ qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorBlockCommit(priv->mon, device, - top && !topIndex ? top : topSource->path, - base && !baseIndex ? base : baseSource->path, - NULL, + topPath, basePath, backingPath, bandwidth); qemuDomainObjExitMonitor(driver, vm); @@ -15547,6 +15569,9 @@ qemuDomainBlockCommit(virDomainPtr dom, vm = NULL; cleanup: + VIR_FREE(topPath); + VIR_FREE(basePath); + VIR_FREE(backingPath); VIR_FREE(device); if (vm) virObjectUnlock(vm); -- 1.9.3
Now that we are able to select images from the backing chain via indexed access we should also convert possible network sources to qemu-compatible strings before passing them to qemu. --- src/qemu/qemu_driver.c | 43 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 424be4a..feef9fa 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14980,6 +14980,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, virDomainDiskDefPtr disk; virStorageSourcePtr baseSource = NULL; unsigned int baseIndex = 0; + char *basePath = NULL; + char *backingPath = NULL; if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -14987,6 +14989,13 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, goto cleanup; } + if (flags & VIR_DOMAIN_BLOCK_REBASE_RELATIVE && !base) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("flag VIR_DOMAIN_BLOCK_REBASE_RELATIVE is valid only " + " with non-null base ")); + goto cleanup; + } + priv = vm->privateData; if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC)) { async = true; @@ -15048,10 +15057,33 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, base, baseIndex, NULL)))) goto endjob; + if (baseSource) { + if (qemuGetDriveSourceString(baseSource, NULL, &basePath) < 0) + goto endjob; + + if (flags & VIR_DOMAIN_BLOCK_REBASE_RELATIVE) { + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CHANGE_BACKING_FILE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this QEMU binary doesn't support relative " + "block pull/rebase")); + goto endjob; + } + + if (disk->src.backingStore == baseSource) { + if (VIR_STRDUP(backingPath, baseSource->relPath) < 0) + goto endjob; + } else { + if (virStorageFileGetRelativeBackingPath(disk->src.backingStore, + baseSource, + &backingPath) < 0) + goto endjob; + } + } + } + qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorBlockJob(priv->mon, device, - baseIndex ? baseSource->path : base, - NULL, bandwidth, info, mode, async); + ret = qemuMonitorBlockJob(priv->mon, device, basePath, backingPath, + bandwidth, info, mode, async); qemuDomainObjExitMonitor(driver, vm); if (ret < 0) goto endjob; @@ -15121,6 +15153,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, } cleanup: + VIR_FREE(basePath); + VIR_FREE(backingPath); VIR_FREE(device); if (vm) virObjectUnlock(vm); @@ -15361,7 +15395,8 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, virCheckFlags(VIR_DOMAIN_BLOCK_REBASE_SHALLOW | VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT | VIR_DOMAIN_BLOCK_REBASE_COPY | - VIR_DOMAIN_BLOCK_REBASE_COPY_RAW, -1); + VIR_DOMAIN_BLOCK_REBASE_COPY_RAW | + VIR_DOMAIN_BLOCK_REBASE_RELATIVE, -1); if (!(vm = qemuDomObjFromDomain(dom))) return -1; -- 1.9.3
participants (3)
-
Eric Blake -
Peter Krempa -
Roman Bogorodskiy