[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&gt; --- 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