5:34 p.m.
In the review of a recent series dealing with a needing to refresh a pool
prior to adding a new volume:
http://www.redhat.com/archives/libvir-list/2015-October/msg00109.html
it was noted that there would be a small timing window possible between
the check and the actual volume creation attempt. Michal noted that we
could add a 'created' type flag in order to "know" whether the libvirt
API's created the volume or not during their buildVol API. As these
things usually do, this also morphed into also needing the same boolean
for the buildVolFrom API.
So that's what this series does... Ok mostly, patch 1 doesn't have anything
to do with this, but I saw the anomaly why working my way through the code.
John Ferlan (4):
storage: Perform some cleanup of calls
storage: Add created to virFileOpenAs
storage: Add created to virDirCreate
storage: Add checks for buildVol create file
src/libxl/libxl_domain.c | 3 ++-
src/libxl/libxl_driver.c | 3 ++-
src/qemu/qemu_driver.c | 7 +++++--
src/qemu/qemu_process.c | 6 +++---
src/storage/storage_backend.c | 28 ++++++++++++++++------------
src/storage/storage_backend.h | 3 +++
src/storage/storage_backend_disk.c | 3 ++-
src/storage/storage_backend_fs.c | 22 +++++++++++++++++-----
src/storage/storage_backend_logical.c | 3 ++-
src/storage/storage_backend_rbd.c | 2 ++
src/storage/storage_backend_sheepdog.c | 2 ++
src/storage/storage_driver.c | 16 +++++++++++-----
src/util/virfile.c | 23 ++++++++++++++++++-----
src/util/virfile.h | 4 ++--
src/util/virstoragefile.c | 3 ++-
15 files changed, 89 insertions(+), 39 deletions(-)
--
2.1.0
5:34 p.m.
New subject: [libvirt] [PATCH 1/4] storage: Perform some cleanup of calls
Cleanup calls to virStorageBackendCopyToFD a bit.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 6a3146c..ad7a576 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -345,9 +345,8 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED,
remain = vol->target.capacity;
if (inputvol) {
- int res = virStorageBackendCopyToFD(vol, inputvol,
- fd, &remain, false, reflink_copy);
- if (res < 0)
+ if (virStorageBackendCopyToFD(vol, inputvol, fd, &remain,
+ false, reflink_copy) < 0)
goto cleanup;
}
@@ -444,9 +443,8 @@ createRawFile(int fd, virStorageVolDefPtr vol,
/* allow zero blocks to be skipped if we've requested sparse
* allocation (allocation < capacity) or we have already
* been able to allocate the required space. */
- ret = virStorageBackendCopyToFD(vol, inputvol, fd, &remain,
- !need_alloc, reflink_copy);
- if (ret < 0)
+ if ((ret = virStorageBackendCopyToFD(vol, inputvol, fd, &remain,
+ !need_alloc, reflink_copy)) < 0)
goto cleanup;
/* If the new allocation is greater than the original capacity,
--
2.1.0
1:51 a.m.
New subject: [libvirt] [PATCH 1/4] storage: Perform some cleanup of calls
On Tue, Oct 06, 2015 at 18:34:53 -0400, John Ferlan wrote:
Cleanup calls to virStorageBackendCopyToFD a bit.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
ACK,
Peter
5:34 p.m.
New subject: [libvirt] [PATCH 2/4] storage: Add created to virFileOpenAs
Add a new boolean 'created' to virFileOpenAs to be set when a file
is created either directly or in a fork'd child. This will allow
a caller to make "decisions" regarding whether or not to delete the
file since virFileOpenAs has many other failure scenarios and there's
no guarantee that the O_CREAT was the cause for failure.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/libxl/libxl_domain.c | 3 ++-
src/libxl/libxl_driver.c | 3 ++-
src/qemu/qemu_driver.c | 7 +++++--
src/qemu/qemu_process.c | 6 +++---
src/storage/storage_backend.c | 2 ++
src/storage/storage_backend_fs.c | 6 ++++--
src/util/virfile.c | 14 +++++++++++---
src/util/virfile.h | 2 +-
src/util/virstoragefile.c | 3 ++-
9 files changed, 32 insertions(+), 14 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 40dcea1..34d7613 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -601,11 +601,12 @@ libxlDomainSaveImageOpen(libxlDriverPrivatePtr driver,
libxlSavefileHeaderPtr ret_hdr)
{
int fd;
+ bool created = false;
virDomainDefPtr def = NULL;
libxlSavefileHeader hdr;
char *xml = NULL;
- if ((fd = virFileOpenAs(from, O_RDONLY, 0, -1, -1, 0)) < 0) {
+ if ((fd = virFileOpenAs(from, O_RDONLY, 0, -1, -1, &created, 0)) < 0) {
virReportSystemError(-fd,
_("Failed to open domain image file
'%s'"), from);
goto error;
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index fc6dcec..c976dcb 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -1646,6 +1646,7 @@ libxlDoDomainSave(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
uint32_t xml_len;
int fd = -1;
int ret = -1;
+ bool created = false;
if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) {
virReportError(VIR_ERR_OPERATION_INVALID,
@@ -1655,7 +1656,7 @@ libxlDoDomainSave(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
}
if ((fd = virFileOpenAs(to, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR,
- -1, -1, 0)) < 0) {
+ -1, -1, &created, 0)) < 0) {
virReportSystemError(-fd,
_("Failed to create domain save file
'%s'"), to);
goto cleanup;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index aff1915..24b74e8 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2915,6 +2915,7 @@ qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid,
bool bypass_security = false;
unsigned int vfoflags = 0;
int fd = -1;
+ bool created = false;
int path_shared = virFileIsSharedFS(path);
uid_t uid = geteuid();
gid_t gid = getegid();
@@ -2953,6 +2954,7 @@ qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid,
}
} else {
if ((fd = virFileOpenAs(path, oflags, S_IRUSR | S_IWUSR, uid, gid,
+ &created,
vfoflags | VIR_FILE_OPEN_NOFORK)) < 0) {
/* If we failed as root, and the error was permission-denied
(EACCES or EPERM), assume it's on a network-connected share
@@ -2992,17 +2994,18 @@ qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid,
* don't want to delete it and allow the following to succeed
* or fail based on existing protections
*/
- if (need_unlink)
+ if (need_unlink && created)
unlink(path);
/* Retry creating the file as qemu user */
/* Since we're passing different modes... */
vfoflags |= VIR_FILE_OPEN_FORCE_MODE;
+ created = false;
if ((fd = virFileOpenAs(path, oflags,
S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP,
- fallback_uid, fallback_gid,
+ fallback_uid, fallback_gid, &created,
vfoflags | VIR_FILE_OPEN_FORK)) < 0) {
virReportSystemError(-fd, oflags & O_CREAT
? _("Error from child process creating
'%s'")
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 8cd713f..cf8a901 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4231,22 +4231,22 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
}
if ((srcFD = virFileOpenAs(master_nvram_path, O_RDONLY,
- 0, -1, -1, 0)) < 0) {
+ 0, -1, -1, &created, 0)) < 0) {
virReportSystemError(-srcFD,
_("Failed to open file '%s'"),
master_nvram_path);
goto cleanup;
}
+ created = false;
if ((dstFD = virFileOpenAs(loader->nvram,
O_WRONLY | O_CREAT | O_EXCL,
S_IRUSR | S_IWUSR,
- cfg->user, cfg->group, 0)) < 0) {
+ cfg->user, cfg->group, &created, 0)) < 0)
{
virReportSystemError(-dstFD,
_("Failed to create file '%s'"),
loader->nvram);
goto cleanup;
}
- created = true;
do {
char buf[1024];
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index ad7a576..b0535e5 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -483,6 +483,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
int ret = -1;
int fd = -1;
int operation_flags;
+ bool created = false;
bool reflink_copy = false;
mode_t open_mode = VIR_STORAGE_DEFAULT_VOL_PERM_MODE;
@@ -525,6 +526,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
open_mode,
vol->target.perms->uid,
vol->target.perms->gid,
+ &created,
operation_flags)) < 0) {
virReportSystemError(-fd,
_("Failed to create file '%s'"),
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 99ea394..b39b5a5 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -1451,13 +1451,14 @@ static int
virStorageFileBackendFileCreate(virStorageSourcePtr src)
{
int fd = -1;
+ bool created = false;
mode_t mode = S_IRUSR;
if (!src->readonly)
mode |= S_IWUSR;
if ((fd = virFileOpenAs(src->path, O_WRONLY | O_TRUNC | O_CREAT, mode,
- src->drv->uid, src->drv->gid, 0)) < 0) {
+ src->drv->uid, src->drv->gid, &created, 0))
< 0) {
errno = -fd;
return -1;
}
@@ -1488,10 +1489,11 @@ virStorageFileBackendFileReadHeader(virStorageSourcePtr src,
char **buf)
{
int fd = -1;
+ bool created = false;
ssize_t ret = -1;
if ((fd = virFileOpenAs(src->path, O_RDONLY, 0,
- src->drv->uid, src->drv->gid, 0)) < 0) {
+ src->drv->uid, src->drv->gid, &created, 0))
< 0) {
virReportSystemError(-fd, _("Failed to open file '%s'"),
src->path);
return -1;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index a81f04c..a783406 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -2060,7 +2060,7 @@ virFileOpenForceOwnerMode(const char *path, int fd, mode_t mode,
* fd, or -errno if there is an error. */
static int
virFileOpenForked(const char *path, int openflags, mode_t mode,
- uid_t uid, gid_t gid, unsigned int flags)
+ uid_t uid, gid_t gid, bool *created, unsigned int flags)
{
pid_t pid;
int status = 0, ret = 0;
@@ -2114,6 +2114,9 @@ virFileOpenForked(const char *path, int openflags, mode_t mode,
goto childerror;
}
+ if (openflags & O_CREAT)
+ *created = true;
+
/* File is successfully open. Set permissions if requested. */
ret = virFileOpenForceOwnerMode(path, fd, mode, uid, gid, flags);
if (ret < 0) {
@@ -2199,6 +2202,7 @@ virFileOpenForked(const char *path, int openflags, mode_t mode,
* @mode: mode to use on creation or when forcing permissions
* @uid: uid that should own file on creation
* @gid: gid that should own file
+ * @created: set to true if O_CREAT and we succeed open
* @flags: bit-wise or of VIR_FILE_OPEN_* flags
*
* Open @path, and return an fd to the open file. @openflags contains
@@ -2222,7 +2226,7 @@ virFileOpenForked(const char *path, int openflags, mode_t mode,
*/
int
virFileOpenAs(const char *path, int openflags, mode_t mode,
- uid_t uid, gid_t gid, unsigned int flags)
+ uid_t uid, gid_t gid, bool *created, unsigned int flags)
{
int ret = 0, fd = -1;
@@ -2246,6 +2250,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;
@@ -2275,7 +2281,8 @@ virFileOpenAs(const char *path, int openflags, mode_t mode,
}
/* passed all prerequisites - retry the open w/fork+setuid */
- if ((fd = virFileOpenForked(path, openflags, mode, uid, gid, flags)) < 0) {
+ if ((fd = virFileOpenForked(path, openflags, mode, uid, gid,
+ created, flags)) < 0) {
ret = fd;
goto error;
}
@@ -2595,6 +2602,7 @@ virFileOpenAs(const char *path ATTRIBUTE_UNUSED,
mode_t mode ATTRIBUTE_UNUSED,
uid_t uid ATTRIBUTE_UNUSED,
gid_t gid ATTRIBUTE_UNUSED,
+ bool *created ATTRIBUTE_UNUSED,
unsigned int flags_unused ATTRIBUTE_UNUSED)
{
virReportError(VIR_ERR_INTERNAL_ERROR,
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 312f226..f6e7566 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -216,7 +216,7 @@ int virFileAccessibleAs(const char *path, int mode,
uid_t uid, gid_t gid)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
int virFileOpenAs(const char *path, int openflags, mode_t mode,
- uid_t uid, gid_t gid,
+ uid_t uid, gid_t gid, bool *created,
unsigned int flags)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
int virFileRemove(const char *path, uid_t uid, gid_t gid);
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 2aa1d90..a9a4413 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -866,12 +866,13 @@ int
virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid)
{
int fd;
+ bool created = false;
int ret = -1;
struct stat sb;
ssize_t len = VIR_STORAGE_MAX_HEADER;
char *header = NULL;
- if ((fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, 0)) < 0) {
+ if ((fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, &created, 0)) < 0) {
virReportSystemError(-fd, _("Failed to open file '%s'"),
path);
return -1;
}
--
2.1.0
1:50 a.m.
New subject: [libvirt] [PATCH 2/4] storage: Add created to virFileOpenAs
On Tue, Oct 06, 2015 at 18:34:54 -0400, John Ferlan wrote:
Add a new boolean 'created' to virFileOpenAs to be set when a
file
is created either directly or in a fork'd child. This will allow
a caller to make "decisions" regarding whether or not to delete the
file since virFileOpenAs has many other failure scenarios and there's
no guarantee that the O_CREAT was the cause for failure.
I don't like this approach. See 3/4 for explanation.
Peter
5:34 p.m.
New subject: [libvirt] [PATCH 3/4] storage: Add created to virDirCreate
Add a new boolean 'created' to virDirCreate to be set when a directory
is created either directly or in a fork'd child. This will allow
a caller to make "decisions" regarding whether or not to delete the
directory since virDirCreate has many other failure scenarios and there's
no guarantee that the mkdir was the cause for failure.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_fs.c | 4 ++++
src/util/virfile.c | 9 +++++++--
src/util/virfile.h | 2 +-
3 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index b39b5a5..fddec4b 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -786,6 +786,7 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED,
mode_t mode;
bool needs_create_as_uid;
unsigned int dir_create_flags;
+ bool created = false;
virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE |
VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret);
@@ -831,6 +832,7 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED,
mode,
pool->def->target.perms.uid,
pool->def->target.perms.gid,
+ &created,
dir_create_flags) < 0)
goto error;
@@ -1081,6 +1083,7 @@ static int createFileDir(virConnectPtr conn ATTRIBUTE_UNUSED,
unsigned int flags)
{
int err;
+ bool created = false;
virCheckFlags(0, -1);
@@ -1104,6 +1107,7 @@ static int createFileDir(virConnectPtr conn ATTRIBUTE_UNUSED,
vol->target.perms->mode),
vol->target.perms->uid,
vol->target.perms->gid,
+ &created,
(pool->def->type == VIR_STORAGE_POOL_NETFS
? VIR_DIR_CREATE_AS_UID : 0))) < 0) {
return -1;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index a783406..0b70d41 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -2412,6 +2412,7 @@ virFileRemove(const char *path,
static int
virDirCreateNoFork(const char *path,
mode_t mode, uid_t uid, gid_t gid,
+ bool *created,
unsigned int flags)
{
int ret = 0;
@@ -2424,6 +2425,7 @@ virDirCreateNoFork(const char *path,
path);
goto error;
}
+ *created = true;
}
if (stat(path, &st) == -1) {
@@ -2454,6 +2456,7 @@ virDirCreateNoFork(const char *path,
int
virDirCreate(const char *path,
mode_t mode, uid_t uid, gid_t gid,
+ bool *created,
unsigned int flags)
{
struct stat st;
@@ -2480,7 +2483,7 @@ virDirCreate(const char *path,
|| (geteuid() != 0)
|| ((uid == (uid_t) -1) && (gid == (gid_t) -1))
|| ((flags & VIR_DIR_CREATE_ALLOW_EXIST) && virFileExists(path))) {
- return virDirCreateNoFork(path, mode, uid, gid, flags);
+ return virDirCreateNoFork(path, mode, uid, gid, created, flags);
}
if (uid == (uid_t) -1)
@@ -2522,7 +2525,7 @@ virDirCreate(const char *path,
*/
if (status == EACCES) {
virResetLastError();
- return virDirCreateNoFork(path, mode, uid, gid, flags);
+ return virDirCreateNoFork(path, mode, uid, gid, created, flags);
}
if (status)
@@ -2548,6 +2551,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 */
@@ -2616,6 +2620,7 @@ virDirCreate(const char *path ATTRIBUTE_UNUSED,
mode_t mode ATTRIBUTE_UNUSED,
uid_t uid ATTRIBUTE_UNUSED,
gid_t gid ATTRIBUTE_UNUSED,
+ bool *created ATTRIBUTE_UNUSED,
unsigned int flags_unused ATTRIBUTE_UNUSED)
{
virReportError(VIR_ERR_INTERNAL_ERROR,
diff --git a/src/util/virfile.h b/src/util/virfile.h
index f6e7566..855134f 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -227,7 +227,7 @@ enum {
VIR_DIR_CREATE_ALLOW_EXIST = (1 << 1),
};
int virDirCreate(const char *path, mode_t mode, uid_t uid, gid_t gid,
- unsigned int flags) ATTRIBUTE_RETURN_CHECK;
+ bool *created, unsigned int flags) ATTRIBUTE_RETURN_CHECK;
int virDirRead(DIR *dirp, struct dirent **ent, const char *dirname)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
--
2.1.0
1:50 a.m.
New subject: [libvirt] [PATCH 3/4] storage: Add created to virDirCreate
On Tue, Oct 06, 2015 at 18:34:55 -0400, John Ferlan wrote:
Add a new boolean 'created' to virDirCreate to be set when a
directory
is created either directly or in a fork'd child. This will allow
a caller to make "decisions" regarding whether or not to delete the
directory since virDirCreate has many other failure scenarios and there's
no guarantee that the mkdir was the cause for failure.
Is there actually a need to pass the 'created' bool out? Can't the
function just delete the directory internally in case a failure happens?
The new semantics basically would be:
virDirCreate was successful -> we have a new dir we can use
virDirCreate failed -> nothing to do/clean up
Peter
6:44 p.m.
New subject: [libvirt] [PATCH 3/4] storage: Add created to virDirCreate
On 10/07/2015 02:50 AM, Peter Krempa wrote:
On Tue, Oct 06, 2015 at 18:34:55 -0400, John Ferlan wrote:
> Add a new boolean 'created' to virDirCreate to be set when a directory
> is created either directly or in a fork'd child. This will allow
> a caller to make "decisions" regarding whether or not to delete the
> directory since virDirCreate has many other failure scenarios and there's
> no guarantee that the mkdir was the cause for failure.
Is there actually a need to pass the 'created' bool out? Can't the
function just delete the directory internally in case a failure happens?
The new semantics basically would be:
virDirCreate was successful -> we have a new dir we can use
virDirCreate failed -> nothing to do/clean up
Perhaps there's no "need" to pass a create boolean - I was merely
following up on Michal's suggestion. I guess I assumed it was a
suggestion discussed among those in the Brno office.
Performing the cleanup in the create functions should be workable. Each
of the functions would need to have code added the case where the
volume/file already exists and adjust the currently common cleanup code
to make decisions based on whether the create was done. I'll cobble
together some patches for that.
John
10:43 p.m.
New subject: [libvirt] [PATCH 3/4] storage: Add created to virDirCreate
On Wed, Oct 07, 2015 at 19:44:21 -0400, John Ferlan wrote:
On 10/07/2015 02:50 AM, Peter Krempa wrote:
> On Tue, Oct 06, 2015 at 18:34:55 -0400, John Ferlan wrote:
>> Add a new boolean 'created' to virDirCreate to be set when a directory
>> is created either directly or in a fork'd child. This will allow
>> a caller to make "decisions" regarding whether or not to delete the
>> directory since virDirCreate has many other failure scenarios and there's
>> no guarantee that the mkdir was the cause for failure.
>
> Is there actually a need to pass the 'created' bool out? Can't the
> function just delete the directory internally in case a failure happens?
>
> The new semantics basically would be:
>
> virDirCreate was successful -> we have a new dir we can use
> virDirCreate failed -> nothing to do/clean up
>
Perhaps there's no "need" to pass a create boolean - I was merely
following up on Michal's suggestion. I guess I assumed it was a
suggestion discussed among those in the Brno office.
I've read that thread now. Michal's suggestion may still be necessary at
the storage driver API level as at that point the stuff seems to be too
convoluted.
I'm unhappy though that the 'created' boolean crept into functions,
whose only purpose is to actually create a directory/file.
The "we've returned failure, but the object actually was created by us"
semantics seems rather weird at that point.
I'd still prefer if the function would clean up internally if possible
though.
Peter
5:34 p.m.
New subject: [libvirt] [PATCH 4/4] storage: Add checks for buildVol create file
The buildVol function can fail in numerous ways, but for cleaner or
clearer error path handling we want to know whether the calls we made
actually created the volume prior to blindly deleting the volume.
It could very well be that in between refreshing the pool, checking
whether the volume was already in the pool, and trying to create the
volume that something 'external' created a volume of the same name.
In this case, failure is likely and since we didn't create the volume
we shouldn't delete it either.
This patch may set the 'created' boolean for the following functions:
virStorageBackendCreateQemuImg (in virStorageBackendCreateExecCommand)
virStorageBackendCreateQcowCreate (in virStorageBackendCreateExecCommand)
virStorageBackendCreateFileDir (in virDirCreate)
virStorageBackendCreateRaw (in virFileOpenAs)
virStorageBackendRBDBuildVol
virStorageBackendSheepdogBuildVol
The patch avoids setting 'created' for the following function since
the 'target' of the buildVolFrom would be some sort of block device:
virStorageBackendCreateBlockFrom
NB: The fs, disk, and logical backends use the same common API
virStorageBackendGetBuildVolFromFunction in order to handle the
volBuildFrom calls to one of the virStorageBackendCreate* APIs
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend.c | 20 ++++++++++++--------
src/storage/storage_backend.h | 3 +++
src/storage/storage_backend_disk.c | 3 ++-
src/storage/storage_backend_fs.c | 16 +++++++++++-----
src/storage/storage_backend_logical.c | 3 ++-
src/storage/storage_backend_rbd.c | 2 ++
src/storage/storage_backend_sheepdog.c | 2 ++
src/storage/storage_driver.c | 16 +++++++++++-----
8 files changed, 45 insertions(+), 20 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index b0535e5..112bb80 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -310,6 +310,7 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED,
virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
virStorageVolDefPtr vol,
virStorageVolDefPtr inputvol,
+ bool *created ATTRIBUTE_UNUSED,
unsigned int flags)
{
int fd = -1;
@@ -478,12 +479,12 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
virStorageVolDefPtr inputvol,
+ bool *created,
unsigned int flags)
{
int ret = -1;
int fd = -1;
int operation_flags;
- bool created = false;
bool reflink_copy = false;
mode_t open_mode = VIR_STORAGE_DEFAULT_VOL_PERM_MODE;
@@ -526,7 +527,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
open_mode,
vol->target.perms->uid,
vol->target.perms->gid,
- &created,
+ created,
operation_flags)) < 0) {
virReportSystemError(-fd,
_("Failed to create file '%s'"),
@@ -674,13 +675,13 @@ virStorageGenerateQcowEncryption(virConnectPtr conn,
static int
virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
- virCommandPtr cmd)
+ virCommandPtr cmd,
+ bool *created)
{
struct stat st;
gid_t gid;
uid_t uid;
mode_t mode;
- bool filecreated = false;
if ((pool->def->type == VIR_STORAGE_POOL_NETFS)
&& (((geteuid() == 0)
@@ -695,7 +696,7 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool,
if (virCommandRun(cmd, NULL) == 0) {
/* command was successfully run, check if the file was created */
if (stat(vol->target.path, &st) >= 0)
- filecreated = true;
+ *created = true;
}
}
@@ -703,7 +704,7 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool,
virCommandSetUID(cmd, -1);
virCommandSetGID(cmd, -1);
- if (!filecreated) {
+ if (!*created) {
if (virCommandRun(cmd, NULL) < 0)
return -1;
if (stat(vol->target.path, &st) < 0) {
@@ -711,6 +712,7 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool,
_("failed to create %s"),
vol->target.path);
return -1;
}
+ *created = true;
}
uid = (vol->target.perms->uid != st.st_uid) ? vol->target.perms->uid
@@ -1088,6 +1090,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn,
virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
virStorageVolDefPtr inputvol,
+ bool *created,
unsigned int flags)
{
int ret = -1;
@@ -1117,7 +1120,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn,
if (!cmd)
goto cleanup;
- ret = virStorageBackendCreateExecCommand(pool, vol, cmd);
+ ret = virStorageBackendCreateExecCommand(pool, vol, cmd, created);
virCommandFree(cmd);
cleanup:
@@ -1134,6 +1137,7 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn
ATTRIBUTE_UNUSED,
virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
virStorageVolDefPtr inputvol,
+ bool *created,
unsigned int flags)
{
int ret;
@@ -1181,7 +1185,7 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn
ATTRIBUTE_UNUSED,
cmd = virCommandNewArgList("qcow-create", size, vol->target.path,
NULL);
- ret = virStorageBackendCreateExecCommand(pool, vol, cmd);
+ ret = virStorageBackendCreateExecCommand(pool, vol, cmd, created);
virCommandFree(cmd);
VIR_FREE(size);
diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
index 39c00b1..d574adb 100644
--- a/src/storage/storage_backend.h
+++ b/src/storage/storage_backend.h
@@ -51,6 +51,7 @@ typedef int (*virStorageBackendDeletePool)(virConnectPtr conn,
typedef int (*virStorageBackendBuildVol)(virConnectPtr conn,
virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
+ bool *created,
unsigned int flags);
typedef int (*virStorageBackendCreateVol)(virConnectPtr conn,
virStoragePoolObjPtr pool,
@@ -66,6 +67,7 @@ typedef int (*virStorageBackendBuildVolFrom)(virConnectPtr conn,
virStoragePoolObjPtr pool,
virStorageVolDefPtr origvol,
virStorageVolDefPtr newvol,
+ bool *created,
unsigned int flags);
typedef int (*virStorageBackendVolumeResize)(virConnectPtr conn,
virStoragePoolObjPtr pool,
@@ -97,6 +99,7 @@ int virStorageBackendCreateRaw(virConnectPtr conn,
virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
virStorageVolDefPtr inputvol,
+ bool *created,
unsigned int flags);
virStorageBackendBuildVolFrom
virStorageBackendGetBuildVolFromFunction(virStorageVolDefPtr vol,
diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c
index 7baecc1..b696c07 100644
--- a/src/storage/storage_backend_disk.c
+++ b/src/storage/storage_backend_disk.c
@@ -945,6 +945,7 @@ virStorageBackendDiskBuildVolFrom(virConnectPtr conn,
virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
virStorageVolDefPtr inputvol,
+ bool *created,
unsigned int flags)
{
virStorageBackendBuildVolFrom build_func;
@@ -953,7 +954,7 @@ virStorageBackendDiskBuildVolFrom(virConnectPtr conn,
if (!build_func)
return -1;
- return build_func(conn, pool, vol, inputvol, flags);
+ return build_func(conn, pool, vol, inputvol, created, flags);
}
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index fddec4b..a9e9e5d 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -1080,10 +1080,10 @@ static int createFileDir(virConnectPtr conn ATTRIBUTE_UNUSED,
virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
virStorageVolDefPtr inputvol,
+ bool *created,
unsigned int flags)
{
int err;
- bool created = false;
virCheckFlags(0, -1);
@@ -1107,11 +1107,12 @@ static int createFileDir(virConnectPtr conn ATTRIBUTE_UNUSED,
vol->target.perms->mode),
vol->target.perms->uid,
vol->target.perms->gid,
- &created,
+ created,
(pool->def->type == VIR_STORAGE_POOL_NETFS
? VIR_DIR_CREATE_AS_UID : 0))) < 0) {
return -1;
}
+ *created = true;
return 0;
}
@@ -1121,6 +1122,7 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn,
virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
virStorageVolDefPtr inputvol,
+ bool *created,
unsigned int flags)
{
virStorageBackendBuildVolFrom create_func;
@@ -1154,7 +1156,7 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn,
return -1;
}
- if (create_func(conn, pool, vol, inputvol, flags) < 0)
+ if (create_func(conn, pool, vol, inputvol, created, flags) < 0)
return -1;
return 0;
}
@@ -1168,13 +1170,15 @@ static int
virStorageBackendFileSystemVolBuild(virConnectPtr conn,
virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
+ bool *created,
unsigned int flags)
{
virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA |
VIR_STORAGE_VOL_CREATE_REFLINK,
-1);
- return _virStorageBackendFileSystemVolBuild(conn, pool, vol, NULL, flags);
+ return _virStorageBackendFileSystemVolBuild(conn, pool, vol, NULL,
+ created, flags);
}
/*
@@ -1185,13 +1189,15 @@ virStorageBackendFileSystemVolBuildFrom(virConnectPtr conn,
virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
virStorageVolDefPtr inputvol,
+ bool *created,
unsigned int flags)
{
virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA |
VIR_STORAGE_VOL_CREATE_REFLINK,
-1);
- return _virStorageBackendFileSystemVolBuild(conn, pool, vol, inputvol, flags);
+ return _virStorageBackendFileSystemVolBuild(conn, pool, vol, inputvol,
+ created, flags);
}
/**
diff --git a/src/storage/storage_backend_logical.c
b/src/storage/storage_backend_logical.c
index 070f2bd..3702140 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -828,6 +828,7 @@ virStorageBackendLogicalBuildVolFrom(virConnectPtr conn,
virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
virStorageVolDefPtr inputvol,
+ bool *created,
unsigned int flags)
{
virStorageBackendBuildVolFrom build_func;
@@ -836,7 +837,7 @@ virStorageBackendLogicalBuildVolFrom(virConnectPtr conn,
if (!build_func)
return -1;
- return build_func(conn, pool, vol, inputvol, flags);
+ return build_func(conn, pool, vol, inputvol, created, flags);
}
static int
diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c
index ac5085a..90d8596 100644
--- a/src/storage/storage_backend_rbd.c
+++ b/src/storage/storage_backend_rbd.c
@@ -491,6 +491,7 @@ static int
virStorageBackendRBDBuildVol(virConnectPtr conn,
virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
+ bool *created,
unsigned int flags)
{
virStorageBackendRBDState ptr;
@@ -531,6 +532,7 @@ virStorageBackendRBDBuildVol(virConnectPtr conn,
vol->name);
goto cleanup;
}
+ *created = true;
if (volStorageBackendRBDRefreshVolInfo(vol, pool, &ptr) < 0)
goto cleanup;
diff --git a/src/storage/storage_backend_sheepdog.c
b/src/storage/storage_backend_sheepdog.c
index 69ba7836..3104578 100644
--- a/src/storage/storage_backend_sheepdog.c
+++ b/src/storage/storage_backend_sheepdog.c
@@ -260,6 +260,7 @@ static int
virStorageBackendSheepdogBuildVol(virConnectPtr conn,
virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
+ bool *created,
unsigned int flags)
{
int ret = -1;
@@ -278,6 +279,7 @@ virStorageBackendSheepdogBuildVol(virConnectPtr conn,
virStorageBackendSheepdogAddHostArg(cmd, pool);
if (virCommandRun(cmd, NULL) < 0)
goto cleanup;
+ *created = true;
if (virStorageBackendSheepdogRefreshVol(conn, pool, vol) < 0)
goto cleanup;
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index ddf4405..66e5be2 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1766,6 +1766,7 @@ storageVolCreateXML(virStoragePoolPtr obj,
virStorageBackendPtr backend;
virStorageVolDefPtr voldef = NULL;
virStorageVolPtr ret = NULL, volobj = NULL;
+ bool created = false;
virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);
@@ -1857,7 +1858,8 @@ storageVolCreateXML(virStoragePoolPtr obj,
voldef->building = true;
virStoragePoolObjUnlock(pool);
- buildret = backend->buildVol(obj->conn, pool, buildvoldef, flags);
+ buildret = backend->buildVol(obj->conn, pool, buildvoldef,
+ &created, flags);
storageDriverLock();
virStoragePoolObjLock(pool);
@@ -1868,8 +1870,9 @@ storageVolCreateXML(virStoragePoolPtr obj,
if (buildret < 0) {
VIR_FREE(buildvoldef);
- storageVolDeleteInternal(volobj, backend, pool, voldef,
- 0, false);
+ if (created)
+ storageVolDeleteInternal(volobj, backend, pool, voldef,
+ 0, false);
voldef = NULL;
goto cleanup;
}
@@ -1917,6 +1920,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
virStorageVolDefPtr origvol = NULL, newvol = NULL, shadowvol = NULL;
virStorageVolPtr ret = NULL, volobj = NULL;
int buildret;
+ bool created = false;
virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA |
VIR_STORAGE_VOL_CREATE_REFLINK,
@@ -2048,7 +2052,8 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
virStoragePoolObjUnlock(origpool);
}
- buildret = backend->buildVolFrom(obj->conn, pool, shadowvol, origvol, flags);
+ buildret = backend->buildVolFrom(obj->conn, pool, shadowvol, origvol,
+ &created, flags);
storageDriverLock();
virStoragePoolObjLock(pool);
@@ -2069,7 +2074,8 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
if (buildret < 0 ||
(backend->refreshVol &&
backend->refreshVol(obj->conn, pool, newvol) < 0)) {
- storageVolDeleteInternal(volobj, backend, pool, newvol, 0, false);
+ if (created)
+ storageVolDeleteInternal(volobj, backend, pool, newvol, 0, false);
newvol = NULL;
goto cleanup;
}
--
2.1.0
1:57 a.m.
New subject: [libvirt] [PATCH 4/4] storage: Add checks for buildVol create file
On Tue, Oct 06, 2015 at 18:34:56 -0400, John Ferlan wrote:
The buildVol function can fail in numerous ways, but for cleaner or
clearer error path handling we want to know whether the calls we made
actually created the volume prior to blindly deleting the volume.
It could very well be that in between refreshing the pool, checking
whether the volume was already in the pool, and trying to create the
volume that something 'external' created a volume of the same name.
In this case, failure is likely and since we didn't create the volume
we shouldn't delete it either.
This patch may set the 'created' boolean for the following functions:
virStorageBackendCreateQemuImg (in virStorageBackendCreateExecCommand)
virStorageBackendCreateQcowCreate (in virStorageBackendCreateExecCommand)
virStorageBackendCreateFileDir (in virDirCreate)
virStorageBackendCreateRaw (in virFileOpenAs)
virStorageBackendRBDBuildVol
virStorageBackendSheepdogBuildVol
Well, as in previous functions. Is there a reason that would inhibit the
actual functions that create the volume to clean it up afterwards in the
same function so that passing up the state would not be necessary?
Peter
3319
days inactive
3321
days old
10 comments
2 participants
participants (2)
-
John Ferlan -
Peter Krempa