If a user specifies the pool explicitly, we should make sure to point out that it's inactive instead of falling back to lookup by key/path and failing at the end. Also if the pool isn't found there's no use in continuing the lookup. This changes the error in case the user-selected pool is inactive from: $ virsh vol-upload --pool inactivepool --vol somevolname volcontents error: failed to get vol 'somevolname' error: Storage volume not found: no storage vol with matching path somevolname To a more descriptive: $ virsh vol-upload --pool inactivepool --vol somevolname volcontents error: pool 'inactivepool' is not active And in case a user specifies an invalid pool from: $ virsh vol-upload --pool invalidpool --vol somevolname volcontents error: failed to get pool 'invalidpool' error: failed to get vol 'somevolname', specifying --pool might help error: Storage volume not found: no storage vol with matching path somevolname To something less confusing: $ virsh vol-upload --pool invalidpool --vol somevolname volcontents error: failed to get pool 'invalidpool' error: Storage pool not found: no storage pool with matching name 'invalidpool' --- tools/virsh-volume.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 898b34b..d7c4823 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -60,8 +60,16 @@ vshCommandOptVolBy(vshControl *ctl, const vshCmd *cmd, vshCommandOptStringReq(ctl, cmd, pooloptname, &p) < 0) return NULL; - if (p) - pool = vshCommandOptPoolBy(ctl, cmd, pooloptname, name, flags); + if (p) { + if (!(pool = vshCommandOptPoolBy(ctl, cmd, pooloptname, name, flags))) + return NULL; + + if (virStoragePoolIsActive(pool) != 1) { + vshError(ctl, _("pool '%s' is not active"), p); + virStoragePoolFree(pool); + return NULL; + } + } vshDebug(ctl, VSH_ERR_DEBUG, "%s: found option <%s>: %s\n", cmd->def->name, optname, n); -- 1.9.0

