8:33 a.m.
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
8:34 a.m.
New subject: [libvirt] [PATCH 01/12] storage: Remove duplicitous refreshVol in RBD buildVol
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(a)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
7:54 a.m.
New subject: [libvirt] [PATCH 01/12] storage: Remove duplicitous refreshVol in RBD buildVol
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(a)redhat.com>
---
src/storage/storage_backend_rbd.c | 3 ---
1 file changed, 3 deletions(-)
ACK,
Peter
8:34 a.m.
New subject: [libvirt] [PATCH 02/12] storage: Remove duplicitous refreshVol in Sheepdog buildVol
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(a)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
7:54 a.m.
New subject: [libvirt] [PATCH 02/12] storage: Remove duplicitous refreshVol in Sheepdog buildVol
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(a)redhat.com>
---
src/storage/storage_backend_sheepdog.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
ACK,
Peter
8:34 a.m.
New subject: [libvirt] [PATCH 03/12] storage: Fix a resource leak in storageVolCreateXML
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(a)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
7:30 a.m.
New subject: [libvirt] [PATCH 03/12] storage: Fix a resource leak in storageVolCreateXML
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(a)redhat.com>
---
src/storage/storage_driver.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
ACK,
Peter
8:34 a.m.
New subject: [libvirt] [PATCH 04/12] storage: Track successful creation of LV for removal
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(a)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
7:32 a.m.
New subject: [libvirt] [PATCH 04/12] storage: Track successful creation of LV for removal
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(a)redhat.com>
---
src/storage/storage_backend_logical.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Definitely better than the previous version.
ACK,
Peter
8:34 a.m.
New subject: [libvirt] [PATCH 05/12] storage: On error unlink created file in virFileOpen{As|Forked}
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(a)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
7:27 a.m.
New subject: [libvirt] [PATCH 05/12] storage: On error unlink created file in virFileOpen{As|Forked}
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(a)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
8:34 a.m.
New subject: [libvirt] [PATCH 06/12] storage: On error rmdir created directory in virDirCreate[NoFork]
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(a)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
7:53 a.m.
New subject: [libvirt] [PATCH 06/12] storage: On error rmdir created directory in virDirCreate[NoFork]
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(a)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
8:34 a.m.
New subject: [libvirt] [PATCH 07/12] storage: Rework error paths for virStorageBackendCreateExecCommand
Rework the code in order to use the "ret = -1;" and goto cleanup;
coding style.
Signed-off-by: John Ferlan <jferlan(a)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
7:46 a.m.
New subject: [libvirt] [PATCH 07/12] storage: Rework error paths for virStorageBackendCreateExecCommand
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(a)redhat.com>
---
src/storage/storage_backend.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
ACK,
Peter
8:34 a.m.
New subject: [libvirt] [PATCH 08/12] storage: Cleanup failures virStorageBackendCreateExecCommand
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(a)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
7:50 a.m.
New subject: [libvirt] [PATCH 08/12] storage: Cleanup failures virStorageBackendCreateExecCommand
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(a)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
2:11 p.m.
New subject: [libvirt] [PATCH 08/12] storage: Cleanup failures virStorageBackendCreateExecCommand
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(a)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
4:43 p.m.
New subject: [libvirt] [PATCH 08/12] storage: Cleanup failures virStorageBackendCreateExecCommand
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
7:21 a.m.
New subject: [libvirt] [PATCH 08/12] storage: Cleanup failures virStorageBackendCreateExecCommand
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
8:34 a.m.
New subject: [libvirt] [PATCH 08/12] storage: Cleanup failures virStorageBackendCreateExecCommand
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
1:01 p.m.
New subject: [libvirt] [PATCH v2 7/12] storage: Refactor and fix virStorageBackendCreateExecCommand
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(a)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
1:10 p.m.
New subject: [libvirt] [PATCH v2 7/12] storage: Refactor and fix virStorageBackendCreateExecCommand
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(a)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
4:35 p.m.
New subject: [libvirt] [PATCH v2 7/12] storage: Refactor and fix virStorageBackendCreateExecCommand
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(a)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
1:03 p.m.
New subject: [libvirt] [PATCH v2 8.5/12] storage: Cleanup failures virStorageBackendCreateExecCommand
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(a)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
8:34 a.m.
New subject: [libvirt] [PATCH 09/12] storage: Cleanup failures in virStorageBackendCreateRaw
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(a)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
7:43 a.m.
New subject: [libvirt] [PATCH 09/12] storage: Cleanup failures in virStorageBackendCreateRaw
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(a)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
3:12 p.m.
New subject: [libvirt] [PATCH 09/12] storage: Cleanup failures in virStorageBackendCreateRaw
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(a)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
7:16 a.m.
New subject: [libvirt] [PATCH 09/12] storage: Cleanup failures in virStorageBackendCreateRaw
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
8:35 a.m.
New subject: [libvirt] [PATCH 09/12] storage: Cleanup failures in virStorageBackendCreateRaw
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
8:45 a.m.
New subject: [libvirt] [PATCH 09/12] storage: Cleanup failures in virStorageBackendCreateRaw
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
8:47 a.m.
New subject: [libvirt] [PATCH 09/12] storage: Cleanup failures in virStorageBackendCreateRaw
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
1:12 p.m.
New subject: [libvirt] [PATCH v2 9/12] storage: Cleanup failures in virStorageBackendCreateRaw
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(a)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
8:34 a.m.
New subject: [libvirt] [PATCH 10/12] storage: Pull volume removal from pool in storageVolDeleteInternal
Move the code that removes the volume from the pool into it's own API
Signed-off-by: John Ferlan <jferlan(a)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
7:40 a.m.
New subject: [libvirt] [PATCH 10/12] storage: Pull volume removal from pool in storageVolDeleteInternal
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(a)redhat.com>
---
src/storage/storage_driver.c | 31 ++++++++++++++++++++-----------
1 file changed, 20 insertions(+), 11 deletions(-)
ACK,
Peter
8:34 a.m.
New subject: [libvirt] [PATCH 11/12] Revert "storage: Prior to creating a volume, refresh the pool"
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(a)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
7:39 a.m.
New subject: [libvirt] [PATCH 11/12] Revert "storage: Prior to creating a volume, refresh the pool"
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(a)redhat.com>
---
src/storage/storage_driver.c | 9 ---------
1 file changed, 9 deletions(-)
ACK,
Peter
8:34 a.m.
New subject: [libvirt] [PATCH 12/12] storage: On 'buildVol' failure don't delete the volume
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(a)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
7:39 a.m.
New subject: [libvirt] [PATCH 12/12] storage: On 'buildVol' failure don't delete the volume
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(a)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
3:58 p.m.
New subject: [libvirt] [PATCH 12/12] storage: On 'buildVol' failure don't delete the volume
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(a)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
7:24 a.m.
New subject: [libvirt] [PATCH 12/12] storage: On 'buildVol' failure don't delete the volume
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
5:34 p.m.
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
3321
days inactive
3332
days old
41 comments
2 participants
participants (2)
-
John Ferlan -
Peter Krempa