[libvirt] [PATCHv2 00/33] Gluster backing chains and relative block commit/pull
Hi, this is my second take on this topic. Some of the patches are still in a RFC status as the last six patches require qemu support which is still on review upstream. The rest should be fine as is even without the qemu bits. Peter Krempa (33): qemu: process: Refresh backing chain info when reconnecting to qemu qemu: Make qemuDomainPrepareDiskChainElement aware of remote storage storage: Store gluster volume name separately storage: Rework debugging of storage file access through storage driver conf: Fix domain disk path iterator to work with networked storage storage: Add NONE protocol type for network disks storage: Add support for access to files using provided uid/gid storage: Add storage file API to read file headers 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 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: 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/conf/domain_conf.c | 72 ++- src/libvirt_private.syms | 7 +- src/qemu/qemu_capabilities.c | 8 + src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_command.c | 24 +- src/qemu/qemu_domain.c | 10 +- src/qemu/qemu_driver.c | 160 +++++-- 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/qemu/qemu_process.c | 5 + src/security/virt-aa-helper.c | 2 + src/storage/storage_backend.c | 16 +- src/storage/storage_backend.h | 22 +- src/storage/storage_backend_fs.c | 127 +++++- src/storage/storage_backend_gluster.c | 203 +++++++-- src/storage/storage_driver.c | 329 +++++++++++++- src/storage/storage_driver.h | 15 +- src/util/virstoragefile.c | 818 ++++++++++++++++++++++++---------- src/util/virstoragefile.h | 31 +- src/util/virstring.c | 44 +- src/util/virstring.h | 7 + tests/Makefile.am | 7 +- tests/qemumonitorjsontest.c | 2 +- tests/virstoragetest.c | 290 +++++++++++- tools/virsh-domain.c | 29 +- 31 files changed, 1988 insertions(+), 424 deletions(-) -- 1.9.3
Refresh the disk backing chains when reconnecting to a qemu process after daemon restart. There are a few internal fields that don't get refreshed from the XML. Until we are able to do that, let's reload all the metadata by the backing chain crawler. --- src/qemu/qemu_process.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a83780f..4aa9ca3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3213,6 +3213,11 @@ qemuProcessReconnect(void *opaque) if (qemuTranslateDiskSourcePool(conn, obj->def->disks[i]) < 0) goto error; + /* XXX we should be able to restore all data from XML in the future */ + if (qemuDomainDetermineDiskChain(driver, obj, + obj->def->disks[i], true) < 0) + goto error; + dev.type = VIR_DOMAIN_DEVICE_DISK; dev.data.disk = obj->def->disks[i]; if (qemuAddSharedDevice(driver, &dev, obj->def->name) < 0) -- 1.9.3
On 05/22/2014 07:47 AM, Peter Krempa wrote:
Refresh the disk backing chains when reconnecting to a qemu process after daemon restart. There are a few internal fields that don't get refreshed from the XML. Until we are able to do that, let's reload all the metadata by the backing chain crawler.
Not necessarily ideal. Reading metadata from a file that is also simultaneously opened read-write by qemu might, in super-rare circumstances, hit a race where qemu is in the middle of updating metadata due to the completion of some job that was started before the libvirtd restart. Although qcow2 has a hard cap of header fitting in one cluster (typically 64k) (at least, after CVE-2014-0144, qemu.git commit 24342f2ca), it is still plausible that qemu is updating the qcow2 header via sectors (perhaps as small as 512-byte chunks) rather than by full clusters; and there are scenarios where an update touches two non-adjacent sectors (such as adjusting the backing file name), such that if a reader sees the old content of one sector but the new content of the other, then it is completely broken in interpreting the data. A slightly safer alternative might be using a QMP command to ask qemu what the backing chain currently is when we reconnect - although that may require correlation back to filenames we passed in, since in the face of fd passing, qemu may only know the name "/dev/fdset/NNN" instead of the filename the earlier libvirtd opened and passed in by fd. And that presupposes that qemu can always be trusted (we don't want to introduce a CVE where a guest that compromises qemu into returning bogus QMP results can then cause libvirt to mismanage the host filesystem). So with those points made, a metadata read race is extremely rare, and I see no point in implementing a half-way solution that requires parsing a QMP command if we are eventually going to switch over to full XML tracking anyways. Therefore, since _this_ patch is a clear improvement (even if not theoretically perfect), we shouldn't hold it up waiting for some other solution. ACK as-is.
src/qemu/qemu_process.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a83780f..4aa9ca3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3213,6 +3213,11 @@ qemuProcessReconnect(void *opaque) if (qemuTranslateDiskSourcePool(conn, obj->def->disks[i]) < 0) goto error;
+ /* XXX we should be able to restore all data from XML in the future */
and thanks for the comment to remind us what we should improve.
+ if (qemuDomainDetermineDiskChain(driver, obj, + obj->def->disks[i], true) < 0) + goto error; + dev.type = VIR_DOMAIN_DEVICE_DISK; dev.data.disk = obj->def->disks[i]; if (qemuAddSharedDevice(driver, &dev, obj->def->name) < 0)
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/22/2014 03:20 PM, Eric Blake wrote:
On 05/22/2014 07:47 AM, Peter Krempa wrote:
Refresh the disk backing chains when reconnecting to a qemu process after daemon restart. There are a few internal fields that don't get refreshed from the XML. Until we are able to do that, let's reload all the metadata by the backing chain crawler.
Not necessarily ideal. Reading metadata from a file that is also simultaneously opened read-write by qemu might, in super-rare circumstances, hit a race where qemu is in the middle of updating metadata due to the completion of some job that was started before the libvirtd restart.
For that matter, all of our other places where we reread the backing chain while qemu is still in operation are also suspect, so your patch isn't making it any worse :)
ACK as-is.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Refactor the function to accept a virStorageSourcePtr instead of just the path, add a check to run it only on local storage and fix callers (possibly by using a newly introduced wrapper that wraps a path in the virStorageSource struct for legacy code) --- Notes: Version 2: - V1 acked by Eric with comment cleanup src/qemu/qemu_driver.c | 77 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 49 insertions(+), 28 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6fda50d..d3e14ea 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12020,22 +12020,26 @@ static int qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk, - const char *file, + virStorageSourcePtr elem, qemuDomainDiskChainMode mode) { /* The easiest way to label a single file with the same * permissions it would have as if part of the disk chain is to * temporarily modify the disk in place. */ - char *origsrc = disk->src.path; - int origformat = disk->src.format; - virStorageSourcePtr origchain = disk->src.backingStore; + virStorageSource origdisk; bool origreadonly = disk->readonly; + virQEMUDriverConfigPtr cfg = NULL; int ret = -1; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - disk->src.path = (char *) file; /* casting away const is safe here */ - disk->src.format = VIR_STORAGE_FILE_RAW; - disk->src.backingStore = NULL; + /* XXX Labelling of non-local files isn't currently supported */ + if (virStorageSourceGetActualType(&disk->src) == VIR_STORAGE_TYPE_NETWORK) + return 0; + + cfg = virQEMUDriverGetConfig(driver); + + /* XXX This would be easier when disk->src will be a pointer */ + memcpy(&origdisk, &disk->src, sizeof(origdisk)); + memcpy(&disk->src, elem, sizeof(*elem)); disk->readonly = mode == VIR_DISK_CHAIN_READ_ONLY; if (mode == VIR_DISK_CHAIN_NO_ACCESS) { @@ -12058,9 +12062,7 @@ qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, ret = 0; cleanup: - disk->src.path = origsrc; - disk->src.format = origformat; - disk->src.backingStore = origchain; + memcpy(&disk->src, &origdisk, sizeof(origdisk)); disk->readonly = origreadonly; virObjectUnref(cfg); return ret; @@ -12071,6 +12073,25 @@ qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, * is sent but failed, and number of frozen filesystems on success. If -2 is * returned, FSThaw should be called revert the quiesced status. */ static int +qemuDomainPrepareDiskChainElementPath(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDiskDefPtr disk, + const char *file, + qemuDomainDiskChainMode mode) +{ + virStorageSource src; + + memset(&src, 0, sizeof(src)); + + src.type = VIR_STORAGE_TYPE_FILE; + src.format = VIR_STORAGE_FILE_RAW; + src.path = (char *) file; /* casting away const is safe here */ + + return qemuDomainPrepareDiskChainElement(driver, vm, disk, &src, mode); +} + + +static int qemuDomainSnapshotFSFreeze(virQEMUDriverPtr driver, virDomainObjPtr vm, const char **mountpoints, @@ -12829,9 +12850,9 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, VIR_FORCE_CLOSE(fd); } - if (qemuDomainPrepareDiskChainElement(driver, vm, disk, source, + if (qemuDomainPrepareDiskChainElement(driver, vm, disk, &snap->src, VIR_DISK_CHAIN_READ_WRITE) < 0) { - qemuDomainPrepareDiskChainElement(driver, vm, disk, source, + qemuDomainPrepareDiskChainElement(driver, vm, disk, &snap->src, VIR_DISK_CHAIN_NO_ACCESS); goto cleanup; } @@ -12962,7 +12983,7 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, (persistDisk && VIR_STRDUP(persistSource, source) < 0)) goto cleanup; - qemuDomainPrepareDiskChainElement(driver, vm, disk, disk->src.path, + qemuDomainPrepareDiskChainElement(driver, vm, disk, &disk->src, VIR_DISK_CHAIN_NO_ACCESS); if (need_unlink && virStorageFileStat(&disk->src, &st) == 0 && S_ISREG(st.st_mode) && @@ -15276,10 +15297,10 @@ qemuDomainBlockCopy(virDomainObjPtr vm, if (VIR_STRDUP(mirror, dest) < 0) goto endjob; - if (qemuDomainPrepareDiskChainElement(driver, vm, disk, dest, - VIR_DISK_CHAIN_READ_WRITE) < 0) { - qemuDomainPrepareDiskChainElement(driver, vm, disk, dest, - VIR_DISK_CHAIN_NO_ACCESS); + if (qemuDomainPrepareDiskChainElementPath(driver, vm, disk, dest, + VIR_DISK_CHAIN_READ_WRITE) < 0) { + qemuDomainPrepareDiskChainElementPath(driver, vm, disk, dest, + VIR_DISK_CHAIN_NO_ACCESS); goto endjob; } @@ -15290,8 +15311,8 @@ qemuDomainBlockCopy(virDomainObjPtr vm, virDomainAuditDisk(vm, NULL, dest, "mirror", ret >= 0); qemuDomainObjExitMonitor(driver, vm); if (ret < 0) { - qemuDomainPrepareDiskChainElement(driver, vm, disk, dest, - VIR_DISK_CHAIN_NO_ACCESS); + qemuDomainPrepareDiskChainElementPath(driver, vm, disk, dest, + VIR_DISK_CHAIN_NO_ACCESS); goto endjob; } @@ -15468,12 +15489,12 @@ qemuDomainBlockCommit(virDomainPtr dom, * operation succeeds, but doing that requires tracking the * operation in XML across libvirtd restarts. */ clean_access = true; - if (qemuDomainPrepareDiskChainElement(driver, vm, disk, baseSource->path, + if (qemuDomainPrepareDiskChainElement(driver, vm, disk, baseSource, VIR_DISK_CHAIN_READ_WRITE) < 0 || (top_parent && top_parent != disk->src.path && - qemuDomainPrepareDiskChainElement(driver, vm, disk, - top_parent, - VIR_DISK_CHAIN_READ_WRITE) < 0)) + qemuDomainPrepareDiskChainElementPath(driver, vm, disk, + top_parent, + VIR_DISK_CHAIN_READ_WRITE) < 0)) goto endjob; /* Start the commit operation. Pass the user's original spelling, @@ -15491,12 +15512,12 @@ qemuDomainBlockCommit(virDomainPtr dom, endjob: if (ret < 0 && clean_access) { /* Revert access to read-only, if possible. */ - qemuDomainPrepareDiskChainElement(driver, vm, disk, baseSource->path, + qemuDomainPrepareDiskChainElement(driver, vm, disk, baseSource, VIR_DISK_CHAIN_READ_ONLY); if (top_parent && top_parent != disk->src.path) - qemuDomainPrepareDiskChainElement(driver, vm, disk, - top_parent, - VIR_DISK_CHAIN_READ_ONLY); + qemuDomainPrepareDiskChainElementPath(driver, vm, disk, + top_parent, + VIR_DISK_CHAIN_READ_ONLY); } if (!qemuDomainObjEndJob(driver, vm)) vm = NULL; -- 1.9.3
On 05/22/2014 07:47 AM, Peter Krempa wrote:
Refactor the function to accept a virStorageSourcePtr instead of just the path, add a check to run it only on local storage and fix callers (possibly by using a newly introduced wrapper that wraps a path in the virStorageSource struct for legacy code) ---
Notes: Version 2: - V1 acked by Eric with comment cleanup
ACK still holds. This is a large series; you should go ahead and push ACK'd patches (especially if it is before the 1.2.5 freeze) rather than waiting for a full-series review, unless stated explicitly in a patch, or until I hit something that requires rework that might affect later patches. [that, and I have patches for live commit that I'm waiting to rebase on top of yours...]
qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk, - const char *file, + virStorageSourcePtr elem, qemuDomainDiskChainMode mode) { /* The easiest way to label a single file with the same * permissions it would have as if part of the disk chain is to * temporarily modify the disk in place. */ - char *origsrc = disk->src.path; - int origformat = disk->src.format; - virStorageSourcePtr origchain = disk->src.backingStore; + virStorageSource origdisk;
+ + /* XXX This would be easier when disk->src will be a pointer */ + memcpy(&origdisk, &disk->src, sizeof(origdisk));
Yep, this is definitely going to conflict with my proposed patch for converting to virStorageSourcePtr in domain_conf.h. I don't mind rebasing on top of yours (in part because the compiler will enforce that my mechanical fallout for a type change catches all uses), but it does mean yours has to go in first :) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/22/14 23:58, Eric Blake wrote:
On 05/22/2014 07:47 AM, Peter Krempa wrote:
Refactor the function to accept a virStorageSourcePtr instead of just the path, add a check to run it only on local storage and fix callers (possibly by using a newly introduced wrapper that wraps a path in the virStorageSource struct for legacy code) ---
Notes: Version 2: - V1 acked by Eric with comment cleanup
ACK still holds.
This is a large series; you should go ahead and push ACK'd patches (especially if it is before the 1.2.5 freeze) rather than waiting for a full-series review, unless stated explicitly in a patch, or until I hit something that requires rework that might affect later patches. [that, and I have patches for live commit that I'm waiting to rebase on top of yours...]
Well in one of my previous large series I've pushed some code that was already ACKed at a start of the series which later ended up unused due to changes later in the series and I was reverting those changes. Thus I usually rather keep them private until I'm sure that it will be used. Said that, I'm going to start pushing this series partially as it's getting tedious for me to spin that amount of patches around. Peter
The gluster volume name was previously stored as part of the source path string. This is unfortunate when we want to do operations on the path as the volume is used separately. Parse and store the volume name separately for gluster storage volumes and use the newly stored variable appropriately. --- Notes: Version 2: - no changes, v1 already ACKed by Eric src/conf/domain_conf.c | 33 ++++++++++++++++++++++++++++++++- src/qemu/qemu_command.c | 19 ++++++++++++++----- src/storage/storage_backend_gluster.c | 27 ++++++++------------------- src/util/virstoragefile.c | 1 + src/util/virstoragefile.h | 1 + 5 files changed, 56 insertions(+), 25 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 055b979..1ddedac 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5002,6 +5002,27 @@ virDomainDiskSourceParse(xmlNodePtr node, goto cleanup; } + /* for historical reasons the volume name for gluster volume is stored + * as a part of the path. This is hard to work with when dealing with + * relative names. Split out the volume into a separate variable */ + if (src->path && 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'; + } + child = node->children; while (child != NULL) { if (child->type == XML_ELEMENT_NODE && @@ -14841,6 +14862,7 @@ virDomainDiskSourceFormat(virBufferPtr buf, unsigned int flags) { size_t n; + char *path = NULL; const char *startupPolicy = NULL; if (policy) @@ -14876,7 +14898,16 @@ virDomainDiskSourceFormat(virBufferPtr buf, case VIR_STORAGE_TYPE_NETWORK: virBufferAsprintf(buf, "<source protocol='%s'", virStorageNetProtocolTypeToString(src->protocol)); - virBufferEscapeString(buf, " name='%s'", src->path); + + + if (src->volume) { + if (virAsprintf(&path, "%s%s", src->volume, src->path) < 0) + return -1; + } + + virBufferEscapeString(buf, " name='%s'", path ? path : src->path); + + VIR_FREE(path); if (src->nhosts == 0) { virBufferAddLit(buf, "/>\n"); diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 193959f..31d0781 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3058,6 +3058,7 @@ qemuNetworkDriveGetPort(int protocol, static char * qemuBuildNetworkDriveURI(int protocol, const char *src, + const char *volume, size_t nhosts, virStorageNetHostDefPtr hosts, const char *username, @@ -3157,11 +3158,18 @@ qemuBuildNetworkDriveURI(int protocol, if ((uri->port = qemuNetworkDriveGetPort(protocol, hosts->port)) < 0) goto cleanup; - if (src && - virAsprintf(&uri->path, "%s%s", - src[0] == '/' ? "" : "/", - src) < 0) - goto cleanup; + if (src) { + if (volume) { + if (virAsprintf(&uri->path, "/%s%s", + volume, src) < 0) + goto cleanup; + } else { + if (virAsprintf(&uri->path, "%s%s", + src[0] == '/' ? "" : "/", + src) < 0) + goto cleanup; + } + } if (hosts->socket && virAsprintf(&uri->query, "socket=%s", hosts->socket) < 0) @@ -3323,6 +3331,7 @@ qemuGetDriveSourceString(virStorageSourcePtr src, case VIR_STORAGE_TYPE_NETWORK: if (!(*source = qemuBuildNetworkDriveURI(src->protocol, src->path, + src->volume, src->nhosts, src->hosts, username, diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 2f0592f..8679d96 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -533,8 +533,6 @@ typedef virStorageFileBackendGlusterPriv *virStorageFileBackendGlusterPrivPtr; struct _virStorageFileBackendGlusterPriv { glfs_t *vol; - char *volname; - char *path; }; @@ -547,7 +545,6 @@ virStorageFileBackendGlusterDeinit(virStorageSourcePtr src) if (priv->vol) glfs_fini(priv->vol); - VIR_FREE(priv->volname); VIR_FREE(priv); src->drv->priv = NULL; @@ -564,21 +561,14 @@ virStorageFileBackendGlusterInit(virStorageSourcePtr src) VIR_DEBUG("initializing gluster storage file %p(%s/%s)", src, hostname, src->path); - if (VIR_ALLOC(priv) < 0) - return -1; - - if (VIR_STRDUP(priv->volname, src->path) < 0) - goto error; - - if (!(priv->path = strchr(priv->volname, '/'))) { + if (!src->volume) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("invalid path of gluster volume: '%s'"), - src->path); - goto error; + _("missing gluster volume name for path '%s'"), src->path); + return -1; } - *priv->path = '\0'; - priv->path++; + if (VIR_ALLOC(priv) < 0) + return -1; if (host->port && virStrToLong_i(host->port, NULL, 10, &port) < 0) { @@ -591,7 +581,7 @@ virStorageFileBackendGlusterInit(virStorageSourcePtr src) if (host->transport == VIR_STORAGE_NET_HOST_TRANS_UNIX) hostname = host->socket; - if (!(priv->vol = glfs_new(priv->volname))) { + if (!(priv->vol = glfs_new(src->volume))) { virReportOOMError(); goto error; } @@ -617,7 +607,6 @@ virStorageFileBackendGlusterInit(virStorageSourcePtr src) return 0; error: - VIR_FREE(priv->volname); if (priv->vol) glfs_fini(priv->vol); VIR_FREE(priv); @@ -632,7 +621,7 @@ virStorageFileBackendGlusterUnlink(virStorageSourcePtr src) virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; int ret; - ret = glfs_unlink(priv->vol, priv->path); + ret = glfs_unlink(priv->vol, src->path); /* preserve errno */ VIR_DEBUG("removing storage file %p(%s/%s): ret=%d, errno=%d", @@ -648,7 +637,7 @@ virStorageFileBackendGlusterStat(virStorageSourcePtr src, virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; int ret; - ret = glfs_stat(priv->vol, priv->path, st); + ret = glfs_stat(priv->vol, src->path, st); /* preserve errno */ VIR_DEBUG("stat of storage file %p(%s/%s): ret=%d, errno=%d", diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 5c1ab62..856b726 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1757,6 +1757,7 @@ virStorageSourceClear(virStorageSourcePtr def) return; VIR_FREE(def->path); + VIR_FREE(def->volume); virStorageSourcePoolDefFree(def->srcpool); VIR_FREE(def->driverName); virBitmapFree(def->features); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index f875b69..d3560a8 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -212,6 +212,7 @@ struct _virStorageSource { int type; /* virStorageType */ char *path; int protocol; /* virStorageNetProtocol */ + char *volume; /* volume name for remote storage */ size_t nhosts; virStorageNetHostDefPtr hosts; virStorageSourcePoolDefPtr srcpool; -- 1.9.3
On 05/22/2014 07:47 AM, Peter Krempa wrote:
The gluster volume name was previously stored as part of the source path string. This is unfortunate when we want to do operations on the path as the volume is used separately.
Parse and store the volume name separately for gluster storage volumes and use the newly stored variable appropriately. ---
Notes: Version 2: - no changes, v1 already ACKed by Eric
ACK still holds. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Print the debug statements of individual file access functions from the main API functions instead of the individual backend functions. Also enhance initialization debug messages on a per-backend basis. --- Notes: Version 2: - no change, v1 already ACKed by Eric src/storage/storage_backend_fs.c | 43 ++++++++++++++++++++++------------- src/storage/storage_backend_gluster.c | 30 +++++++++--------------- src/storage/storage_driver.c | 27 +++++++++++++++++++--- 3 files changed, 62 insertions(+), 38 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index de27890..3d088ba 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1339,18 +1339,31 @@ virStorageBackend virStorageBackendNetFileSystem = { }; +static void +virStorageFileBackendFileDeinit(virStorageSourcePtr src) +{ + VIR_DEBUG("deinitializing FS storage file %p (%s:%s)", src, + virStorageTypeToString(virStorageSourceGetActualType(src)), + src->path); + +} + + static int -virStorageFileBackendFileUnlink(virStorageSourcePtr src) +virStorageFileBackendFileInit(virStorageSourcePtr src) { - int ret; + VIR_DEBUG("initializing FS storage file %p (%s:%s)", src, + virStorageTypeToString(virStorageSourceGetActualType(src)), + src->path); - ret = unlink(src->path); - /* preserve errno */ + return 0; +} - VIR_DEBUG("removing storage file %p(%s): ret=%d, errno=%d", - src, src->path, ret, errno); - return ret; +static int +virStorageFileBackendFileUnlink(virStorageSourcePtr src) +{ + return unlink(src->path); } @@ -1358,21 +1371,16 @@ static int virStorageFileBackendFileStat(virStorageSourcePtr src, struct stat *st) { - int ret; - - ret = stat(src->path, st); - /* preserve errno */ - - VIR_DEBUG("stat of storage file %p(%s): ret=%d, errno=%d", - src, src->path, ret, errno); - - return ret; + return stat(src->path, st); } virStorageFileBackend virStorageFileBackendFile = { .type = VIR_STORAGE_TYPE_FILE, + .backendInit = virStorageFileBackendFileInit, + .backendDeinit = virStorageFileBackendFileDeinit, + .storageFileUnlink = virStorageFileBackendFileUnlink, .storageFileStat = virStorageFileBackendFileStat, }; @@ -1381,6 +1389,9 @@ virStorageFileBackend virStorageFileBackendFile = { virStorageFileBackend virStorageFileBackendBlock = { .type = VIR_STORAGE_TYPE_BLOCK, + .backendInit = virStorageFileBackendFileInit, + .backendDeinit = virStorageFileBackendFileDeinit, + .storageFileStat = virStorageFileBackendFileStat, }; diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 8679d96..e578e73 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -539,10 +539,12 @@ struct _virStorageFileBackendGlusterPriv { static void virStorageFileBackendGlusterDeinit(virStorageSourcePtr src) { - VIR_DEBUG("deinitializing gluster storage file %p(%s/%s)", - src, src->hosts[0].name, src->path); virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; + VIR_DEBUG("deinitializing gluster storage file %p (gluster://%s:%s/%s%s)", + src, src->hosts->name, src->hosts->port ? src->hosts->port : "0", + src->volume, src->path); + if (priv->vol) glfs_fini(priv->vol); @@ -558,12 +560,14 @@ virStorageFileBackendGlusterInit(virStorageSourcePtr src) const char *hostname = host->name; int port = 0; - VIR_DEBUG("initializing gluster storage file %p(%s/%s)", - src, hostname, src->path); + VIR_DEBUG("initializing gluster storage file %p (gluster://%s:%s/%s%s)", + src, hostname, host->port ? host->port : "0", + NULLSTR(src->volume), src->path); if (!src->volume) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("missing gluster volume name for path '%s'"), src->path); + _("missing gluster volume name for path '%s'"), + src->path); return -1; } @@ -619,14 +623,8 @@ static int virStorageFileBackendGlusterUnlink(virStorageSourcePtr src) { virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; - int ret; - - ret = glfs_unlink(priv->vol, src->path); - /* preserve errno */ - VIR_DEBUG("removing storage file %p(%s/%s): ret=%d, errno=%d", - src, src->hosts[0].name, src->path, ret, errno); - return ret; + return glfs_unlink(priv->vol, src->path); } @@ -635,14 +633,8 @@ virStorageFileBackendGlusterStat(virStorageSourcePtr src, struct stat *st) { virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; - int ret; - ret = glfs_stat(priv->vol, src->path, st); - /* preserve errno */ - - VIR_DEBUG("stat of storage file %p(%s/%s): ret=%d, errno=%d", - src, src->hosts[0].name, src->path, ret, errno); - return ret; + return glfs_stat(priv->vol, src->path, st); } diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index f58e0a4..455a2ef 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2835,13 +2835,20 @@ virStorageFileInit(virStorageSourcePtr src) int virStorageFileCreate(virStorageSourcePtr src) { + int ret; + if (!virStorageFileIsInitialized(src) || !src->drv->backend->storageFileCreate) { errno = ENOSYS; return -2; } - return src->drv->backend->storageFileCreate(src); + ret = src->drv->backend->storageFileCreate(src); + + VIR_DEBUG("created storage file %p: ret=%d, errno=%d", + src, ret, errno); + + return ret; } @@ -2858,13 +2865,20 @@ virStorageFileCreate(virStorageSourcePtr src) int virStorageFileUnlink(virStorageSourcePtr src) { + int ret; + if (!virStorageFileIsInitialized(src) || !src->drv->backend->storageFileUnlink) { errno = ENOSYS; return -2; } - return src->drv->backend->storageFileUnlink(src); + ret = src->drv->backend->storageFileUnlink(src); + + VIR_DEBUG("unlinked storage file %p: ret=%d, errno=%d", + src, ret, errno); + + return ret; } @@ -2881,11 +2895,18 @@ int virStorageFileStat(virStorageSourcePtr src, struct stat *st) { + int ret; + if (!virStorageFileIsInitialized(src) || !src->drv->backend->storageFileStat) { errno = ENOSYS; return -2; } - return src->drv->backend->storageFileStat(src, st); + ret = src->drv->backend->storageFileStat(src, st); + + VIR_DEBUG("stat of storage file %p: ret=%d, errno=%d", + src, ret, errno); + + return ret; } -- 1.9.3
Skip networked storage but continue iteration through backing chain to iterate through all the local paths in the backing chain. --- Notes: Version 2: - no change, v1 already ACKed by Eric src/conf/domain_conf.c | 37 ++++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1ddedac..4bc71c8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18686,34 +18686,37 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, int ret = -1; size_t depth = 0; virStorageSourcePtr tmp; - const char *path = virDomainDiskGetSource(disk); - int type = virDomainDiskGetType(disk); + char *brokenRaw = NULL; - if (!path || type == VIR_STORAGE_TYPE_NETWORK || - (type == VIR_STORAGE_TYPE_VOLUME && - disk->src.srcpool && - disk->src.srcpool->mode == VIR_STORAGE_SOURCE_POOL_MODE_DIRECT)) - return 0; - - if (iter(disk, path, 0, opaque) < 0) - goto cleanup; + if (!ignoreOpenFailure) { + if (virStorageFileChainGetBroken(&disk->src, &brokenRaw) < 0) + goto cleanup; - tmp = disk->src.backingStore; - while (tmp && virStorageIsFile(tmp->path)) { - if (!ignoreOpenFailure && tmp->backingStoreRaw && !tmp->backingStore) { + if (brokenRaw) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to visit backing chain file %s"), - tmp->backingStoreRaw); + brokenRaw); goto cleanup; } - if (iter(disk, tmp->path, ++depth, opaque) < 0) - goto cleanup; - tmp = tmp->backingStore; + } + + for (tmp = &disk->src; tmp; tmp = tmp->backingStore) { + int actualType = virStorageSourceGetActualType(tmp); + /* execute the callback only for local storage */ + if (actualType != VIR_STORAGE_TYPE_NETWORK && + actualType != VIR_STORAGE_TYPE_VOLUME && + tmp->path) { + if (iter(disk, tmp->path, depth, opaque) < 0) + goto cleanup; + } + + depth++; } ret = 0; cleanup: + VIR_FREE(brokenRaw); return ret; } -- 1.9.3
Currently the protocol type with index 0 was NBD which made it hard to distinguish whether the protocol type was actually assigned. Add a new protocol type with index 0 to distinguish it explicitly. --- Notes: Version 2: - fixed condition in virDomainDiskSourceParse so that "none" isn't accepted - fixed typo in comment in qemuNetworkDriveGetPort() - fixed commit message src/conf/domain_conf.c | 2 +- src/qemu/qemu_command.c | 5 ++++- src/qemu/qemu_driver.c | 3 +++ src/util/virstoragefile.c | 1 + src/util/virstoragefile.h | 1 + 5 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4bc71c8..4fb2e1d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4989,7 +4989,7 @@ virDomainDiskSourceParse(xmlNodePtr node, goto cleanup; } - if ((src->protocol = virStorageNetProtocolTypeFromString(protocol)) < 0){ + if ((src->protocol = virStorageNetProtocolTypeFromString(protocol)) <= 0){ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown protocol type '%s'"), protocol); goto cleanup; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 31d0781..0a7b2ff 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3046,7 +3046,8 @@ qemuNetworkDriveGetPort(int protocol, case VIR_STORAGE_NET_PROTOCOL_RBD: case VIR_STORAGE_NET_PROTOCOL_LAST: - /* not aplicable */ + case VIR_STORAGE_NET_PROTOCOL_NONE: + /* not applicable */ return -1; } @@ -3262,6 +3263,7 @@ qemuBuildNetworkDriveURI(int protocol, case VIR_STORAGE_NET_PROTOCOL_LAST: + case VIR_STORAGE_NET_PROTOCOL_NONE: goto cleanup; } @@ -11080,6 +11082,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps, case VIR_STORAGE_NET_PROTOCOL_FTPS: case VIR_STORAGE_NET_PROTOCOL_TFTP: case VIR_STORAGE_NET_PROTOCOL_LAST: + case VIR_STORAGE_NET_PROTOCOL_NONE: /* ignored for now */ break; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d3e14ea..2b852eb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12390,6 +12390,7 @@ qemuDomainSnapshotPrepareDiskExternalBackingInactive(virDomainDiskDefPtr disk) case VIR_STORAGE_TYPE_NETWORK: switch ((virStorageNetProtocol) disk->src.protocol) { + case VIR_STORAGE_NET_PROTOCOL_NONE: case VIR_STORAGE_NET_PROTOCOL_NBD: case VIR_STORAGE_NET_PROTOCOL_RBD: case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: @@ -12455,6 +12456,7 @@ qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSnapshotDiskDefPtr d case VIR_STORAGE_NET_PROTOCOL_GLUSTER: return 0; + case VIR_STORAGE_NET_PROTOCOL_NONE: case VIR_STORAGE_NET_PROTOCOL_NBD: case VIR_STORAGE_NET_PROTOCOL_RBD: case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: @@ -12597,6 +12599,7 @@ qemuDomainSnapshotPrepareDiskInternal(virConnectPtr conn, case VIR_STORAGE_TYPE_NETWORK: switch ((virStorageNetProtocol) disk->src.protocol) { + case VIR_STORAGE_NET_PROTOCOL_NONE: case VIR_STORAGE_NET_PROTOCOL_NBD: case VIR_STORAGE_NET_PROTOCOL_RBD: case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 856b726..b90cdc9 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -72,6 +72,7 @@ VIR_ENUM_IMPL(virStorageFileFeature, ) VIR_ENUM_IMPL(virStorageNetProtocol, VIR_STORAGE_NET_PROTOCOL_LAST, + "none", "nbd", "rbd", "sheepdog", diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index d3560a8..082ff5b 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -119,6 +119,7 @@ struct _virStorageTimestamps { /* Information related to network storage */ typedef enum { + VIR_STORAGE_NET_PROTOCOL_NONE, VIR_STORAGE_NET_PROTOCOL_NBD, VIR_STORAGE_NET_PROTOCOL_RBD, VIR_STORAGE_NET_PROTOCOL_SHEEPDOG, -- 1.9.3
On 05/22/2014 07:47 AM, Peter Krempa wrote:
Currently the protocol type with index 0 was NBD which made it hard to distinguish whether the protocol type was actually assigned. Add a new protocol type with index 0 to distinguish it explicitly. ---
Notes: Version 2: - fixed condition in virDomainDiskSourceParse so that "none" isn't accepted - fixed typo in comment in qemuNetworkDriveGetPort() - fixed commit message
src/conf/domain_conf.c | 2 +- src/qemu/qemu_command.c | 5 ++++- src/qemu/qemu_driver.c | 3 +++ src/util/virstoragefile.c | 1 + src/util/virstoragefile.h | 1 + 5 files changed, 10 insertions(+), 2 deletions(-)
ACK with one more nit. Previous ack on 4 and 5 still stands, too.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4bc71c8..4fb2e1d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4989,7 +4989,7 @@ virDomainDiskSourceParse(xmlNodePtr node, goto cleanup; }
- if ((src->protocol = virStorageNetProtocolTypeFromString(protocol)) < 0){ + if ((src->protocol = virStorageNetProtocolTypeFromString(protocol)) <= 0){
Worth adding the space before { as long as you are touching this line. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
To allow using the storage driver APIs to access files on various storage sources in an universal fashion possibly on storage such as nfs with root squash we'll need to store the desired uid/gid in the metadata. Add new initialisation API that will store the desired uid/gid and a wrapper for the current use. Additionally add docs for the two APIs. --- src/storage/storage_backend.h | 3 +++ src/storage/storage_driver.c | 39 ++++++++++++++++++++++++++++++++++++++- src/storage/storage_driver.h | 5 +++-- 3 files changed, 44 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 456b9d7..fcbb6da 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -169,6 +169,9 @@ typedef virStorageFileBackend *virStorageFileBackendPtr; struct _virStorageDriverData { virStorageFileBackendPtr backend; void *priv; + + uid_t uid; + gid_t gid; }; typedef int diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 455a2ef..5e740f9 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2801,13 +2801,37 @@ virStorageFileDeinit(virStorageSourcePtr src) } +/** + * virStorageFileInitAs: + * + * @src: storage source definition + * @uid: uid to access the file as, -1 for current uid + * @gid: gid to access the file as, -1 for current gid + * + * Initialize a storage source to be used with storage driver. Use the provided + * uid and gid if possible for the operations. + * + * Returns 0 if the storage file was successfully initialized, -1 if the + * initialization failed. Libvirt error is reported. + */ int -virStorageFileInit(virStorageSourcePtr src) +virStorageFileInitAs(virStorageSourcePtr src, + uid_t uid, gid_t gid) { int actualType = virStorageSourceGetActualType(src); if (VIR_ALLOC(src->drv) < 0) return -1; + if (uid == (uid_t) -1) + src->drv->uid = geteuid(); + else + src->drv->uid = uid; + + if (gid == (gid_t) -1) + src->drv->gid = getegid(); + else + src->drv->gid = gid; + if (!(src->drv->backend = virStorageFileBackendForType(actualType, src->protocol))) goto error; @@ -2825,6 +2849,19 @@ virStorageFileInit(virStorageSourcePtr src) /** + * virStorageFileInit: + * + * See virStorageFileInitAs. The file is initialized to be accessed by the + * current user. + */ +int +virStorageFileInit(virStorageSourcePtr src) +{ + return virStorageFileInitAs(src, (uid_t) -1, (gid_t) -1); +} + + +/** * virStorageFileCreate: Creates an empty storage file via storage driver * * @src: file structure pointing to the file diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h index fb03870..49be999 100644 --- a/src/storage/storage_driver.h +++ b/src/storage/storage_driver.h @@ -29,8 +29,9 @@ # include "storage_conf.h" # include "virstoragefile.h" -int -virStorageFileInit(virStorageSourcePtr src); +int virStorageFileInit(virStorageSourcePtr src); +int virStorageFileInitAs(virStorageSourcePtr src, + uid_t uid, gid_t gid); void virStorageFileDeinit(virStorageSourcePtr src); int virStorageFileCreate(virStorageSourcePtr src); -- 1.9.3
On 05/22/2014 07:47 AM, Peter Krempa wrote:
To allow using the storage driver APIs to access files on various storage sources in an universal fashion possibly on storage such as nfs
s/an universal/a universal/
with root squash we'll need to store the desired uid/gid in the metadata.
Add new initialisation API that will store the desired uid/gid and a wrapper for the current use. Additionally add docs for the two APIs. --- src/storage/storage_backend.h | 3 +++ src/storage/storage_driver.c | 39 ++++++++++++++++++++++++++++++++++++++- src/storage/storage_driver.h | 5 +++-- 3 files changed, 44 insertions(+), 3 deletions(-)
+/** + * virStorageFileInitAs: + * + * @src: storage source definition + * @uid: uid to access the file as, -1 for current uid + * @gid: gid to access the file as, -1 for current gid
Correct grammar as written, but didn't flow well and took me two reads to avoid confusion. Would be easier with the addition of "or", as in: @xid: xid to access the file as, or -1 for current xid or even: @xid: xid used to access the file, or -1 for current xid
+ if (uid == (uid_t) -1) + src->drv->uid = geteuid(); + else + src->drv->uid = uid;
Do we need to do the conversion here, or can we store -1 and let other routines later do the conversion? I'm not sure if it matters either way, so I'm fine leaving it this way.
+int +virStorageFileInit(virStorageSourcePtr src) +{ + return virStorageFileInitAs(src, (uid_t) -1, (gid_t) -1);
Casts aren't strictly necessary on this line (the C compiler does the correct conversion from int to uid_t thanks to the function prototype). ACK with the comment and cast cleanup. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/23/14 00:59, Eric Blake wrote:
On 05/22/2014 07:47 AM, Peter Krempa wrote:
To allow using the storage driver APIs to access files on various storage sources in an universal fashion possibly on storage such as nfs
s/an universal/a universal/
with root squash we'll need to store the desired uid/gid in the metadata.
Add new initialisation API that will store the desired uid/gid and a wrapper for the current use. Additionally add docs for the two APIs. --- src/storage/storage_backend.h | 3 +++ src/storage/storage_driver.c | 39 ++++++++++++++++++++++++++++++++++++++- src/storage/storage_driver.h | 5 +++-- 3 files changed, 44 insertions(+), 3 deletions(-)
+/** + * virStorageFileInitAs: + * + * @src: storage source definition + * @uid: uid to access the file as, -1 for current uid + * @gid: gid to access the file as, -1 for current gid
Correct grammar as written, but didn't flow well and took me two reads to avoid confusion. Would be easier with the addition of "or", as in:
@xid: xid to access the file as, or -1 for current xid
or even:
@xid: xid used to access the file, or -1 for current xid
+ if (uid == (uid_t) -1) + src->drv->uid = geteuid(); + else + src->drv->uid = uid;
Do we need to do the conversion here, or can we store -1 and let other routines later do the conversion? I'm not sure if it matters either way, so I'm fine leaving it this way.
We've got a syntax check for that :) src/storage/storage_driver.c:2825: if (uid == -1) maint.mk: cast -1 to ([ug]id_t) before comparing against id make: *** [sc_prohibit_risky_id_promotion] Error 1
+int +virStorageFileInit(virStorageSourcePtr src) +{ + return virStorageFileInitAs(src, (uid_t) -1, (gid_t) -1);
Casts aren't strictly necessary on this line (the C compiler does the correct conversion from int to uid_t thanks to the function prototype).
The compiler and syntax check are quiet here, so I've removed them.
ACK with the comment and cast cleanup.
Peter
On 05/23/2014 02:49 AM, Peter Krempa wrote:
+ if (uid == (uid_t) -1) + src->drv->uid = geteuid(); + else + src->drv->uid = uid;
Do we need to do the conversion here, or can we store -1 and let other routines later do the conversion? I'm not sure if it matters either way, so I'm fine leaving it this way.
We've got a syntax check for that :)
src/storage/storage_driver.c:2825: if (uid == -1) maint.mk: cast -1 to ([ug]id_t) before comparing against id make: *** [sc_prohibit_risky_id_promotion] Error 1
The comparison to casted -1 is required (and the syntax check is correct on that front); what I was asking was whether we need the entire if statement to convert -1 into a uid up front, or whether we can just store -1 here and let later clients convert to uid at the time it is used. In other words, I was asking if you could collapse those four lines into: src->drv->uid = uid; and whether all subsequent users would handle the -1 just fine. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Add storage driver based functions to access headers of storage files for metadata extraction. Along with this patch a local filesystem and gluster via libgfapi implementation is provided. The gluster implementation is based on code of the saferead_lim function. --- src/storage/storage_backend.h | 7 ++++ src/storage/storage_backend_fs.c | 30 ++++++++++++++++ src/storage/storage_backend_gluster.c | 66 +++++++++++++++++++++++++++++++++++ src/storage/storage_driver.c | 40 +++++++++++++++++++++ src/storage/storage_driver.h | 3 ++ 5 files changed, 146 insertions(+) diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index fcbb6da..6be5ca7 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -190,6 +190,12 @@ typedef int (*virStorageFileBackendStat)(virStorageSourcePtr src, struct stat *st); +typedef ssize_t +(*virStorageFileBackendReadHeader)(virStorageSourcePtr src, + ssize_t max_len, + char **buf); + + virStorageFileBackendPtr virStorageFileBackendForType(int type, int protocol); @@ -204,6 +210,7 @@ struct _virStorageFileBackend { * error on failure. */ virStorageFileBackendInit backendInit; virStorageFileBackendDeinit backendDeinit; + virStorageFileBackendReadHeader storageFileReadHeader; /* 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 3d088ba..33551e7 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1375,6 +1375,34 @@ virStorageFileBackendFileStat(virStorageSourcePtr src, } +static ssize_t +virStorageFileBackendFileReadHeader(virStorageSourcePtr src, + ssize_t max_len, + char **buf) +{ + int fd = -1; + ssize_t ret = -1; + + if ((fd = virFileOpenAs(src->path, O_RDONLY, 0, + src->drv->uid, src->drv->gid, 0)) < 0) { + virReportSystemError(-fd, _("Failed to open file '%s'"), + src->path); + return -1; + } + + if ((ret = virFileReadHeaderFD(fd, max_len, buf)) < 0) { + virReportSystemError(errno, + _("cannot read header '%s'"), src->path); + goto cleanup; + } + + cleanup: + VIR_FORCE_CLOSE(fd); + + return ret; +} + + virStorageFileBackend virStorageFileBackendFile = { .type = VIR_STORAGE_TYPE_FILE, @@ -1383,6 +1411,7 @@ virStorageFileBackend virStorageFileBackendFile = { .storageFileUnlink = virStorageFileBackendFileUnlink, .storageFileStat = virStorageFileBackendFileStat, + .storageFileReadHeader = virStorageFileBackendFileReadHeader, }; @@ -1393,6 +1422,7 @@ virStorageFileBackend virStorageFileBackendBlock = { .backendDeinit = virStorageFileBackendFileDeinit, .storageFileStat = virStorageFileBackendFileStat, + .storageFileReadHeader = virStorageFileBackendFileReadHeader, }; diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index e578e73..badffae 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -638,6 +638,71 @@ virStorageFileBackendGlusterStat(virStorageSourcePtr src, } +static ssize_t +virStorageFileBackendGlusterReadHeader(virStorageSourcePtr src, + ssize_t max_len, + char **buf) +{ + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; + glfs_fd_t *fd = NULL; + size_t alloc = 0; + size_t size = 0; + int save_errno; + ssize_t ret = -1; + + *buf = NULL; + + if (!(fd = glfs_open(priv->vol, src->path, O_RDONLY))) { + virReportSystemError(errno, _("Failed to open file '%s'"), + src->path); + goto cleanup; + } + + /* code below is shamelesly stolen from saferead_lim */ + for (;;) { + int count; + int requested; + + if (size + BUFSIZ + 1 > alloc) { + alloc += alloc / 2; + if (alloc < size + BUFSIZ + 1) + alloc = size + BUFSIZ + 1; + + if (VIR_REALLOC_N(*buf, alloc) < 0) { + save_errno = errno; + break; + } + } + + /* Ensure that (size + requested <= max_len); */ + requested = MIN(size < max_len ? max_len - size : 0, + alloc - size - 1); + count = glfs_read(fd, *buf + size, requested, 0); + size += count; + + if (count != requested || requested == 0) { + save_errno = errno; + if (count < 0) { + virReportSystemError(errno, + _("cannot read header '%s'"), src->path); + break; + } + ret = size; + goto cleanup; + } + } + + VIR_FREE(*buf); + errno = save_errno; + + cleanup: + if (fd) + glfs_close(fd); + + return ret; +} + + virStorageFileBackend virStorageFileBackendGluster = { .type = VIR_STORAGE_TYPE_NETWORK, .protocol = VIR_STORAGE_NET_PROTOCOL_GLUSTER, @@ -647,4 +712,5 @@ virStorageFileBackend virStorageFileBackendGluster = { .storageFileUnlink = virStorageFileBackendGlusterUnlink, .storageFileStat = virStorageFileBackendGlusterStat, + .storageFileReadHeader = virStorageFileBackendGlusterReadHeader, }; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 5e740f9..3019531 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2947,3 +2947,43 @@ virStorageFileStat(virStorageSourcePtr src, return ret; } + + +/** + * virStorageFileReadHeader: read the beginning bytes of a file into a buffer + * + * @src: file structure pointing to the file + * @max_len: maximum number of bytes read from the storage file + * @buf: buffer to read the data into. buffer shall be freed by caller) + * + * Returns the count of bytes read on success and -1 on failure, -2 if the + * function isn't supported by the backend. Libvirt error is reported on failure. + */ +ssize_t +virStorageFileReadHeader(virStorageSourcePtr src, + ssize_t max_len, + char **buf) +{ + ssize_t ret; + + if (!virStorageFileIsInitialized(src)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("storage file backend not initialized")); + return -1; + } + + if (!src->drv->backend->storageFileReadHeader) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("storage file header reading is not supported for " + "storage type %s (protocol: %s)'"), + virStorageTypeToString(src->type), + virStorageNetProtocolTypeToString(src->protocol)); + return -1; + } + + ret = src->drv->backend->storageFileReadHeader(src, max_len, buf); + + VIR_DEBUG("read of storage header %p: ret=%zd", src, ret); + + return ret; +} diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h index 49be999..d67d74b 100644 --- a/src/storage/storage_driver.h +++ b/src/storage/storage_driver.h @@ -38,6 +38,9 @@ int virStorageFileCreate(virStorageSourcePtr src); int virStorageFileUnlink(virStorageSourcePtr src); int virStorageFileStat(virStorageSourcePtr src, struct stat *stat); +ssize_t virStorageFileReadHeader(virStorageSourcePtr src, + ssize_t max_len, + char **buf); int storageRegister(void); -- 1.9.3
On 05/22/2014 07:47 AM, Peter Krempa wrote:
Add storage driver based functions to access headers of storage files for metadata extraction. Along with this patch a local filesystem and gluster via libgfapi implementation is provided. The gluster implementation is based on code of the saferead_lim function. --- src/storage/storage_backend.h | 7 ++++ src/storage/storage_backend_fs.c | 30 ++++++++++++++++ src/storage/storage_backend_gluster.c | 66 +++++++++++++++++++++++++++++++++++ src/storage/storage_driver.c | 40 +++++++++++++++++++++ src/storage/storage_driver.h | 3 ++ 5 files changed, 146 insertions(+)
+++ b/src/storage/storage_backend_gluster.c @@ -638,6 +638,71 @@ virStorageFileBackendGlusterStat(virStorageSourcePtr src, }
+static ssize_t +virStorageFileBackendGlusterReadHeader(virStorageSourcePtr src, + ssize_t max_len, + char **buf) +{ + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; + glfs_fd_t *fd = NULL; + size_t alloc = 0; + size_t size = 0; + int save_errno; + ssize_t ret = -1; + + *buf = NULL; + + if (!(fd = glfs_open(priv->vol, src->path, O_RDONLY))) { + virReportSystemError(errno, _("Failed to open file '%s'"), + src->path); + goto cleanup; + } + + /* code below is shamelesly stolen from saferead_lim */
s/shamelesly/shamelessly/
+ if (!src->drv->backend->storageFileReadHeader) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("storage file header reading is not supported for " + "storage type %s (protocol: %s)'"), + virStorageTypeToString(src->type), + virStorageNetProtocolTypeToString(src->protocol));
Oh slick - the earlier patch that adds protocol==0 as "none" rather than "nbd" makes this message tolerable even for non-network storage types. Less special-casing is always good :)
+ return -1;
Isn't this supposed to be 'return -2' according to the function documentation? ACK if you agree about the return value fix, and if it is the only change you make besides the typo fix. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/23/14 01:13, Eric Blake wrote:
On 05/22/2014 07:47 AM, Peter Krempa wrote:
Add storage driver based functions to access headers of storage files for metadata extraction. Along with this patch a local filesystem and gluster via libgfapi implementation is provided. The gluster implementation is based on code of the saferead_lim function. --- src/storage/storage_backend.h | 7 ++++ src/storage/storage_backend_fs.c | 30 ++++++++++++++++ src/storage/storage_backend_gluster.c | 66 +++++++++++++++++++++++++++++++++++ src/storage/storage_driver.c | 40 +++++++++++++++++++++ src/storage/storage_driver.h | 3 ++ 5 files changed, 146 insertions(+)
ACK if you agree about the return value fix, and if it is the only change you make besides the typo fix.
Patches 1-8 are now pushed with the suggested changes. Thanks. Peter
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. --- 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 33551e7..1f436fa 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1339,6 +1339,14 @@ virStorageBackend virStorageBackendNetFileSystem = { }; +typedef struct _virStorageFileBackendFsPriv virStorageFileBackendFsPriv; +typedef virStorageFileBackendFsPriv *virStorageFileBackendFsPrivPtr; + +struct _virStorageFileBackendFsPriv { + char *uid; /* unique file identifier (canonical path) */ +}; + + static void virStorageFileBackendFileDeinit(virStorageSourcePtr src) { @@ -1346,16 +1354,27 @@ virStorageFileBackendFileDeinit(virStorageSourcePtr src) virStorageTypeToString(virStorageSourceGetActualType(src)), src->path); + virStorageFileBackendFsPrivPtr priv = src->drv->priv; + + VIR_FREE(priv->uid); + 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; } @@ -1403,6 +1422,23 @@ virStorageFileBackendFileReadHeader(virStorageSourcePtr src, } +static const char * +virStorageFileBackendFileGetUniqueIdentifier(virStorageSourcePtr src) +{ + virStorageFileBackendFsPrivPtr priv = src->drv->priv; + + if (!priv->uid) { + if (!(priv->uid = canonicalize_file_name(src->path))) { + virReportSystemError(errno, _("can't canonicalize path '%s'"), + src->path); + return NULL; + } + } + + return priv->uid; +} + + virStorageFileBackend virStorageFileBackendFile = { .type = VIR_STORAGE_TYPE_FILE, @@ -1412,6 +1448,8 @@ virStorageFileBackend virStorageFileBackendFile = { .storageFileUnlink = virStorageFileBackendFileUnlink, .storageFileStat = virStorageFileBackendFileStat, .storageFileReadHeader = virStorageFileBackendFileReadHeader, + + .storageFileGetUniqueIdentifier = virStorageFileBackendFileGetUniqueIdentifier, }; @@ -1423,7 +1461,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 3019531..2036bfd 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. 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/22/2014 07:47 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. --- 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(+)
+typedef struct _virStorageFileBackendFsPriv virStorageFileBackendFsPriv; +typedef virStorageFileBackendFsPriv *virStorageFileBackendFsPrivPtr; + +struct _virStorageFileBackendFsPriv { + char *uid; /* unique file identifier (canonical path) */
The name 'uid' is misleading (too commonly used to mean user-id, which it is not). Can you change it to something like 'fid' (short for file-id) or 'canon'?
+}; + + static void virStorageFileBackendFileDeinit(virStorageSourcePtr src) { @@ -1346,16 +1354,27 @@ virStorageFileBackendFileDeinit(virStorageSourcePtr src) virStorageTypeToString(virStorageSourceGetActualType(src)), src->path);
+ virStorageFileBackendFsPrivPtr priv = src->drv->priv; + + VIR_FREE(priv->uid);
As evidence, if I see this line in isolation, I think "why are you freeing an integer" - I had to look at the header to learn "oh, it's not a usual uid, but a string".
+/* + * 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. Returns NULL on error and sets a libvirt + * error code */
Is it also worth documenting that the string is invalidated when the input parameter 'src' is freed? The idea makes sense, but I'd like to see a v3. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Add a storage driver API equivalent ot the access() function. Implementations for the filesystem and gluster backends are provided. --- 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 1f436fa..85264a7 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1439,6 +1439,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, @@ -1448,6 +1457,7 @@ virStorageFileBackend virStorageFileBackendFile = { .storageFileUnlink = virStorageFileBackendFileUnlink, .storageFileStat = virStorageFileBackendFileStat, .storageFileReadHeader = virStorageFileBackendFileReadHeader, + .storageFileAccess = virStorageFileBackendFileAccess, .storageFileGetUniqueIdentifier = virStorageFileBackendFileGetUniqueIdentifier, }; @@ -1461,6 +1471,7 @@ virStorageFileBackend virStorageFileBackendBlock = { .storageFileStat = virStorageFileBackendFileStat, .storageFileReadHeader = virStorageFileBackendFileReadHeader, + .storageFileAccess = virStorageFileBackendFileAccess, .storageFileGetUniqueIdentifier = virStorageFileBackendFileGetUniqueIdentifier, }; @@ -1472,6 +1483,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 badffae..1a844c9 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 2036bfd..cb660ea 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 accessability 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
On 05/22/2014 07:47 AM, Peter Krempa wrote:
Add a storage driver API equivalent ot the access() function.
s/ot/of/
Implementations for the filesystem and gluster backends are provided. --- 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
+ +/** + * virStorageFileAccess: Check accessability of a storage file
s/accessability/accessibility/ ACK with fixes. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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. --- 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 | 7 +- tests/virstoragetest.c | 2 + 11 files changed, 258 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 c3332c9..25dab2d 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 cb660ea..c90107b 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 b90cdc9..06bb68c 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, @@ -1019,7 +962,7 @@ virStorageFileGetMetadataFromBuf(const char *path, /* Internal version that also supports a containing directory name. */ -static int +int virStorageFileGetMetadataFromFDInternal(virStorageSourcePtr meta, int fd, int *backingFormat) @@ -1108,180 +1051,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 082ff5b..c1babdc 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..4d24822 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -869,7 +869,12 @@ 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 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
On 05/22/2014 07:47 AM, Peter Krempa wrote:
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. --- 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 | 7 +- tests/virstoragetest.c | 2 + 11 files changed, 258 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)";; \
Fair enough - this lets security/ use storage/, but as they are on the same level, it is not a layering violation.
/* Internal version that also supports a containing directory name. */ -static int +int virStorageFileGetMetadataFromFDInternal(virStorageSourcePtr meta, int fd, int *backingFormat)
It's a bit confusing that we now have virStorageFile* functions spread across two different files; maybe a later patch should rename the storage_driver.h functions to have a different prefix?
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
That's a lot of whitespace (5 tab + 3 space); 1 or 2 tabs would have been sufficient. We aren't very consistent on whether to end a list with $(NULL), so that you can insert a new line anywhere later (even last) without having to touch multiple lines. But overall, this looks like straightforward code motion. ACK with whitespace nit fixed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/23/14 18:38, Eric Blake wrote:
On 05/22/2014 07:47 AM, Peter Krempa wrote:
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. --- 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 | 7 +- tests/virstoragetest.c | 2 + 11 files changed, 258 insertions(+), 239 deletions(-)
/* Internal version that also supports a containing directory name. */ -static int +int virStorageFileGetMetadataFromFDInternal(virStorageSourcePtr meta, int fd, int *backingFormat)
It's a bit confusing that we now have virStorageFile* functions spread across two different files; maybe a later patch should rename the storage_driver.h functions to have a different prefix?
We definitely can address this later. Any suggestions on the name of the moved code? Peter
On 05/26/2014 03:27 AM, Peter Krempa wrote:
On 05/23/14 18:38, Eric Blake wrote:
On 05/22/2014 07:47 AM, Peter Krempa wrote:
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.
src/storage/storage_driver.c | 233 ++++++++++++++++++++++++++++++++++++++++++ src/storage/storage_driver.h | 5 + src/util/virstoragefile.c | 233 +-----------------------------------------
virStorageFileGetMetadataFromFDInternal(virStorageSourcePtr meta, int fd, int *backingFormat)
It's a bit confusing that we now have virStorageFile* functions spread across two different files; maybe a later patch should rename the storage_driver.h functions to have a different prefix?
We definitely can address this later. Any suggestions on the name of the moved code?
virStorageDriverFoo? It's slightly longer, so might cause long lines, but seems reasonable for the file name. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/28/14 03:53, Eric Blake wrote:
On 05/26/2014 03:27 AM, Peter Krempa wrote:
On 05/23/14 18:38, Eric Blake wrote:
On 05/22/2014 07:47 AM, Peter Krempa wrote:
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.
src/storage/storage_driver.c | 233 ++++++++++++++++++++++++++++++++++++++++++ src/storage/storage_driver.h | 5 + src/util/virstoragefile.c | 233 +-----------------------------------------
virStorageFileGetMetadataFromFDInternal(virStorageSourcePtr meta, int fd, int *backingFormat)
It's a bit confusing that we now have virStorageFile* functions spread across two different files; maybe a later patch should rename the storage_driver.h functions to have a different prefix?
We definitely can address this later. Any suggestions on the name of the moved code?
virStorageDriverFoo? It's slightly longer, so might cause long lines, but seems reasonable for the file name.
And what about the functions to access storage files via the storage driver? they are called virStorageFileFoo but reside in the storage driver too. Should I change those too? Peter src/storage/storage_driver.h: int virStorageFileInit(virStorageSourcePtr src); int virStorageFileInitAs(virStorageSourcePtr src, uid_t uid, gid_t gid); void virStorageFileDeinit(virStorageSourcePtr src); int virStorageFileCreate(virStorageSourcePtr src); int virStorageFileUnlink(virStorageSourcePtr src); int virStorageFileStat(virStorageSourcePtr src, struct stat *stat); ssize_t virStorageFileReadHeader(virStorageSourcePtr src, ssize_t max_len, char **buf); 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);
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. --- 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 c90107b..cd4c6b5 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
On 05/22/2014 07:47 AM, Peter Krempa wrote:
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. --- src/storage/storage_driver.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
@@ -3173,6 +3174,16 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, ret = 0; goto cleanup; } + + /* update the type for local storage */ + if (stat(backingStore->path, &st) == 0) {
Does this add additional stat() calls to the loop, which could be optimized by including a struct stat in virStorageSource and reusing that data when already obtained earlier? At any rate, cleaning that up to minimize stat calls should be a separate patch, so this one is fine. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/23/14 18:57, Eric Blake wrote:
On 05/22/2014 07:47 AM, Peter Krempa wrote:
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. --- src/storage/storage_driver.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
@@ -3173,6 +3174,16 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, ret = 0; goto cleanup; } + + /* update the type for local storage */ + if (stat(backingStore->path, &st) == 0) {
Does this add additional stat() calls to the loop, which could be optimized by including a struct stat in virStorageSource and reusing that data when already obtained earlier? At any rate, cleaning that up to minimize stat calls should be a separate patch, so this one is fine.
This indeed adds a second call to stat here, but the original call is later removed. This call is added to satisfy the test and allow changing some of the control flow.
ACK.
Peter
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. --- 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
On 05/22/2014 07:47 AM, Peter Krempa wrote:
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. --- 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) {
Same question as in 12/33 on whether we should cache a struct stat to minimize the calls to stat() (or at most have one stat() prior to open, and one fstat() after open or after any resize operation). Another good reason to minimize stats, as well as to trust open/fstat more than stat/open, is to minimize TOCTTOU races. But this patch makes sense whether or not we do that as a followup. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/23/14 19:29, Eric Blake wrote:
On 05/22/2014 07:47 AM, Peter Krempa wrote:
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. --- 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) {
Same question as in 12/33 on whether we should cache a struct stat to
Not in this case. This code here is used to set the correct type of the file prior to the first iteration of the loop in the tests. The test code doesn't initialize the type of the volume to the actual type (for directories) so we need to fix it up before iterating. Otherwise the test would fail in the later patches as it would try to access a directory as a file.
minimize the calls to stat() (or at most have one stat() prior to open, and one fstat() after open or after any resize operation). Another good reason to minimize stats, as well as to trust open/fstat more than stat/open, is to minimize TOCTTOU races. But this patch makes sense
This argument would be relevant in the previous patch. Here in the test code there is no possible race.
whether or not we do that as a followup.
ACK.
Peter
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. --- 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
On 05/22/2014 07:47 AM, Peter Krempa wrote:
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. --- src/storage/storage_backend.c | 16 ++++++++++++++-- src/storage/storage_backend.h | 4 +++- 2 files changed, 17 insertions(+), 3 deletions(-)
ACK. Now that we're in freeze, it's probably better to wait for the rest of this series until after the release (this particular patch doesn't fix a bug, and later patches move into feature addition) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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 wether we support a given image is introduced to avoid errors for unimplemented backends. --- 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 cd4c6b5..eebb7ee 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
On 05/22/2014 07:47 AM, 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 wether we support a given image is introduced
s/wether/whether/
to avoid errors for unimplemented backends. --- src/storage/storage_driver.c | 77 +++++++++++++++++++++++++++++++------------- 1 file changed, 54 insertions(+), 23 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Use virStorageFileReadHeader() to read headers of storage files possibly on remote storate 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. --- 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 25dab2d..19c89bc 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 eebb7ee..67ab3af 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 06bb68c..9f1b4f4 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, @@ -962,7 +961,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 c1babdc..7dd0187 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
On 05/22/2014 07:47 AM, Peter Krempa wrote:
Use virStorageFileReadHeader() to read headers of storage files possibly on remote storate to retrieve the image metadata.
s/storate/storage/
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. --- 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(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Use virStorageFileAccess() to to check wether the file is accessible in the main part of the metadata crawler. --- 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 67ab3af..67882ce 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
On 05/22/2014 07:47 AM, Peter Krempa wrote:
Use virStorageFileAccess() to to check wether the file is accessible in
s/wether/whether/
the main part of the metadata crawler. --- src/storage/storage_driver.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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 19c89bc..2b2bc10 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 9f1b4f4..db71cf3 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, @@ -1560,3 +1575,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 7dd0187..0d0f958 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; }; @@ -320,5 +322,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
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 67882ce..a4518cb 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
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 a4518cb..9761dd0 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
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. --- src/libvirt_private.syms | 1 + src/util/virstring.c | 24 ++++++++++++++++++++---- src/util/virstring.h | 6 ++++++ 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2b2bc10..ee745ea 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) -- 1.9.3
On 05/22/2014 07:48 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. --- src/libvirt_private.syms | 1 + src/util/virstring.c | 24 ++++++++++++++++++++---- src/util/virstring.h | 6 ++++++ 3 files changed, 27 insertions(+), 4 deletions(-)
Makes sense, but you should also enhance tests/virstringtest.c to cover the new function. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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 ee745ea..cb1fd72 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
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 db71cf3..21d71c4 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 @@ -1924,3 +1925,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 0d0f958..3708c5e 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -324,5 +324,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
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 cb1fd72..6787b51 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 21d71c4..a390b1c 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2019,3 +2019,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 3708c5e..8b23f95 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -327,4 +327,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
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 6787b51..02d86a8 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 a390b1c..330210d 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1959,13 +1959,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; @@ -2009,6 +2043,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++; } @@ -2016,11 +2084,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 8b23f95..a8412a5 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -324,6 +324,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
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 1a844c9..25d2aed 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
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 f8ab975..04249c5 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); @@ -2193,19 +2247,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; @@ -2771,8 +2820,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) @@ -2909,8 +2957,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; @@ -3304,8 +3352,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; @@ -3346,9 +3393,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; @@ -3547,7 +3593,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; @@ -3726,31 +3772,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
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 2b852eb..5a4d3b4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15509,6 +15509,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 04249c5..5b0766a 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3441,6 +3441,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; @@ -3452,6 +3453,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
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 5a4d3b4..e2eccee 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14823,7 +14823,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) @@ -15040,7 +15040,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; @@ -15080,8 +15080,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 d6271fb..fc166a1 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 5b0766a..1e7880b 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3749,6 +3749,7 @@ int qemuMonitorJSONBlockJob(qemuMonitorPtr mon, const char *device, const char *base, + const char *backingName, unsigned long long speed, virDomainBlockJobInfoPtr info, qemuMonitorBlockJobCmd mode, @@ -3764,6 +3765,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"), @@ -3798,6 +3812,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
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 +++++++ 2 files changed, 11 insertions(+) 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} }; -- 1.9.3
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 +++++++++++++++++++--- 2 files changed, 21 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; -- 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_capabilities.c | 5 +++++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_driver.c | 33 +++++++++++++++++++++++++++++---- 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 03d8842..ccd9a93 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", + "block-commit-backing-name", ); @@ -3100,6 +3101,10 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, if (virQEMUCapsProbeQMPCommandLine(qemuCaps, mon) < 0) goto cleanup; + /* TODO, WIP: We shall test this */ + if (qemuCaps->version >= 2000000) + virQEMUCapsSet(qemuCaps, QEMU_CAPS_BLOCKPULL_BACKING_NAME); + ret = 0; cleanup: VIR_FREE(package); diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index d755caa..773298d 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_BLOCKPULL_BACKING_NAME = 168, /* support for backing name tweaking */ QEMU_CAPS_LAST, /* this must always be the last item */ }; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e2eccee..1e7ad1a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15414,8 +15414,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; @@ -15500,6 +15504,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_BLOCKPULL_BACKING_NAME)) { + 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 @@ -15507,9 +15531,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); @@ -15527,6 +15549,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_capabilities.c | 5 ++++- src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_driver.c | 44 ++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 45 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index ccd9a93..7c5c989 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -257,6 +257,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "host-pci-multidomain", "msg-timestamp", "block-commit-backing-name", + "block-stream-backing-name", ); @@ -3102,8 +3103,10 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, goto cleanup; /* TODO, WIP: We shall test this */ - if (qemuCaps->version >= 2000000) + if (qemuCaps->version >= 2000000) { virQEMUCapsSet(qemuCaps, QEMU_CAPS_BLOCKPULL_BACKING_NAME); + virQEMUCapsSet(qemuCaps, QEMU_CAPS_BLOCKSTREAM_BACKING_NAME); + } ret = 0; cleanup: diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 773298d..3938f42 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -207,6 +207,7 @@ enum virQEMUCapsFlags { QEMU_CAPS_HOST_PCI_MULTIDOMAIN = 166, /* support domain > 0 in host pci address */ QEMU_CAPS_MSG_TIMESTAMP = 167, /* -msg timestamp */ QEMU_CAPS_BLOCKPULL_BACKING_NAME = 168, /* support for backing name tweaking */ + QEMU_CAPS_BLOCKSTREAM_BACKING_NAME = 169, /* support for backing name tweaking */ QEMU_CAPS_LAST, /* this must always be the last item */ }; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1e7ad1a..19a6763 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14969,6 +14969,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", @@ -14976,6 +14978,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; @@ -15037,10 +15046,34 @@ 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_BLOCKSTREAM_BACKING_NAME)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this QEMU binary doesn't support relative " + "block pull/rebase")); + goto endjob; + } + + if (disk->src.backingStore == baseSource) { + if (baseSource->backingRelative && + 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; @@ -15110,6 +15143,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, } cleanup: + VIR_FREE(basePath); + VIR_FREE(backingPath); VIR_FREE(device); if (vm) virObjectUnlock(vm); @@ -15350,7 +15385,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 (2)
-
Eric Blake -
Peter Krempa