--- src/storage/storage_driver.c | 89 ++++++++++++++++++++++---------------------- 1 file changed, 44 insertions(+), 45 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index e0ebdb0..942ba35 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1909,13 +1909,13 @@ storageVolDownload(virStorageVolPtr obj, virReportError(VIR_ERR_NO_STORAGE_POOL, _("no storage pool with matching name '%s'"), obj->pool); - goto out; + goto cleanup; } if (!virStoragePoolObjIsActive(pool)) { virReportError(VIR_ERR_OPERATION_INVALID, _("storage pool '%s' is not active"), pool->def->name); - goto out; + goto cleanup; } vol = virStorageVolDefFindByName(pool, obj->name); @@ -1924,28 +1924,28 @@ storageVolDownload(virStorageVolPtr obj, virReportError(VIR_ERR_NO_STORAGE_VOL, _("no storage vol with matching name '%s'"), obj->name); - goto out; + goto cleanup; } if (virStorageVolDownloadEnsureACL(obj->conn, pool->def, vol) < 0) - goto out; + goto cleanup; if (vol->building) { virReportError(VIR_ERR_OPERATION_INVALID, _("volume '%s' is still being allocated."), vol->name); - goto out; + goto cleanup; } if (virFDStreamOpenFile(stream, vol->target.path, offset, length, O_RDONLY) < 0) - goto out; + goto cleanup; ret = 0; -out: +cleanup: if (pool) virStoragePoolObjUnlock(pool); @@ -1975,13 +1975,13 @@ storageVolUpload(virStorageVolPtr obj, virReportError(VIR_ERR_NO_STORAGE_POOL, _("no storage pool with matching name '%s'"), obj->pool); - goto out; + goto cleanup; } if (!virStoragePoolObjIsActive(pool)) { virReportError(VIR_ERR_OPERATION_INVALID, _("storage pool '%s' is not active"), pool->def->name); - goto out; + goto cleanup; } vol = virStorageVolDefFindByName(pool, obj->name); @@ -1990,17 +1990,17 @@ storageVolUpload(virStorageVolPtr obj, virReportError(VIR_ERR_NO_STORAGE_VOL, _("no storage vol with matching name '%s'"), obj->name); - goto out; + goto cleanup; } if (virStorageVolUploadEnsureACL(obj->conn, pool->def, vol) < 0) - goto out; + goto cleanup; if (vol->building) { virReportError(VIR_ERR_OPERATION_INVALID, _("volume '%s' is still being allocated."), vol->name); - goto out; + goto cleanup; } /* Not using O_CREAT because the file is required to @@ -2009,11 +2009,11 @@ storageVolUpload(virStorageVolPtr obj, vol->target.path, offset, length, O_WRONLY) < 0) - goto out; + goto cleanup; ret = 0; -out: +cleanup: if (pool) virStoragePoolObjUnlock(pool); @@ -2044,17 +2044,17 @@ storageVolResize(virStorageVolPtr obj, virReportError(VIR_ERR_NO_STORAGE_POOL, _("no storage pool with matching name '%s'"), obj->pool); - goto out; + goto cleanup; } if (!virStoragePoolObjIsActive(pool)) { virReportError(VIR_ERR_OPERATION_INVALID, _("storage pool '%s' is not active"), pool->def->name); - goto out; + goto cleanup; } if ((backend = virStorageBackendForType(pool->def->type)) == NULL) - goto out; + goto cleanup; vol = virStorageVolDefFindByName(pool, obj->name); @@ -2062,17 +2062,17 @@ storageVolResize(virStorageVolPtr obj, virReportError(VIR_ERR_NO_STORAGE_VOL, _("no storage vol with matching name '%s'"), obj->name); - goto out; + goto cleanup; } if (virStorageVolResizeEnsureACL(obj->conn, pool->def, vol) < 0) - goto out; + goto cleanup; if (vol->building) { virReportError(VIR_ERR_OPERATION_INVALID, _("volume '%s' is still being allocated."), vol->name); - goto out; + goto cleanup; } if (flags & VIR_STORAGE_VOL_RESIZE_DELTA) { @@ -2086,7 +2086,7 @@ storageVolResize(virStorageVolPtr obj, virReportError(VIR_ERR_INVALID_ARG, "%s", _("can't shrink capacity below " "existing allocation")); - goto out; + goto cleanup; } if (abs_capacity < vol->capacity && @@ -2094,24 +2094,24 @@ storageVolResize(virStorageVolPtr obj, virReportError(VIR_ERR_INVALID_ARG, "%s", _("Can't shrink capacity below current " "capacity with shrink flag explicitly specified")); - goto out; + goto cleanup; } if (abs_capacity > vol->capacity + pool->def->available) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Not enough space left on storage pool")); - goto out; + goto cleanup; } if (!backend->resizeVol) { virReportError(VIR_ERR_NO_SUPPORT, "%s", _("storage pool does not support changing of " "volume capacity")); - goto out; + goto cleanup; } if (backend->resizeVol(obj->conn, pool, vol, abs_capacity, flags) < 0) - goto out; + goto cleanup; vol->capacity = abs_capacity; if (flags & VIR_STORAGE_VOL_RESIZE_ALLOCATE) @@ -2123,7 +2123,7 @@ storageVolResize(virStorageVolPtr obj, ret = 0; -out: +cleanup: if (pool) virStoragePoolObjUnlock(pool); @@ -2157,7 +2157,7 @@ storageVolZeroSparseFile(virStorageVolDefPtr vol, _("Failed to truncate volume with " "path '%s' to 0 bytes"), vol->target.path); - goto out; + return ret; } ret = ftruncate(fd, size); @@ -2168,7 +2168,6 @@ storageVolZeroSparseFile(virStorageVolDefPtr vol, vol->target.path, (uintmax_t)size); } -out: return ret; } @@ -2194,7 +2193,7 @@ storageWipeExtent(virStorageVolDefPtr vol, _("Failed to seek to position %ju in volume " "with path '%s'"), (uintmax_t)extent_start, vol->target.path); - goto out; + goto cleanup; } remaining = extent_length; @@ -2208,7 +2207,7 @@ storageWipeExtent(virStorageVolDefPtr vol, "storage volume with path '%s'"), write_size, vol->target.path); - goto out; + goto cleanup; } *bytes_wiped += written; @@ -2220,7 +2219,7 @@ storageWipeExtent(virStorageVolDefPtr vol, virReportSystemError(errno, _("cannot sync data to volume with path '%s'"), vol->target.path); - goto out; + goto cleanup; } VIR_DEBUG("Wrote %zu bytes to volume with path '%s'", @@ -2228,7 +2227,7 @@ storageWipeExtent(virStorageVolDefPtr vol, ret = 0; -out: +cleanup: return ret; } @@ -2251,14 +2250,14 @@ storageVolWipeInternal(virStorageVolDefPtr def, virReportSystemError(errno, _("Failed to open storage volume with path '%s'"), def->target.path); - goto out; + goto cleanup; } if (fstat(fd, &st) == -1) { virReportSystemError(errno, _("Failed to stat storage volume with path '%s'"), def->target.path); - goto out; + goto cleanup; } if (algorithm != VIR_STORAGE_VOL_WIPE_ALG_ZERO) { @@ -2298,17 +2297,17 @@ storageVolWipeInternal(virStorageVolDefPtr def, def->target.path, NULL); if (virCommandRun(cmd, NULL) < 0) - goto out; + goto cleanup; ret = 0; - goto out; + goto cleanup; } else { if (S_ISREG(st.st_mode) && st.st_blocks < (st.st_size / DEV_BSIZE)) { ret = storageVolZeroSparseFile(def, st.st_size, fd); } else { if (VIR_ALLOC_N(writebuf, st.st_blksize) < 0) - goto out; + goto cleanup; ret = storageWipeExtent(def, fd, @@ -2320,7 +2319,7 @@ storageVolWipeInternal(virStorageVolDefPtr def, } } -out: +cleanup: virCommandFree(cmd); VIR_FREE(writebuf); VIR_FORCE_CLOSE(fd); @@ -2355,13 +2354,13 @@ storageVolWipePattern(virStorageVolPtr obj, virReportError(VIR_ERR_NO_STORAGE_POOL, _("no storage pool with matching name '%s'"), obj->pool); - goto out; + goto cleanup; } if (!virStoragePoolObjIsActive(pool)) { virReportError(VIR_ERR_OPERATION_INVALID, _("storage pool '%s' is not active"), pool->def->name); - goto out; + goto cleanup; } vol = virStorageVolDefFindByName(pool, obj->name); @@ -2370,26 +2369,26 @@ storageVolWipePattern(virStorageVolPtr obj, virReportError(VIR_ERR_NO_STORAGE_VOL, _("no storage vol with matching name '%s'"), obj->name); - goto out; + goto cleanup; } if (virStorageVolWipePatternEnsureACL(obj->conn, pool->def, vol) < 0) - goto out; + goto cleanup; if (vol->building) { virReportError(VIR_ERR_OPERATION_INVALID, _("volume '%s' is still being allocated."), vol->name); - goto out; + goto cleanup; } if (storageVolWipeInternal(vol, algorithm) == -1) { - goto out; + goto cleanup; } ret = 0; -out: +cleanup: if (pool) { virStoragePoolObjUnlock(pool); } -- 1.9.0

Pools that are not backed by files in the filesystem cause problems with some APIs. Error out when attempting to upload a volume in such a pool as currently we expect a local file representation for it. --- src/storage/storage_driver.c | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 942ba35..79beb45 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2003,13 +2003,32 @@ storageVolUpload(virStorageVolPtr obj, goto cleanup; } - /* Not using O_CREAT because the file is required to - * already exist at this point */ - if (virFDStreamOpenFile(stream, - vol->target.path, - offset, length, - O_WRONLY) < 0) + switch ((enum virStoragePoolType) pool->def->type) { + case VIR_STORAGE_POOL_DIR: + case VIR_STORAGE_POOL_FS: + case VIR_STORAGE_POOL_NETFS: + case VIR_STORAGE_POOL_LOGICAL: + case VIR_STORAGE_POOL_DISK: + case VIR_STORAGE_POOL_ISCSI: + case VIR_STORAGE_POOL_SCSI: + case VIR_STORAGE_POOL_MPATH: + /* Not using O_CREAT because the file is required to already exist at + * this point */ + if (virFDStreamOpenFile(stream, vol->target.path, + offset, length, O_WRONLY) < 0) + goto cleanup; + + break; + + case VIR_STORAGE_POOL_SHEEPDOG: + case VIR_STORAGE_POOL_RBD: + case VIR_STORAGE_POOL_GLUSTER: + case VIR_STORAGE_POOL_LAST: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("volume upload is not supported with pools of type %s"), + virStoragePoolTypeToString(pool->def->type)); goto cleanup; + } ret = 0; -- 1.9.0

