[libvirt] [PATCH 00/12] Have 'buildVol' callers to clean up after themselves
NOTE: Although one may consider this a v2 of : http://www.redhat.com/archives/libvir-list/2015-October/msg00196.html It's more tackling the same problem a different way... Rather than pass a 'created' boolean around, this series investigated each of the 'createVol' and 'buildVol' paths in order to make more conscious decisions related to whether or not a volume was created and to handle failures after creation by deleting the volume/file. The end result is that all the 'buildVol' backends will now delete the volume they created on error paths leaving the storage driver to only need to remove the volume from the pool. As such this series will also revert a prior patch in this area. John Ferlan (12): storage: Remove duplicitous refreshVol in RBD buildVol storage: Remove duplicitous refreshVol in Sheepdog buildVol storage: Fix a resource leak in storageVolCreateXML storage: Track successful creation of LV for removal storage: On error unlink created file in virFileOpen{As|Forked} storage: On error rmdir created directory in virDirCreate[NoFork] storage: Rework error paths for virStorageBackendCreateExecCommand storage: Cleanup failures virStorageBackendCreateExecCommand storage: Cleanup failures in virStorageBackendCreateRaw storage: Pull volume removal from pool in storageVolDeleteInternal Revert "storage: Prior to creating a volume, refresh the pool" storage: On 'buildVol' failure don't delete the volume src/storage/storage_backend.c | 38 +++++++++++++++++------ src/storage/storage_backend_logical.c | 5 ++- src/storage/storage_backend_rbd.c | 3 -- src/storage/storage_backend_sheepdog.c | 5 +-- src/storage/storage_driver.c | 56 ++++++++++++++++++++-------------- src/util/virfile.c | 22 ++++++++++++- 6 files changed, 88 insertions(+), 41 deletions(-) -- 2.1.0
As of commit id '155ca616' a 'refreshVol' is called after the buildVol succeeds in storageVolCreateXML, thus the volStorageBackendRBDRefreshVolInfo call in virStorageBackendRBDBuildVol is no longer necessary. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_rbd.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index ac5085a..5ae4713 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -532,9 +532,6 @@ virStorageBackendRBDBuildVol(virConnectPtr conn, goto cleanup; } - if (volStorageBackendRBDRefreshVolInfo(vol, pool, &ptr) < 0) - goto cleanup; - ret = 0; cleanup: -- 2.1.0
On Fri, Oct 09, 2015 at 09:34:00 -0400, John Ferlan wrote:
As of commit id '155ca616' a 'refreshVol' is called after the buildVol succeeds in storageVolCreateXML, thus the volStorageBackendRBDRefreshVolInfo call in virStorageBackendRBDBuildVol is no longer necessary.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_rbd.c | 3 --- 1 file changed, 3 deletions(-)
ACK, Peter
As of commit id '155ca616' a 'refreshVol' is called after a buildVol succeeds in storageVolCreateXML, thus a volStorageBackendSheepdogRefreshVolInfo call in virStorageBackendSheepdogBuildVol is no longer necessary. Additionally, the 'conn' parameter becomes unused. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_sheepdog.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c index 69ba7836..1200813 100644 --- a/src/storage/storage_backend_sheepdog.c +++ b/src/storage/storage_backend_sheepdog.c @@ -257,7 +257,7 @@ virStorageBackendSheepdogCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED, static int -virStorageBackendSheepdogBuildVol(virConnectPtr conn, +virStorageBackendSheepdogBuildVol(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool, virStorageVolDefPtr vol, unsigned int flags) @@ -279,9 +279,6 @@ virStorageBackendSheepdogBuildVol(virConnectPtr conn, if (virCommandRun(cmd, NULL) < 0) goto cleanup; - if (virStorageBackendSheepdogRefreshVol(conn, pool, vol) < 0) - goto cleanup; - ret = 0; cleanup: virCommandFree(cmd); -- 2.1.0
On Fri, Oct 09, 2015 at 09:34:01 -0400, John Ferlan wrote:
As of commit id '155ca616' a 'refreshVol' is called after a buildVol succeeds in storageVolCreateXML, thus a volStorageBackendSheepdogRefreshVolInfo call in virStorageBackendSheepdogBuildVol is no longer necessary.
Additionally, the 'conn' parameter becomes unused.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_sheepdog.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
ACK, Peter
Commit id '1b5685da' refactored the code to move buildvoldef inside the buildVol conditional; however, the VIR_FREE of the memory was left only when 'buildret' failed, thus we're leaking memory. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index ddf4405..1c8ab87 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1859,6 +1859,8 @@ storageVolCreateXML(virStoragePoolPtr obj, buildret = backend->buildVol(obj->conn, pool, buildvoldef, flags); + VIR_FREE(buildvoldef); + storageDriverLock(); virStoragePoolObjLock(pool); storageDriverUnlock(); @@ -1867,7 +1869,6 @@ storageVolCreateXML(virStoragePoolPtr obj, pool->asyncjobs--; if (buildret < 0) { - VIR_FREE(buildvoldef); storageVolDeleteInternal(volobj, backend, pool, voldef, 0, false); voldef = NULL; -- 2.1.0
On Fri, Oct 09, 2015 at 09:34:02 -0400, John Ferlan wrote:
Commit id '1b5685da' refactored the code to move buildvoldef inside the buildVol conditional; however, the VIR_FREE of the memory was left only when 'buildret' failed, thus we're leaking memory.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
ACK, Peter
https://bugzilla.redhat.com/show_bug.cgi?id=1233003 Track when the logical volume was successfully created in order to properly handle the call to virStorageBackendLogicalDeleteVol. It's possible that the failure to create was because someone created an LV in the pool outside of libvirt's knowledge. In this case, we don't want to delete that LV. A subsequent or future refresh of the pool will find the volume and cause an earlier failure Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 070f2bd..f1321db 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -731,6 +731,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, virCommandPtr cmd = NULL; virErrorPtr err; struct stat sb; + bool created = false; if (vol->target.encryption != NULL) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -771,6 +772,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, if (virCommandRun(cmd, NULL) < 0) goto error; + created = true; virCommandFree(cmd); cmd = NULL; @@ -816,7 +818,8 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, error: err = virSaveLastError(); VIR_FORCE_CLOSE(fd); - virStorageBackendLogicalDeleteVol(conn, pool, vol, 0); + if (created) + virStorageBackendLogicalDeleteVol(conn, pool, vol, 0); virCommandFree(cmd); virSetError(err); virFreeError(err); -- 2.1.0
On Fri, Oct 09, 2015 at 09:34:03 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1233003
Track when the logical volume was successfully created in order to properly handle the call to virStorageBackendLogicalDeleteVol. It's possible that the failure to create was because someone created an LV in the pool outside of libvirt's knowledge. In this case, we don't want to delete that LV. A subsequent or future refresh of the pool will find the volume and cause an earlier failure
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Definitely better than the previous version. ACK, Peter
After a successful creation of a file, if some other call results in returning a failure, let's unlink the file we created to prevent another round trip or confusion in the caller. In particular, this function can be called during a storage backend buildVol, so in order to ensure that caller doesn't need to distinguish between failed create or some other failure after create, just remove the volume we created. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virfile.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index a81f04c..51198e2 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2069,6 +2069,7 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, int pair[2] = { -1, -1 }; gid_t *groups; int ngroups; + bool created = false; /* parent is running as root, but caller requested that the * file be opened as some other user and/or group). The @@ -2113,6 +2114,8 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, path); goto childerror; } + if (openflags & O_CREAT) + created = true; /* File is successfully open. Set permissions if requested. */ ret = virFileOpenForceOwnerMode(path, fd, mode, uid, gid, flags); @@ -2141,8 +2144,11 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, /* XXX This makes assumptions about errno being < 255, which is * not true on Hurd. */ VIR_FORCE_CLOSE(pair[1]); - if (ret < 0) + if (ret < 0) { VIR_FORCE_CLOSE(fd); + if (created) + unlink(path); + } ret = -ret; if ((ret & 0xff) != ret) { VIR_WARN("unable to pass desired return value %d", ret); @@ -2225,6 +2231,7 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, uid_t uid, gid_t gid, unsigned int flags) { int ret = 0, fd = -1; + bool created = false; /* allow using -1 to mean "current value" */ if (uid == (uid_t) -1) @@ -2246,6 +2253,8 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, if (!(flags & VIR_FILE_OPEN_FORK)) goto error; } else { + if (openflags & O_CREAT) + created = true; ret = virFileOpenForceOwnerMode(path, fd, mode, uid, gid, flags); if (ret < 0) goto error; @@ -2288,6 +2297,8 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, if (fd >= 0) { /* some other failure after the open succeeded */ VIR_FORCE_CLOSE(fd); + if (created) + unlink(path); } /* whoever failed the open last has already set ret = -errno */ return ret; -- 2.1.0
On Fri, Oct 09, 2015 at 09:34:04 -0400, John Ferlan wrote:
After a successful creation of a file, if some other call results in returning a failure, let's unlink the file we created to prevent another round trip or confusion in the caller. In particular, this function can be called during a storage backend buildVol, so in order to ensure that caller doesn't need to distinguish between failed create or some other failure after create, just remove the volume we created.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virfile.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c index a81f04c..51198e2 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c
...
@@ -2069,6 +2069,7 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, @@ -2225,6 +2231,7 @@ virFileOpenAs(const char *path, int openflags, mode_t mode,
Both of the modified functions have comments explaining them, but this patch doesn't modify them. Code looks definitely cleaner though. ACK if you explain the change in the function headers. Peter
After a successful creation of a directory, if some other call results in returning a failure, let's remove the directory we created to prevent another round trip or confusion in the caller. In particular, this function can be called during a storage backend buildVol, so in order to ensure that caller doesn't need to distinguish between failed create or some other failure after create, just remove the directory we created. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virfile.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/util/virfile.c b/src/util/virfile.c index 51198e2..2cb2469 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2420,6 +2420,7 @@ virDirCreateNoFork(const char *path, { int ret = 0; struct stat st; + bool created = false; if (!((flags & VIR_DIR_CREATE_ALLOW_EXIST) && virFileExists(path))) { if (mkdir(path, mode) < 0) { @@ -2428,6 +2429,7 @@ virDirCreateNoFork(const char *path, path); goto error; } + created = true; } if (stat(path, &st) == -1) { @@ -2451,6 +2453,8 @@ virDirCreateNoFork(const char *path, goto error; } error: + if (ret < 0 && created) + rmdir(path); return ret; } @@ -2465,6 +2469,7 @@ virDirCreate(const char *path, int status = 0, ret = 0; gid_t *groups; int ngroups; + bool created = false; /* Everything after this check is crazyness to allow setting uid/gid * on directories that are on root-squash NFS shares. We only want @@ -2552,6 +2557,7 @@ virDirCreate(const char *path, } goto childerror; } + created = true; /* check if group was set properly by creating after * setgid. If not, try doing it with chown */ @@ -2578,6 +2584,9 @@ virDirCreate(const char *path, } childerror: + if (ret != 0 && created) + rmdir(path); + if ((ret & 0xff) != ret) { VIR_WARN("unable to pass desired return value %d", ret); ret = 0xff; -- 2.1.0
On Fri, Oct 09, 2015 at 09:34:05 -0400, John Ferlan wrote:
After a successful creation of a directory, if some other call results in returning a failure, let's remove the directory we created to prevent another round trip or confusion in the caller. In particular, this function can be called during a storage backend buildVol, so in order to ensure that caller doesn't need to distinguish between failed create or some other failure after create, just remove the directory we created.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virfile.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/src/util/virfile.c b/src/util/virfile.c index 51198e2..2cb2469 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c
...
@@ -2420,6 +2420,7 @@ virDirCreateNoFork(const char *path, @@ -2465,6 +2469,7 @@ virDirCreate(const char *path,
Please document the semantics in the function comments for the two functions. ACK with docs. Peter
Rework the code in order to use the "ret = -1;" and goto cleanup; coding style. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index ad7a576..a375fe0 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -679,6 +679,7 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, uid_t uid; mode_t mode; bool filecreated = false; + int ret = -1; if ((pool->def->type == VIR_STORAGE_POOL_NETFS) && (((geteuid() == 0) @@ -703,11 +704,11 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, if (!filecreated) { if (virCommandRun(cmd, NULL) < 0) - return -1; + goto cleanup; if (stat(vol->target.path, &st) < 0) { virReportSystemError(errno, _("failed to create %s"), vol->target.path); - return -1; + goto cleanup; } } @@ -721,7 +722,7 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, _("cannot chown %s to (%u, %u)"), vol->target.path, (unsigned int) uid, (unsigned int) gid); - return -1; + goto cleanup; } mode = (vol->target.perms->mode == (mode_t) -1 ? @@ -730,9 +731,13 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, virReportSystemError(errno, _("cannot set mode of '%s' to %04o"), vol->target.path, mode); - return -1; + goto cleanup; } - return 0; + + ret = 0; + + cleanup: + return ret; } enum { -- 2.1.0
On Fri, Oct 09, 2015 at 09:34:06 -0400, John Ferlan wrote:
Rework the code in order to use the "ret = -1;" and goto cleanup; coding style.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
ACK, Peter
After a successful qemu-img/qcow-create of the backing file, if we fail to stat the file, change it owner/group, or mode, then the cleanup path should delete the file. Also moved the virCommandSetUID/virCommandSetGID inside the condition used to actually run the command rather than randomly setting and not using it if the file had been created. The 'cmd' buffer is only used if we need to create. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a375fe0..7d0de63 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -698,13 +698,16 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, } } - /* don't change uid/gid if we retry */ - virCommandSetUID(cmd, -1); - virCommandSetGID(cmd, -1); - if (!filecreated) { + /* don't change uid/gid if we retry */ + virCommandSetUID(cmd, -1); + virCommandSetGID(cmd, -1); + if (virCommandRun(cmd, NULL) < 0) goto cleanup; + + filecreated = true; + if (stat(vol->target.path, &st) < 0) { virReportSystemError(errno, _("failed to create %s"), vol->target.path); @@ -737,6 +740,8 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, ret = 0; cleanup: + if (ret < 0 && filecreated) + unlink(vol->target.path); return ret; } -- 2.1.0
On Fri, Oct 09, 2015 at 09:34:07 -0400, John Ferlan wrote:
After a successful qemu-img/qcow-create of the backing file, if we fail to stat the file, change it owner/group, or mode, then the cleanup path should delete the file.
Also moved the virCommandSetUID/virCommandSetGID inside the condition used to actually run the command rather than randomly setting and not using it if the file had been created. The 'cmd' buffer is only used if we need to create.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a375fe0..7d0de63 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c
...
@@ -737,6 +740,8 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, ret = 0;
cleanup: + if (ret < 0 && filecreated) + unlink(vol->target.path);
This might not work if the volume was created with different uid/gid as the process that is attempting to delete it (in case of e.g. NFS.). Peter
On 10/13/2015 08:50 AM, Peter Krempa wrote:
On Fri, Oct 09, 2015 at 09:34:07 -0400, John Ferlan wrote:
After a successful qemu-img/qcow-create of the backing file, if we fail to stat the file, change it owner/group, or mode, then the cleanup path should delete the file.
Also moved the virCommandSetUID/virCommandSetGID inside the condition used to actually run the command rather than randomly setting and not using it if the file had been created. The 'cmd' buffer is only used if we need to create.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a375fe0..7d0de63 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c
...
@@ -737,6 +740,8 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, ret = 0;
cleanup: + if (ret < 0 && filecreated) + unlink(vol->target.path);
This might not work if the volume was created with different uid/gid as the process that is attempting to delete it (in case of e.g. NFS.).
Ugh... this function was really strange especially that chown/chmod after a create on an NETFS target. The net result if the first pile of 'ifs' passes is a creation in a NETFS pool using either 'qemu-img' or 'qcow-create' under the uid/gid from the vol->target.{uid|gid}. So yes, we'd have to virFileRemove that. The other creation would able to unlink directly, although I suppose a revector into virFileRemove would work, although it'd be passing uid, gid == -1, -1. I know you don't necessarily prefer inline diffs, but this one's fairly short: - if (ret < 0 && filecreated) - unlink(vol->target.path); + if (ret < 0 && filecreated) { + if (netfs) + virFileDelete(vol->target.path, vol->target.uid, vol->target.gid); + else + unlink(vol->target.path); + } where 'netfs' is a new bool set when we create in that first if Does this look more reasonable? John
On 10/13/2015 03:11 PM, John Ferlan wrote:
On 10/13/2015 08:50 AM, Peter Krempa wrote:
On Fri, Oct 09, 2015 at 09:34:07 -0400, John Ferlan wrote:
After a successful qemu-img/qcow-create of the backing file, if we fail to stat the file, change it owner/group, or mode, then the cleanup path should delete the file.
Also moved the virCommandSetUID/virCommandSetGID inside the condition used to actually run the command rather than randomly setting and not using it if the file had been created. The 'cmd' buffer is only used if we need to create.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a375fe0..7d0de63 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c
...
@@ -737,6 +740,8 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, ret = 0;
cleanup: + if (ret < 0 && filecreated) + unlink(vol->target.path);
This might not work if the volume was created with different uid/gid as the process that is attempting to delete it (in case of e.g. NFS.).
Ugh... this function was really strange especially that chown/chmod after a create on an NETFS target. The net result if the first pile of 'ifs' passes is a creation in a NETFS pool using either 'qemu-img' or 'qcow-create' under the uid/gid from the vol->target.{uid|gid}. So yes, we'd have to virFileRemove that.
The other creation would able to unlink directly, although I suppose a revector into virFileRemove would work, although it'd be passing uid, gid == -1, -1.
I know you don't necessarily prefer inline diffs, but this one's fairly short:
- if (ret < 0 && filecreated) - unlink(vol->target.path); + if (ret < 0 && filecreated) { + if (netfs) + virFileDelete(vol->target.path, vol->target.uid, vol->target.gid);
virFileRemove(vol->target.path, vol->target.perms->uid, vol->target.perms->gid); What I get for typing and not compiling ;-) John
+ else + unlink(vol->target.path); + }
where 'netfs' is a new bool set when we create in that first if
Does this look more reasonable?
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Oct 13, 2015 at 15:11:01 -0400, John Ferlan wrote:
On 10/13/2015 08:50 AM, Peter Krempa wrote:
On Fri, Oct 09, 2015 at 09:34:07 -0400, John Ferlan wrote:
...
This might not work if the volume was created with different uid/gid as the process that is attempting to delete it (in case of e.g. NFS.).
Ugh... this function was really strange especially that chown/chmod after a create on an NETFS target. The net result if the first pile of 'ifs' passes is a creation in a NETFS pool using either 'qemu-img' or 'qcow-create' under the uid/gid from the vol->target.{uid|gid}. So yes, we'd have to virFileRemove that.
It might be a better option to refactor it prior to tweaking the cleanup path.
The other creation would able to unlink directly, although I suppose a revector into virFileRemove would work, although it'd be passing uid, gid == -1, -1.
I know you don't necessarily prefer inline diffs, but this one's fairly short:
- if (ret < 0 && filecreated) - unlink(vol->target.path); + if (ret < 0 && filecreated) { + if (netfs) + virFileDelete(vol->target.path, vol->target.uid, vol->target.gid); + else + unlink(vol->target.path); + }
where 'netfs' is a new bool set when we create in that first if
Does this look more reasonable?
Perhaps even without the netfs check if you think it makes sense so that the logic isn't overcomplicated. Peter
On 10/14/2015 08:21 AM, Peter Krempa wrote:
On Tue, Oct 13, 2015 at 15:11:01 -0400, John Ferlan wrote:
On 10/13/2015 08:50 AM, Peter Krempa wrote:
On Fri, Oct 09, 2015 at 09:34:07 -0400, John Ferlan wrote:
...
This might not work if the volume was created with different uid/gid as the process that is attempting to delete it (in case of e.g. NFS.).
Ugh... this function was really strange especially that chown/chmod after a create on an NETFS target. The net result if the first pile of 'ifs' passes is a creation in a NETFS pool using either 'qemu-img' or 'qcow-create' under the uid/gid from the vol->target.{uid|gid}. So yes, we'd have to virFileRemove that.
It might be a better option to refactor it prior to tweaking the cleanup path.
I think for the uid/gid && chown paths - yes, I can see that. The way I read the code, we'll get the uid/gid of the created file from the NFS pool and it'll be the same as "vol->target.perms->{uid|gid}", thus we won't make the chown call because the uid/gid will be set to -1. So I think it could be refactored inside the "if (!filecreated)". However, it's the chmod path that's got me thinking a bit harder. It seems that code should check "st.st_mode != mode" before calling - although chmod could consider that a noop before it "checks" whether the calling process has the ability to perform the change. Regardless, since the NETFS path doesn't include setting mode bits (e.g. generating a "mask" to be used in the umask at the start of the command processing), thus it seems that path would attempt the chmod path. Although perhaps we've been "fortunate" to not use this path for NETFS since it seems logically to me it would have failed in the funky root squash environment. I'll have to give this path a try since I have root squash pool configured. John
The other creation would able to unlink directly, although I suppose a revector into virFileRemove would work, although it'd be passing uid, gid == -1, -1.
I know you don't necessarily prefer inline diffs, but this one's fairly short:
- if (ret < 0 && filecreated) - unlink(vol->target.path); + if (ret < 0 && filecreated) { + if (netfs) + virFileDelete(vol->target.path, vol->target.uid, vol->target.gid); + else + unlink(vol->target.path); + }
where 'netfs' is a new bool set when we create in that first if
Does this look more reasonable?
Perhaps even without the netfs check if you think it makes sense so that the logic isn't overcomplicated.
Peter
Refactor the code to handle the NETFS pool separately from other pool types. When the original code was developed (commit id 'e1f27784') the virCommandSetUmask did not exist (commit id '0e1a1a8c4'), so there was no way to set the permissions for the creation. Now that it exists, we can use it. The code currently fails to create a volume in a NETFS pool from some other volume within the pool because the chmod done after file creation fails. So adjust the code to jump to cleanup once the file is successfully created in the NETFS pool. If it fails or for non NETFS files, just perform the create, chown, and chmod in order Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a375fe0..037d6d7 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -678,7 +678,6 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, gid_t gid; uid_t uid; mode_t mode; - bool filecreated = false; int ret = -1; if ((pool->def->type == VIR_STORAGE_POOL_NETFS) @@ -690,26 +689,29 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, virCommandSetUID(cmd, vol->target.perms->uid); virCommandSetGID(cmd, vol->target.perms->gid); + mode = (vol->target.perms->mode == (mode_t) -1 ? + VIR_STORAGE_DEFAULT_VOL_PERM_MODE : vol->target.perms->mode); + virCommandSetUmask(cmd, 0777 - mode); - if (virCommandRun(cmd, NULL) == 0) { - /* command was successfully run, check if the file was created */ - if (stat(vol->target.path, &st) >= 0) - filecreated = true; + if ((ret = virCommandRun(cmd, NULL)) == 0) { + /* command was successfully run, if the file was created + * then we are done */ + if (virFileExists(vol->target.path)) + goto cleanup; } } - /* don't change uid/gid if we retry */ + /* don't change uid/gid/mode if we retry */ virCommandSetUID(cmd, -1); virCommandSetGID(cmd, -1); + virCommandSetUmask(cmd, 0); - if (!filecreated) { - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; - if (stat(vol->target.path, &st) < 0) { - virReportSystemError(errno, - _("failed to create %s"), vol->target.path); - goto cleanup; - } + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + if (stat(vol->target.path, &st) < 0) { + virReportSystemError(errno, + _("failed to create %s"), vol->target.path); + goto cleanup; } uid = (vol->target.perms->uid != st.st_uid) ? vol->target.perms->uid -- 2.1.0
Yes, this should have read 8/12... On 10/14/2015 02:01 PM, John Ferlan wrote:
Refactor the code to handle the NETFS pool separately from other pool types. When the original code was developed (commit id 'e1f27784') the virCommandSetUmask did not exist (commit id '0e1a1a8c4'), so there was no way to set the permissions for the creation. Now that it exists, we can use it. The code currently fails to create a volume in a NETFS pool from some other volume within the pool because the chmod done after file creation fails.
So adjust the code to jump to cleanup once the file is successfully created in the NETFS pool. If it fails or for non NETFS files, just perform the create, chown, and chmod in order
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a375fe0..037d6d7 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -678,7 +678,6 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, gid_t gid; uid_t uid; mode_t mode; - bool filecreated = false; int ret = -1;
if ((pool->def->type == VIR_STORAGE_POOL_NETFS) @@ -690,26 +689,29 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool,
virCommandSetUID(cmd, vol->target.perms->uid); virCommandSetGID(cmd, vol->target.perms->gid); + mode = (vol->target.perms->mode == (mode_t) -1 ? + VIR_STORAGE_DEFAULT_VOL_PERM_MODE : vol->target.perms->mode); + virCommandSetUmask(cmd, 0777 - mode);
^^^ Just to be clear - this fixes a bug in the existing code which cannot create 'qcow2' file in an NFS pool that has root squash enabled. The failure "currently" is : virsh vol-create-from bz1253609 qemu-img.xml test.img --inputpool bz1253609 error: Failed to create vol from qemu-img.xml error: cannot set mode of '/tmp/netfs-pool/qemu-img-test.img' to 0644: Operation not permitted Assuming that 'test.img' was created using : virsh vol-create bz1253609 vol.xml The qemu-img.xml is: <volume> <name>qemu-img-test.img</name> <source> </source> <capacity>4048576000</capacity> <allocation>0</allocation> <target> <format type='qcow2'/> <permissions> <mode>0644</mode> <owner>107</owner> <group>107</group> <label>system_u:object_r:nfs_t:s0</label> </permissions> </target> </volume> it fails it other ways if <mode> and/or <owner>/<group> are not supplied. FWIW: The 'vol.xml' file differs from the above by only the "type='raw'" John
- if (virCommandRun(cmd, NULL) == 0) { - /* command was successfully run, check if the file was created */ - if (stat(vol->target.path, &st) >= 0) - filecreated = true; + if ((ret = virCommandRun(cmd, NULL)) == 0) { + /* command was successfully run, if the file was created + * then we are done */ + if (virFileExists(vol->target.path)) + goto cleanup; } }
- /* don't change uid/gid if we retry */ + /* don't change uid/gid/mode if we retry */ virCommandSetUID(cmd, -1); virCommandSetGID(cmd, -1); + virCommandSetUmask(cmd, 0);
- if (!filecreated) { - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; - if (stat(vol->target.path, &st) < 0) { - virReportSystemError(errno, - _("failed to create %s"), vol->target.path); - goto cleanup; - } + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + if (stat(vol->target.path, &st) < 0) { + virReportSystemError(errno, + _("failed to create %s"), vol->target.path); + goto cleanup; }
uid = (vol->target.perms->uid != st.st_uid) ? vol->target.perms->uid
Ping or should I just resend what's left rather than 3 adjusted and inserted patches? Tks, John On 10/14/2015 02:01 PM, John Ferlan wrote:
Refactor the code to handle the NETFS pool separately from other pool types. When the original code was developed (commit id 'e1f27784') the virCommandSetUmask did not exist (commit id '0e1a1a8c4'), so there was no way to set the permissions for the creation. Now that it exists, we can use it. The code currently fails to create a volume in a NETFS pool from some other volume within the pool because the chmod done after file creation fails.
So adjust the code to jump to cleanup once the file is successfully created in the NETFS pool. If it fails or for non NETFS files, just perform the create, chown, and chmod in order
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a375fe0..037d6d7 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -678,7 +678,6 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, gid_t gid; uid_t uid; mode_t mode; - bool filecreated = false; int ret = -1;
if ((pool->def->type == VIR_STORAGE_POOL_NETFS) @@ -690,26 +689,29 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool,
virCommandSetUID(cmd, vol->target.perms->uid); virCommandSetGID(cmd, vol->target.perms->gid); + mode = (vol->target.perms->mode == (mode_t) -1 ? + VIR_STORAGE_DEFAULT_VOL_PERM_MODE : vol->target.perms->mode); + virCommandSetUmask(cmd, 0777 - mode);
- if (virCommandRun(cmd, NULL) == 0) { - /* command was successfully run, check if the file was created */ - if (stat(vol->target.path, &st) >= 0) - filecreated = true; + if ((ret = virCommandRun(cmd, NULL)) == 0) { + /* command was successfully run, if the file was created + * then we are done */ + if (virFileExists(vol->target.path)) + goto cleanup; } }
- /* don't change uid/gid if we retry */ + /* don't change uid/gid/mode if we retry */ virCommandSetUID(cmd, -1); virCommandSetGID(cmd, -1); + virCommandSetUmask(cmd, 0);
- if (!filecreated) { - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; - if (stat(vol->target.path, &st) < 0) { - virReportSystemError(errno, - _("failed to create %s"), vol->target.path); - goto cleanup; - } + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + if (stat(vol->target.path, &st) < 0) { + virReportSystemError(errno, + _("failed to create %s"), vol->target.path); + goto cleanup; }
uid = (vol->target.perms->uid != st.st_uid) ? vol->target.perms->uid
After a successful qemu-img/qcow-create of the backing file, if we fail to stat the file, change it owner/group, or mode, then the cleanup path should delete the file. Signed-off-by: John Ferlan <jferlan@redhat.com> --- Well I obviously screwed up the number on the previous one where I put 7/12 instead of 8/12 <sigh> src/storage/storage_backend.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 037d6d7..487c914 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -678,6 +678,7 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, gid_t gid; uid_t uid; mode_t mode; + bool filecreated = false; int ret = -1; if ((pool->def->type == VIR_STORAGE_POOL_NETFS) @@ -708,6 +709,9 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, if (virCommandRun(cmd, NULL) < 0) goto cleanup; + + filecreated = true; + if (stat(vol->target.path, &st) < 0) { virReportSystemError(errno, _("failed to create %s"), vol->target.path); @@ -739,6 +743,8 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, ret = 0; cleanup: + if (ret < 0 && filecreated) + unlink(vol->target.path); return ret; } -- 2.1.0
If the volume target path doesn't exist prior to calling virFileOpenAs and we have a successful call, then we know we've had a successful creation. For any other failures in the function we should clean up after ourselves by using virFileDelete if we're about to return failure. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 7d0de63..32f85ac 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -485,6 +485,8 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, int operation_flags; bool reflink_copy = false; mode_t open_mode = VIR_STORAGE_DEFAULT_VOL_PERM_MODE; + bool exists = false; + bool created = false; virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA | VIR_STORAGE_VOL_CREATE_REFLINK, @@ -520,6 +522,8 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, if (vol->target.perms->mode != (mode_t) -1) open_mode = vol->target.perms->mode; + if (virFileExists(vol->target.path)) + exists = true; if ((fd = virFileOpenAs(vol->target.path, O_RDWR | O_CREAT | O_EXCL, open_mode, @@ -531,6 +535,8 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, vol->target.path); goto cleanup; } + if (!exists) + created = true; if (vol->target.nocow) { #ifdef __linux__ @@ -557,6 +563,10 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, ret = -1; cleanup: + if (ret < 0 && created) + ignore_value(virFileRemove(vol->target.path, + vol->target.perms->uid, + vol->target.perms->gid)); VIR_FORCE_CLOSE(fd); return ret; } -- 2.1.0
On Fri, Oct 09, 2015 at 09:34:08 -0400, John Ferlan wrote:
If the volume target path doesn't exist prior to calling virFileOpenAs and we have a successful call, then we know we've had a successful creation. For any other failures in the function we should clean up after ourselves by using virFileDelete if we're about to return failure.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 7d0de63..32f85ac 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c
...
@@ -520,6 +522,8 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, if (vol->target.perms->mode != (mode_t) -1) open_mode = vol->target.perms->mode;
+ if (virFileExists(vol->target.path)) + exists = true;
Why even bother checking? Shouldn't this function fail right away if the target file exists? Peter
On 10/13/2015 08:43 AM, Peter Krempa wrote:
On Fri, Oct 09, 2015 at 09:34:08 -0400, John Ferlan wrote:
If the volume target path doesn't exist prior to calling virFileOpenAs and we have a successful call, then we know we've had a successful creation. For any other failures in the function we should clean up after ourselves by using virFileDelete if we're about to return failure.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 7d0de63..32f85ac 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c
...
@@ -520,6 +522,8 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, if (vol->target.perms->mode != (mode_t) -1) open_mode = vol->target.perms->mode;
+ if (virFileExists(vol->target.path)) + exists = true;
Why even bother checking? Shouldn't this function fail right away if the target file exists?
Paranoia. Which function? I assume you mean *CreateRaw. Nothing in the function checks for existence. And while there isn't a caller currently that wouldn't be creating, I just wanted to be sure and perhaps future proof. I have no qualms in removing if that's felt is what is right. John
On Tue, Oct 13, 2015 at 16:12:46 -0400, John Ferlan wrote:
On 10/13/2015 08:43 AM, Peter Krempa wrote:
On Fri, Oct 09, 2015 at 09:34:08 -0400, John Ferlan wrote:
...
@@ -520,6 +522,8 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, if (vol->target.perms->mode != (mode_t) -1) open_mode = vol->target.perms->mode;
+ if (virFileExists(vol->target.path)) + exists = true;
Why even bother checking? Shouldn't this function fail right away if the target file exists?
Paranoia. Which function? I assume you mean *CreateRaw. Nothing in the function checks for existence. And while there isn't a caller currently that wouldn't be creating, I just wanted to be sure and perhaps future proof.
Erm, you've patched virFileOpenAs a few patches before this to do the right thing when creating the file so I don't see a reason to make sure again.
I have no qualms in removing if that's felt is what is right.
... Peter
On 10/14/2015 08:16 AM, Peter Krempa wrote:
On Tue, Oct 13, 2015 at 16:12:46 -0400, John Ferlan wrote:
On 10/13/2015 08:43 AM, Peter Krempa wrote:
On Fri, Oct 09, 2015 at 09:34:08 -0400, John Ferlan wrote:
...
@@ -520,6 +522,8 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, if (vol->target.perms->mode != (mode_t) -1) open_mode = vol->target.perms->mode;
+ if (virFileExists(vol->target.path)) + exists = true;
Why even bother checking? Shouldn't this function fail right away if the target file exists?
Paranoia. Which function? I assume you mean *CreateRaw. Nothing in the function checks for existence. And while there isn't a caller currently that wouldn't be creating, I just wanted to be sure and perhaps future proof.
Erm, you've patched virFileOpenAs a few patches before this to do the right thing when creating the file so I don't see a reason to make sure again.
I know that, look closer at the code after the call to virFileOpenAs. We know that if virOpenFileAs succeeds then we've created the file (if it didn't exist prior to calling virOpenFileAs); however, if the following fails: if ((ret = createRawFile(fd, vol, inputvol, reflink_copy)) < 0) /* createRawFile already reported the exact error. */ ret = -1; we would have left the function creating the file, but not deleting it because some subsequent action failed. John
On Wed, Oct 14, 2015 at 09:35:24 -0400, John Ferlan wrote:
On 10/14/2015 08:16 AM, Peter Krempa wrote:
On Tue, Oct 13, 2015 at 16:12:46 -0400, John Ferlan wrote:
...
Erm, you've patched virFileOpenAs a few patches before this to do the right thing when creating the file so I don't see a reason to make sure again.
I know that, look closer at the code after the call to virFileOpenAs. We know that if virOpenFileAs succeeds then we've created the file (if it didn't exist prior to calling virOpenFileAs); however, if the following fails:
if ((ret = createRawFile(fd, vol, inputvol, reflink_copy)) < 0) /* createRawFile already reported the exact error. */ ret = -1;
we would have left the function creating the file, but not deleting it because some subsequent action failed.
I agree that virStorageBackendCreateRaw needs to delete the file in some cases. I just don't think that checking if the file exists prior to the call to virOpenFileAs is necessary or makes sense in any way. Peter
On 10/14/2015 09:45 AM, Peter Krempa wrote:
On Wed, Oct 14, 2015 at 09:35:24 -0400, John Ferlan wrote:
On 10/14/2015 08:16 AM, Peter Krempa wrote:
On Tue, Oct 13, 2015 at 16:12:46 -0400, John Ferlan wrote:
...
Erm, you've patched virFileOpenAs a few patches before this to do the right thing when creating the file so I don't see a reason to make sure again.
I know that, look closer at the code after the call to virFileOpenAs. We know that if virOpenFileAs succeeds then we've created the file (if it didn't exist prior to calling virOpenFileAs); however, if the following fails:
if ((ret = createRawFile(fd, vol, inputvol, reflink_copy)) < 0) /* createRawFile already reported the exact error. */ ret = -1;
we would have left the function creating the file, but not deleting it because some subsequent action failed.
I agree that virStorageBackendCreateRaw needs to delete the file in some cases. I just don't think that checking if the file exists prior to the call to virOpenFileAs is necessary or makes sense in any way.
OK - I'm just making sure since it wasn't clear in my mind... again the 'exists' was merely paranoia in the event that the file didn't exist before virFileCreateAs and then we delete something we didn't create. I'll remove the 'exists' checking then - no problem John
After successfully returning from virFileOpenAs, if subsequent calls fail, then we need to remove the file since our caller expects that failures after creation will remove the created file. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 487c914..8dcb1dc 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -485,6 +485,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, int operation_flags; bool reflink_copy = false; mode_t open_mode = VIR_STORAGE_DEFAULT_VOL_PERM_MODE; + bool created = false; virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA | VIR_STORAGE_VOL_CREATE_REFLINK, @@ -531,6 +532,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, vol->target.path); goto cleanup; } + created = true; if (vol->target.nocow) { #ifdef __linux__ @@ -557,6 +559,10 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, ret = -1; cleanup: + if (ret < 0 && created) + ignore_value(virFileRemove(vol->target.path, + vol->target.perms->uid, + vol->target.perms->gid)); VIR_FORCE_CLOSE(fd); return ret; } -- 2.1.0
Move the code that removes the volume from the pool into it's own API Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 1c8ab87..e5e4901 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1619,6 +1619,25 @@ storagePoolLookupByTargetPath(virConnectPtr conn, } +static void +storageVolRemoveFromPool(virStoragePoolObjPtr pool, + virStorageVolDefPtr vol) +{ + size_t i; + + for (i = 0; i < pool->volumes.count; i++) { + if (pool->volumes.objs[i] == vol) { + VIR_INFO("Deleting volume '%s' from storage pool '%s'", + vol->name, pool->def->name); + virStorageVolDefFree(vol); + + VIR_DELETE_ELEMENT(pool->volumes.objs, i, pool->volumes.count); + break; + } + } +} + + static int storageVolDeleteInternal(virStorageVolPtr obj, virStorageBackendPtr backend, @@ -1627,7 +1646,6 @@ storageVolDeleteInternal(virStorageVolPtr obj, unsigned int flags, bool updateMeta) { - size_t i; int ret = -1; if (!backend->deleteVol) { @@ -1651,16 +1669,7 @@ storageVolDeleteInternal(virStorageVolPtr obj, } } - for (i = 0; i < pool->volumes.count; i++) { - if (pool->volumes.objs[i] == vol) { - VIR_INFO("Deleting volume '%s' from storage pool '%s'", - vol->name, pool->def->name); - virStorageVolDefFree(vol); - - VIR_DELETE_ELEMENT(pool->volumes.objs, i, pool->volumes.count); - break; - } - } + storageVolRemoveFromPool(pool, vol); ret = 0; cleanup: -- 2.1.0
On Fri, Oct 09, 2015 at 09:34:09 -0400, John Ferlan wrote:
Move the code that removes the volume from the pool into it's own API
It's more of a helper than an API ...
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-)
ACK, Peter
This reverts commit fdda37608a6e22406fbdfe4ac0c573a96a8d0417. This commit only manages a symptom of finding a buildRet failure where a volume was not listed in the pool, but someone created the volume outside of libvirt in the pool being managed by libvirt. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index e5e4901..58bc4bd 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1805,15 +1805,6 @@ storageVolCreateXML(virStoragePoolPtr obj, if (virStorageVolCreateXMLEnsureACL(obj->conn, pool->def, voldef) < 0) goto cleanup; - /* While not perfect, refresh the list of volumes in the pool and - * then check that the incoming name isn't already in the pool. - */ - if (backend->refreshPool) { - virStoragePoolObjClearVols(pool); - if (backend->refreshPool(obj->conn, pool) < 0) - goto cleanup; - } - if (virStorageVolDefFindByName(pool, voldef->name)) { virReportError(VIR_ERR_STORAGE_VOL_EXIST, _("'%s'"), voldef->name); -- 2.1.0
On Fri, Oct 09, 2015 at 09:34:10 -0400, John Ferlan wrote:
This reverts commit fdda37608a6e22406fbdfe4ac0c573a96a8d0417.
This commit only manages a symptom of finding a buildRet failure where a volume was not listed in the pool, but someone created the volume outside of libvirt in the pool being managed by libvirt.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 9 --------- 1 file changed, 9 deletions(-)
ACK, Peter
https://bugzilla.redhat.com/show_bug.cgi?id=1233003 Commit id 'fdda3760' only managed a symptom where it was possible to create a file in a pool without libvirt's knowledge, so it was reverted. The real fix is to have all the createVol API's which actually create a volume (disk, logical, zfs) and the buildVol API's which handle the real creation of some volume file (fs, rbd, sheepdog) manage deleting any volume which they create when there is some sort of error in processing the volume. This way the onus isn't left up to the storage_driver to determine whether the buildVol failure was due to some failure as a result of adjustments made to the volume after creation such as getting sizes, changing ownership, changing volume protections, etc. or simple a failure in creation. Without needing to consider that the volume has to be removed, the buildVol failure path only needs to remove the volume from the pool. This way if a creation failed due to duplicate name, libvirt wouldn't remove a volume that it didn't create in the pool target. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 58bc4bd..0c70482 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1869,8 +1869,17 @@ storageVolCreateXML(virStoragePoolPtr obj, pool->asyncjobs--; if (buildret < 0) { - storageVolDeleteInternal(volobj, backend, pool, voldef, - 0, false); + /* NB: A 'buildVol' backend must remove any volume created + * on error since this code does not distinguish whether the + * failure is to create the volume, reserve any space necessary + * for the volume, get data about the volume, change it's + * accessibility, etc. All that should be done at this point + * is remove the volume from the pool. This avoids issues + * arising from a creation failure due to some external action + * which created a volume of the same name that libvirt was + * not aware of between the time checked and create attempt. + */ + storageVolRemoveFromPool(pool, voldef); voldef = NULL; goto cleanup; } -- 2.1.0
On Fri, Oct 09, 2015 at 09:34:11 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1233003
Commit id 'fdda3760' only managed a symptom where it was possible to create a file in a pool without libvirt's knowledge, so it was reverted.
The real fix is to have all the createVol API's which actually create a volume (disk, logical, zfs) and the buildVol API's which handle the real creation of some volume file (fs, rbd, sheepdog) manage deleting any volume which they create when there is some sort of error in processing the volume.
This way the onus isn't left up to the storage_driver to determine whether the buildVol failure was due to some failure as a result of adjustments made to the volume after creation such as getting sizes, changing ownership, changing volume protections, etc. or simple a failure in creation.
Without needing to consider that the volume has to be removed, the buildVol failure path only needs to remove the volume from the pool. This way if a creation failed due to duplicate name, libvirt wouldn't remove a volume that it didn't create in the pool target.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 58bc4bd..0c70482 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1869,8 +1869,17 @@ storageVolCreateXML(virStoragePoolPtr obj, pool->asyncjobs--;
if (buildret < 0) { - storageVolDeleteInternal(volobj, backend, pool, voldef, - 0, false); + /* NB: A 'buildVol' backend must remove any volume created + * on error since this code does not distinguish whether the + * failure is to create the volume, reserve any space necessary + * for the volume, get data about the volume, change it's + * accessibility, etc. All that should be done at this point + * is remove the volume from the pool. This avoids issues + * arising from a creation failure due to some external action + * which created a volume of the same name that libvirt was + * not aware of between the time checked and create attempt. + */
This comment should document the required behavior at the point where we define the callback structure. Otherwise somebody might miss it as it'd be stashed in a random function.
+ storageVolRemoveFromPool(pool, voldef); voldef = NULL; goto cleanup;
ACK if you move the comment to a reasonable place. Peter
On 10/13/2015 08:39 AM, Peter Krempa wrote:
On Fri, Oct 09, 2015 at 09:34:11 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1233003
Commit id 'fdda3760' only managed a symptom where it was possible to create a file in a pool without libvirt's knowledge, so it was reverted.
The real fix is to have all the createVol API's which actually create a volume (disk, logical, zfs) and the buildVol API's which handle the real creation of some volume file (fs, rbd, sheepdog) manage deleting any volume which they create when there is some sort of error in processing the volume.
This way the onus isn't left up to the storage_driver to determine whether the buildVol failure was due to some failure as a result of adjustments made to the volume after creation such as getting sizes, changing ownership, changing volume protections, etc. or simple a failure in creation.
Without needing to consider that the volume has to be removed, the buildVol failure path only needs to remove the volume from the pool. This way if a creation failed due to duplicate name, libvirt wouldn't remove a volume that it didn't create in the pool target.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 58bc4bd..0c70482 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1869,8 +1869,17 @@ storageVolCreateXML(virStoragePoolPtr obj, pool->asyncjobs--;
if (buildret < 0) { - storageVolDeleteInternal(volobj, backend, pool, voldef, - 0, false); + /* NB: A 'buildVol' backend must remove any volume created + * on error since this code does not distinguish whether the + * failure is to create the volume, reserve any space necessary + * for the volume, get data about the volume, change it's + * accessibility, etc. All that should be done at this point + * is remove the volume from the pool. This avoids issues + * arising from a creation failure due to some external action + * which created a volume of the same name that libvirt was + * not aware of between the time checked and create attempt. + */
This comment should document the required behavior at the point where we define the callback structure. Otherwise somebody might miss it as it'd be stashed in a random function.
OK, I left: /* buildVol handles deleting volume on failure */ Since a few lines later the refreshVol calls the DeleteVol - I just wanted to make it clear that the same delete call is not necessary.
+ storageVolRemoveFromPool(pool, voldef); voldef = NULL; goto cleanup;
ACK if you move the comment to a reasonable place.
I hope src/storage/storage_backend.h is your idea of the reasonable place - I added the following text there right above the typedef: /* A 'buildVol' backend must remove any volume created on error since * the storage driver does not distinguish whether the failure is due * to failure to create the volume, to reserve any space necessary for * the volume, to get data about the volume, to change it's accessibility, * etc. This avoids issues arising from a creation failure due to some * external action which created a volume of the same name that libvirt * was not aware of between checking the pool and the create attempt. It * also avoids extra round trips to just delete a file. */ typedef int (*virStorageBackendBuildVol)(virConnectPtr conn, If you had a different location in mind, then let me know. I'll wait on comments from my responses from patch 8 & 9 pushing this. Since in my mind those should be done first. John
On Tue, Oct 13, 2015 at 16:58:53 -0400, John Ferlan wrote:
On 10/13/2015 08:39 AM, Peter Krempa wrote:
On Fri, Oct 09, 2015 at 09:34:11 -0400, John Ferlan wrote:
...
This comment should document the required behavior at the point where we define the callback structure. Otherwise somebody might miss it as it'd be stashed in a random function.
OK, I left: /* buildVol handles deleting volume on failure */
Since a few lines later the refreshVol calls the DeleteVol - I just wanted to make it clear that the same delete call is not necessary.
Ok, that makes sense.
+ storageVolRemoveFromPool(pool, voldef); voldef = NULL; goto cleanup;
ACK if you move the comment to a reasonable place.
I hope src/storage/storage_backend.h is your idea of the reasonable place - I added the following text there right above the typedef:
/* A 'buildVol' backend must remove any volume created on error since * the storage driver does not distinguish whether the failure is due * to failure to create the volume, to reserve any space necessary for * the volume, to get data about the volume, to change it's accessibility, * etc. This avoids issues arising from a creation failure due to some * external action which created a volume of the same name that libvirt * was not aware of between checking the pool and the create attempt. It * also avoids extra round trips to just delete a file. */ typedef int (*virStorageBackendBuildVol)(virConnectPtr conn,
If you had a different location in mind, then let me know.
I had in mind a few lines below where the storage driver struct is defined but this place is equally good since anyone implementing the API will have to look at the function type. Peter
On 10/09/2015 09:33 AM, John Ferlan wrote:
NOTE: Although one may consider this a v2 of :
http://www.redhat.com/archives/libvir-list/2015-October/msg00196.html
It's more tackling the same problem a different way...
Rather than pass a 'created' boolean around, this series investigated each of the 'createVol' and 'buildVol' paths in order to make more conscious decisions related to whether or not a volume was created and to handle failures after creation by deleting the volume/file.
The end result is that all the 'buildVol' backends will now delete the volume they created on error paths leaving the storage driver to only need to remove the volume from the pool.
As such this series will also revert a prior patch in this area.
John Ferlan (12): storage: Remove duplicitous refreshVol in RBD buildVol storage: Remove duplicitous refreshVol in Sheepdog buildVol storage: Fix a resource leak in storageVolCreateXML storage: Track successful creation of LV for removal storage: On error unlink created file in virFileOpen{As|Forked} storage: On error rmdir created directory in virDirCreate[NoFork] storage: Rework error paths for virStorageBackendCreateExecCommand storage: Cleanup failures virStorageBackendCreateExecCommand storage: Cleanup failures in virStorageBackendCreateRaw storage: Pull volume removal from pool in storageVolDeleteInternal Revert "storage: Prior to creating a volume, refresh the pool" storage: On 'buildVol' failure don't delete the volume
src/storage/storage_backend.c | 38 +++++++++++++++++------ src/storage/storage_backend_logical.c | 5 ++- src/storage/storage_backend_rbd.c | 3 -- src/storage/storage_backend_sheepdog.c | 5 +-- src/storage/storage_driver.c | 56 ++++++++++++++++++++-------------- src/util/virfile.c | 22 ++++++++++++- 6 files changed, 88 insertions(+), 41 deletions(-)
Pushed patches 1-7 after adjusting comments in patch 5 & 6. Waiting for feedback on 8, 9, and 12... Tks for the quick review! John
participants (2)
-
John Ferlan -
Peter Krempa