When looking up a volume by path on a non-local filesystem don't use the "cleaned" path that might be mangled in such a way that it will differ from a path provided by a storage backend. Skip the cleanup step for gluster, sheepdog and RBD. --- src/storage/storage_driver.c | 82 ++++++++++++++++++++++++++++---------------- 1 file changed, 53 insertions(+), 29 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 79beb45..7c31cb4 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1446,42 +1446,66 @@ storageVolLookupByPath(virConnectPtr conn, storageDriverLock(driver); for (i = 0; i < driver->pools.count && !ret; i++) { - virStoragePoolObjLock(driver->pools.objs[i]); - if (virStoragePoolObjIsActive(driver->pools.objs[i])) { - virStorageVolDefPtr vol; - char *stable_path; - - stable_path = virStorageBackendStablePath(driver->pools.objs[i], - cleanpath, - false); - if (stable_path == NULL) { - /* Don't break the whole lookup process if it fails on - * getting the stable path for some of the pools. - */ - VIR_WARN("Failed to get stable path for pool '%s'", - driver->pools.objs[i]->def->name); - virStoragePoolObjUnlock(driver->pools.objs[i]); - continue; - } + virStoragePoolObjPtr pool = driver->pools.objs[i]; + virStorageVolDefPtr vol; + char *stable_path = NULL; + + virStoragePoolObjLock(pool); - vol = virStorageVolDefFindByPath(driver->pools.objs[i], - stable_path); - VIR_FREE(stable_path); + if (!virStoragePoolObjIsActive(pool)) { + virStoragePoolObjUnlock(pool); + continue; + } - if (vol) { - if (virStorageVolLookupByPathEnsureACL(conn, driver->pools.objs[i]->def, vol) < 0) { - virStoragePoolObjUnlock(driver->pools.objs[i]); + switch ((enum virStoragePoolType) pool->def->type) { + case VIR_STORAGE_POOL_DIR: + case VIR_STORAGE_POOL_FS: + case VIR_STORAGE_POOL_NETFS: + case VIR_STORAGE_POOL_LOGICAL: + case VIR_STORAGE_POOL_DISK: + case VIR_STORAGE_POOL_ISCSI: + case VIR_STORAGE_POOL_SCSI: + case VIR_STORAGE_POOL_MPATH: + stable_path = virStorageBackendStablePath(pool, + cleanpath, + false); + if (stable_path == NULL) { + /* Don't break the whole lookup process if it fails on + * getting the stable path for some of the pools. + */ + VIR_WARN("Failed to get stable path for pool '%s'", + pool->def->name); + virStoragePoolObjUnlock(pool); + continue; + } + break; + + case VIR_STORAGE_POOL_GLUSTER: + case VIR_STORAGE_POOL_RBD: + case VIR_STORAGE_POOL_SHEEPDOG: + case VIR_STORAGE_POOL_LAST: + if (VIR_STRDUP(stable_path, path) < 0) { + virStoragePoolObjUnlock(pool); goto cleanup; } + break; + } - ret = virGetStorageVol(conn, - driver->pools.objs[i]->def->name, - vol->name, - vol->key, - NULL, NULL); + vol = virStorageVolDefFindByPath(pool, stable_path); + VIR_FREE(stable_path); + + if (vol) { + if (virStorageVolLookupByPathEnsureACL(conn, pool->def, vol) < 0) { + virStoragePoolObjUnlock(pool); + goto cleanup; } + + ret = virGetStorageVol(conn, pool->def->name, + vol->name, vol->key, + NULL, NULL); } - virStoragePoolObjUnlock(driver->pools.objs[i]); + + virStoragePoolObjUnlock(pool); } if (!ret) -- 1.9.0

In storageVolLookupByPath the provided path is "sanitized" at first. This removes some extra slashes and stuff. When the lookup of the volume fails the original path is used which makes it hard to trace errors in some cases. Improve the error message to print the sanitized path along with the user provided path if they are not equal. --- src/storage/storage_driver.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 7c31cb4..1b08619 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1508,9 +1508,16 @@ storageVolLookupByPath(virConnectPtr conn, virStoragePoolObjUnlock(pool); } - if (!ret) - virReportError(VIR_ERR_NO_STORAGE_VOL, - _("no storage vol with matching path %s"), path); + if (!ret) { + if (STREQ(path, cleanpath)) { + virReportError(VIR_ERR_NO_STORAGE_VOL, + _("no storage vol with matching path '%s'"), path); + } else { + virReportError(VIR_ERR_NO_STORAGE_VOL, + _("no storage vol with matching path '%s' (%s)"), + path, cleanpath); + } + } cleanup: VIR_FREE(cleanpath); -- 1.9.0

With most of our storage backends it's possible to have two separate volume keys to point to a single volume. (By creating sym/hard-links to local files or by mounting remote filesystems to two different locations and creating pools on top of them) Document this possibility. --- docs/formatstorage.html.in | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index a089a31..1cd82b4 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -311,8 +311,10 @@ <dd>Providing a name for the volume which is unique to the pool. This is mandatory when defining a volume. <span class="since">Since 0.4.1</span></dd> <dt><code>key</code></dt> - <dd>Providing an identifier for the volume which is globally unique. - This cannot be set when creating a volume: it is always generated. + <dd>Providing an identifier for the volume which identifies a + single volume. In some cases it's possible to have two distinct keys + identifying a single volume. This field cannot be set when creating + a volume: it is always generated. <span class="since">Since 0.4.1</span></dd> <dt><code>allocation</code></dt> <dd>Providing the total storage allocation for the volume. This -- 1.9.0