Let's make use of the auto __cleanup capabilities cleaning up any
now unnecessary goto paths.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
---
src/storage/storage_backend_disk.c | 85 +++++++++------------
src/storage/storage_backend_fs.c | 39 +++-------
src/storage/storage_backend_logical.c | 101 +++++++------------------
src/storage/storage_backend_sheepdog.c | 59 ++++++---------
src/storage/storage_backend_vstorage.c | 14 +---
src/storage/storage_backend_zfs.c | 47 +++---------
src/storage/storage_driver.c | 3 +-
src/storage/storage_util.c | 34 +++------
src/util/virstoragefile.c | 67 +++++++---------
tests/storagepoolxml2argvtest.c | 7 +-
tests/storagevolxml2argvtest.c | 6 +-
tests/virstoragetest.c | 6 +-
12 files changed, 156 insertions(+), 312 deletions(-)
diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c
index 061c494b7d..4fb38178b2 100644
--- a/src/storage/storage_backend_disk.c
+++ b/src/storage/storage_backend_disk.c
@@ -356,12 +356,12 @@ virStorageBackendDiskReadPartitions(virStoragePoolObjPtr pool,
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
char *parthelper_path;
- virCommandPtr cmd;
struct virStorageBackendDiskPoolVolData cbdata = {
.pool = pool,
.vol = vol,
};
int ret;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
if (!(parthelper_path = virFileFindResource("libvirt_parthelper",
abs_topbuilddir "/src",
@@ -392,7 +392,6 @@ virStorageBackendDiskReadPartitions(virStoragePoolObjPtr pool,
6,
virStorageBackendDiskMakeVol,
&cbdata);
- virCommandFree(cmd);
VIR_FREE(parthelper_path);
return ret;
}
@@ -421,8 +420,8 @@ virStorageBackendDiskReadGeometry(virStoragePoolObjPtr pool)
{
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
char *parthelper_path;
- virCommandPtr cmd;
int ret;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
if (!(parthelper_path = virFileFindResource("libvirt_parthelper",
abs_topbuilddir "/src",
@@ -438,7 +437,6 @@ virStorageBackendDiskReadGeometry(virStoragePoolObjPtr pool)
3,
virStorageBackendDiskMakePoolGeometry,
pool);
- virCommandFree(cmd);
VIR_FREE(parthelper_path);
return ret;
}
@@ -502,51 +500,40 @@ virStorageBackendDiskBuildPool(virStoragePoolObjPtr pool,
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
int format = def->source.format;
const char *fmt;
- bool ok_to_mklabel = false;
- int ret = -1;
- virCommandPtr cmd = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE |
- VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret);
+ VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, -1);
- VIR_EXCLUSIVE_FLAGS_GOTO(VIR_STORAGE_POOL_BUILD_OVERWRITE,
- VIR_STORAGE_POOL_BUILD_NO_OVERWRITE,
- error);
+ VIR_EXCLUSIVE_FLAGS_RET(VIR_STORAGE_POOL_BUILD_OVERWRITE,
+ VIR_STORAGE_POOL_BUILD_NO_OVERWRITE,
+ -1);
fmt = virStoragePoolFormatDiskTypeToString(format);
- if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) {
- ok_to_mklabel = true;
- } else {
- if (virStorageBackendDeviceIsEmpty(def->source.devices[0].path,
- fmt, true))
- ok_to_mklabel = true;
- }
+ if (!(flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) &&
+ !(virStorageBackendDeviceIsEmpty(def->source.devices[0].path,
+ fmt, true)))
+ return -1;
- if (ok_to_mklabel) {
- if (virStorageBackendZeroPartitionTable(def->source.devices[0].path,
- 1024 * 1024) < 0)
- goto error;
+ if (virStorageBackendZeroPartitionTable(def->source.devices[0].path,
+ 1024 * 1024) < 0)
+ return -1;
- /* eg parted /dev/sda mklabel --script msdos */
- if (format == VIR_STORAGE_POOL_DISK_UNKNOWN)
- format = def->source.format = VIR_STORAGE_POOL_DISK_DOS;
- if (format == VIR_STORAGE_POOL_DISK_DOS)
- fmt = "msdos";
- else
- fmt = virStoragePoolFormatDiskTypeToString(format);
-
- cmd = virCommandNewArgList(PARTED,
- def->source.devices[0].path,
- "mklabel",
- "--script",
- fmt,
- NULL);
- ret = virCommandRun(cmd, NULL);
- }
+ /* eg parted /dev/sda mklabel --script msdos */
+ if (format == VIR_STORAGE_POOL_DISK_UNKNOWN)
+ format = def->source.format = VIR_STORAGE_POOL_DISK_DOS;
+ if (format == VIR_STORAGE_POOL_DISK_DOS)
+ fmt = "msdos";
+ else
+ fmt = virStoragePoolFormatDiskTypeToString(format);
- error:
- virCommandFree(cmd);
- return ret;
+ cmd = virCommandNewArgList(PARTED,
+ def->source.devices[0].path,
+ "mklabel",
+ "--script",
+ fmt,
+ NULL);
+ return virCommandRun(cmd, NULL);
}
@@ -787,9 +774,9 @@ virStorageBackendDiskDeleteVol(virStoragePoolObjPtr pool,
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
char *src_path = def->source.devices[0].path;
char *srcname = last_component(src_path);
- virCommandPtr cmd = NULL;
bool isDevMapperDevice;
int rc = -1;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
virCheckFlags(0, -1);
@@ -862,7 +849,6 @@ virStorageBackendDiskDeleteVol(virStoragePoolObjPtr pool,
rc = 0;
cleanup:
VIR_FREE(devpath);
- virCommandFree(cmd);
return rc;
}
@@ -876,11 +862,13 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool,
unsigned long long startOffset = 0, endOffset = 0;
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
virErrorPtr save_err;
- virCommandPtr cmd = virCommandNewArgList(PARTED,
- def->source.devices[0].path,
- "mkpart",
- "--script",
- NULL);
+ VIR_AUTOPTR(virCommand) cmd = NULL;
+
+ cmd = virCommandNewArgList(PARTED,
+ def->source.devices[0].path,
+ "mkpart",
+ "--script",
+ NULL);
if (vol->target.encryption &&
vol->target.encryption->format != VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) {
@@ -934,7 +922,6 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool,
cleanup:
VIR_FREE(partFormat);
- virCommandFree(cmd);
return res;
error:
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 420601a303..0436c25af0 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -95,8 +95,6 @@ virStorageBackendFileSystemNetFindPoolSourcesFunc(char **const groups,
static int
virStorageBackendFileSystemNetFindNFSPoolSources(virNetfsDiscoverState *state)
{
- int ret = -1;
-
/*
* # showmount --no-headers -e HOSTNAME
* /tmp *
@@ -112,7 +110,7 @@ virStorageBackendFileSystemNetFindNFSPoolSources(virNetfsDiscoverState *state)
1
};
- virCommandPtr cmd = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
cmd = virCommandNewArgList(SHOWMOUNT,
"--no-headers",
@@ -120,16 +118,9 @@ virStorageBackendFileSystemNetFindNFSPoolSources(virNetfsDiscoverState *state)
state->host,
NULL);
- if (virCommandRunRegex(cmd, 1, regexes, vars,
- virStorageBackendFileSystemNetFindPoolSourcesFunc,
- state, NULL, NULL) < 0)
- goto cleanup;
-
- ret = 0;
-
- cleanup:
- virCommandFree(cmd);
- return ret;
+ return virCommandRunRegex(cmd, 1, regexes, vars,
+ virStorageBackendFileSystemNetFindPoolSourcesFunc,
+ state, NULL, NULL);
}
@@ -309,9 +300,9 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool)
{
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
char *src = NULL;
- virCommandPtr cmd = NULL;
int ret = -1;
int rc;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
if (virStorageBackendFileSystemIsValid(pool) < 0)
return -1;
@@ -334,7 +325,6 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool)
ret = 0;
cleanup:
- virCommandFree(cmd);
VIR_FREE(src);
return ret;
}
@@ -376,9 +366,8 @@ static int
virStorageBackendFileSystemStop(virStoragePoolObjPtr pool)
{
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
- virCommandPtr cmd = NULL;
- int ret = -1;
int rc;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
if (virStorageBackendFileSystemIsValid(pool) < 0)
return -1;
@@ -388,13 +377,7 @@ virStorageBackendFileSystemStop(virStoragePoolObjPtr pool)
return rc;
cmd = virCommandNewArgList(UMOUNT, def->target.path, NULL);
- if (virCommandRun(cmd, NULL) < 0)
- goto cleanup;
-
- ret = 0;
- cleanup:
- virCommandFree(cmd);
- return ret;
+ return virCommandRun(cmd, NULL);
}
#endif /* WITH_STORAGE_FS */
@@ -432,8 +415,7 @@ static int
virStorageBackendExecuteMKFS(const char *device,
const char *format)
{
- int ret = 0;
- virCommandPtr cmd = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
cmd = virCommandNewArgList(MKFS, "-t", format, NULL);
@@ -456,11 +438,10 @@ virStorageBackendExecuteMKFS(const char *device,
_("Failed to make filesystem of "
"type '%s' on device '%s'"),
format, device);
- ret = -1;
+ return -1;
}
- virCommandFree(cmd);
- return ret;
+ return 0;
}
#else /* #ifdef MKFS */
static int
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c
index fd95bd0d48..9ebc560a46 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -48,13 +48,11 @@ static int
virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool,
bool on)
{
- int ret;
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
- virCommandPtr cmd = virStorageBackendLogicalChangeCmd(VGCHANGE, def, on);
+ VIR_AUTOPTR(virCommand) cmd = NULL;
- ret = virCommandRun(cmd, NULL);
- virCommandFree(cmd);
- return ret;
+ cmd = virStorageBackendLogicalChangeCmd(VGCHANGE, def, on);
+ return virCommandRun(cmd, NULL);
}
@@ -67,11 +65,11 @@ virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool,
static void
virStorageBackendLogicalRemoveDevice(const char *path)
{
- virCommandPtr cmd = virCommandNewArgList(PVREMOVE, path, NULL);
+ VIR_AUTOPTR(virCommand) cmd = NULL;
+ cmd = virCommandNewArgList(PVREMOVE, path, NULL);
if (virCommandRun(cmd, NULL) < 0)
VIR_INFO("Failed to pvremove logical device '%s'", path);
- virCommandFree(cmd);
}
@@ -85,8 +83,7 @@ virStorageBackendLogicalRemoveDevice(const char *path)
static int
virStorageBackendLogicalInitializeDevice(const char *path)
{
- int ret = -1;
- virCommandPtr pvcmd = NULL;
+ VIR_AUTOPTR(virCommand) pvcmd = NULL;
/*
* LVM requires that the first sector is blanked if using
@@ -101,15 +98,7 @@ virStorageBackendLogicalInitializeDevice(const char *path)
* clever enough todo this for us :-(
*/
pvcmd = virCommandNewArgList(PVCREATE, path, NULL);
- if (virCommandRun(pvcmd, NULL) < 0)
- goto cleanup;
-
- ret = 0;
-
- cleanup:
- virCommandFree(pvcmd);
-
- return ret;
+ return virCommandRun(pvcmd, NULL);
}
@@ -426,13 +415,12 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool,
int vars[] = {
VIR_STORAGE_VOL_LOGICAL_REGEX_COUNT
};
- int ret = -1;
- virCommandPtr cmd;
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
struct virStorageBackendLogicalPoolVolData cbdata = {
.pool = pool,
.vol = vol,
};
+ VIR_AUTOPTR(virCommand) cmd = NULL;
cmd = virCommandNewArgList(LVS,
"--separator", "#",
@@ -444,20 +432,9 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool,
"lv_name,origin,uuid,devices,segtype,stripes,seg_size,vg_extent_size,size,lv_attr",
def->source.name,
NULL);
- if (virCommandRunRegex(cmd,
- 1,
- regexes,
- vars,
- virStorageBackendLogicalMakeVol,
- &cbdata,
- "lvs",
- NULL) < 0)
- goto cleanup;
-
- ret = 0;
- cleanup:
- virCommandFree(cmd);
- return ret;
+ return virCommandRunRegex(cmd, 1, regexes, vars,
+ virStorageBackendLogicalMakeVol,
+ &cbdata, "lvs", NULL);
}
static int
@@ -550,8 +527,7 @@ virStorageBackendLogicalGetPoolSources(virStoragePoolSourceListPtr sourceList)
int vars[] = {
2
};
- virCommandPtr cmd;
- int ret = -1;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
/*
* NOTE: ignoring errors here; this is just to "touch" any logical volumes
@@ -567,15 +543,9 @@ virStorageBackendLogicalGetPoolSources(virStoragePoolSourceListPtr sourceList)
"--noheadings",
"-o", "pv_name,vg_name",
NULL, NULL);
- if (virCommandRunRegex(cmd, 1, regexes, vars,
- virStorageBackendLogicalFindPoolSourcesFunc,
- sourceList, "pvs", NULL) < 0)
- goto cleanup;
- ret = 0;
-
- cleanup:
- virCommandFree(cmd);
- return ret;
+ return virCommandRunRegex(cmd, 1, regexes, vars,
+ virStorageBackendLogicalFindPoolSourcesFunc,
+ sourceList, "pvs", NULL);
}
@@ -737,9 +707,9 @@ virStorageBackendLogicalBuildPool(virStoragePoolObjPtr pool,
unsigned int flags)
{
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
- virCommandPtr vgcmd = NULL;
int ret = -1;
size_t i = 0;
+ VIR_AUTOPTR(virCommand) vgcmd = NULL;
virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE |
VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret);
@@ -773,8 +743,6 @@ virStorageBackendLogicalBuildPool(virStoragePoolObjPtr pool,
ret = 0;
cleanup:
- virCommandFree(vgcmd);
-
/* On any failure, run through the devices that had pvcreate run in
* in order to run pvremove on the device; otherwise, subsequent build
* will fail if a pvcreate had been run already. */
@@ -805,8 +773,8 @@ virStorageBackendLogicalRefreshPool(virStoragePoolObjPtr pool)
2
};
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
- virCommandPtr cmd = NULL;
int ret = -1;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
virWaitForDevices();
@@ -838,7 +806,6 @@ virStorageBackendLogicalRefreshPool(virStoragePoolObjPtr pool)
ret = 0;
cleanup:
- virCommandFree(cmd);
if (ret < 0)
virStoragePoolObjClearVols(pool);
return ret;
@@ -863,9 +830,8 @@ virStorageBackendLogicalDeletePool(virStoragePoolObjPtr pool,
unsigned int flags)
{
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
- virCommandPtr cmd = NULL;
size_t i;
- int ret = -1;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
virCheckFlags(0, -1);
@@ -874,17 +840,13 @@ virStorageBackendLogicalDeletePool(virStoragePoolObjPtr pool,
"-f", def->source.name,
NULL);
if (virCommandRun(cmd, NULL) < 0)
- goto cleanup;
+ return -1;
/* now remove the pv devices and clear them out */
for (i = 0; i < def->source.ndevice; i++)
virStorageBackendLogicalRemoveDevice(def->source.devices[i].path);
- ret = 0;
-
- cleanup:
- virCommandFree(cmd);
- return ret;
+ return 0;
}
@@ -893,10 +855,8 @@ virStorageBackendLogicalDeleteVol(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
virStorageVolDefPtr vol,
unsigned int flags)
{
- int ret = -1;
-
- virCommandPtr lvchange_cmd = NULL;
- virCommandPtr lvremove_cmd = NULL;
+ VIR_AUTOPTR(virCommand) lvchange_cmd = NULL;
+ VIR_AUTOPTR(virCommand) lvremove_cmd = NULL;
virCheckFlags(0, -1);
@@ -907,18 +867,14 @@ virStorageBackendLogicalDeleteVol(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
if (virCommandRun(lvremove_cmd, NULL) < 0) {
if (virCommandRun(lvchange_cmd, NULL) < 0) {
- goto cleanup;
+ return -1;
} else {
if (virCommandRun(lvremove_cmd, NULL) < 0)
- goto cleanup;
+ return -1;
}
}
- ret = 0;
- cleanup:
- virCommandFree(lvchange_cmd);
- virCommandFree(lvremove_cmd);
- return ret;
+ return 0;
}
@@ -926,9 +882,8 @@ static int
virStorageBackendLogicalLVCreate(virStorageVolDefPtr vol,
virStoragePoolDefPtr def)
{
- int ret;
unsigned long long capacity = vol->target.capacity;
- virCommandPtr cmd = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
if (vol->target.encryption &&
vol->target.encryption->format != VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) {
@@ -961,9 +916,7 @@ virStorageBackendLogicalLVCreate(virStorageVolDefPtr vol,
else
virCommandAddArg(cmd, def->source.name);
- ret = virCommandRun(cmd, NULL);
- virCommandFree(cmd);
- return ret;
+ return virCommandRun(cmd, NULL);
}
diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c
index 9ab318bb4d..73dcfb2f40 100644
--- a/src/storage/storage_backend_sheepdog.c
+++ b/src/storage/storage_backend_sheepdog.c
@@ -141,8 +141,9 @@ virStorageBackendSheepdogRefreshAllVol(virStoragePoolObjPtr pool)
size_t i;
VIR_AUTOPTR(virString) lines = NULL;
VIR_AUTOPTR(virString) cells = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
- virCommandPtr cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "list", "-r", NULL);
+ cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "list", "-r", NULL);
virStorageBackendSheepdogAddHostArg(cmd, pool);
virCommandSetOutputBuffer(cmd, &output);
if (virCommandRun(cmd, NULL) < 0)
@@ -172,7 +173,6 @@ virStorageBackendSheepdogRefreshAllVol(virStoragePoolObjPtr pool)
ret = 0;
cleanup:
- virCommandFree(cmd);
VIR_FREE(output);
return ret;
}
@@ -183,7 +183,7 @@ virStorageBackendSheepdogRefreshPool(virStoragePoolObjPtr pool)
{
int ret = -1;
char *output = NULL;
- virCommandPtr cmd;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
cmd = virCommandNewArgList(SHEEPDOGCLI, "node", "info", "-r", NULL);
virStorageBackendSheepdogAddHostArg(cmd, pool);
@@ -197,7 +197,6 @@ virStorageBackendSheepdogRefreshPool(virStoragePoolObjPtr pool)
ret = virStorageBackendSheepdogRefreshAllVol(pool);
cleanup:
- virCommandFree(cmd);
VIR_FREE(output);
return ret;
}
@@ -208,14 +207,13 @@ virStorageBackendSheepdogDeleteVol(virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
unsigned int flags)
{
+ VIR_AUTOPTR(virCommand) cmd = NULL;
+
virCheckFlags(0, -1);
- virCommandPtr cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "delete", vol->name, NULL);
+ cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "delete", vol->name, NULL);
virStorageBackendSheepdogAddHostArg(cmd, pool);
- int ret = virCommandRun(cmd, NULL);
-
- virCommandFree(cmd);
- return ret;
+ return virCommandRun(cmd, NULL);
}
@@ -252,27 +250,20 @@ virStorageBackendSheepdogBuildVol(virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
unsigned int flags)
{
- int ret = -1;
- virCommandPtr cmd = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
virCheckFlags(0, -1);
if (!vol->target.capacity) {
virReportError(VIR_ERR_NO_SUPPORT, "%s",
_("volume capacity required for this pool"));
- goto cleanup;
+ return -1;
}
cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "create", vol->name, NULL);
virCommandAddArgFormat(cmd, "%llu", vol->target.capacity);
virStorageBackendSheepdogAddHostArg(cmd, pool);
- if (virCommandRun(cmd, NULL) < 0)
- goto cleanup;
-
- ret = 0;
- cleanup:
- virCommandFree(cmd);
- return ret;
+ return virCommandRun(cmd, NULL);
}
@@ -341,33 +332,30 @@ static int
virStorageBackendSheepdogRefreshVol(virStoragePoolObjPtr pool,
virStorageVolDefPtr vol)
{
- int ret;
char *output = NULL;
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
- virCommandPtr cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "list", vol->name, "-r", NULL);
+ VIR_AUTOPTR(virCommand) cmd = NULL;
+ cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "list", vol->name, "-r", NULL);
virStorageBackendSheepdogAddHostArg(cmd, pool);
virCommandSetOutputBuffer(cmd, &output);
- ret = virCommandRun(cmd, NULL);
-
- if (ret < 0)
- goto cleanup;
+ if (virCommandRun(cmd, NULL) < 0)
+ return -1;
- if ((ret = virStorageBackendSheepdogParseVdiList(vol, output)) < 0)
- goto cleanup;
+ if (virStorageBackendSheepdogParseVdiList(vol, output) < 0)
+ return -1;
vol->type = VIR_STORAGE_VOL_NETWORK;
VIR_FREE(vol->key);
if (virAsprintf(&vol->key, "%s/%s",
def->source.name, vol->name) < 0)
- goto cleanup;
+ return -1;
VIR_FREE(vol->target.path);
ignore_value(VIR_STRDUP(vol->target.path, vol->name));
- cleanup:
- virCommandFree(cmd);
- return ret;
+
+ return 0;
}
@@ -377,15 +365,14 @@ virStorageBackendSheepdogResizeVol(virStoragePoolObjPtr pool,
unsigned long long capacity,
unsigned int flags)
{
+ VIR_AUTOPTR(virCommand) cmd = NULL;
+
virCheckFlags(0, -1);
- virCommandPtr cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "resize", vol->name, NULL);
+ cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "resize", vol->name, NULL);
virCommandAddArgFormat(cmd, "%llu", capacity);
virStorageBackendSheepdogAddHostArg(cmd, pool);
- int ret = virCommandRun(cmd, NULL);
-
- virCommandFree(cmd);
- return ret;
+ return virCommandRun(cmd, NULL);
}
diff --git a/src/storage/storage_backend_vstorage.c b/src/storage/storage_backend_vstorage.c
index 0a9abd446b..8c5becd4c4 100644
--- a/src/storage/storage_backend_vstorage.c
+++ b/src/storage/storage_backend_vstorage.c
@@ -39,10 +39,10 @@ virStorageBackendVzPoolStart(virStoragePoolObjPtr pool)
{
int ret = -1;
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
- virCommandPtr cmd = NULL;
char *grp_name = NULL;
char *usr_name = NULL;
char *mode = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
/* Check the permissions */
if (def->target.perms.mode == (mode_t)-1)
@@ -75,7 +75,6 @@ virStorageBackendVzPoolStart(virStoragePoolObjPtr pool)
ret = 0;
cleanup:
- virCommandFree(cmd);
VIR_FREE(mode);
VIR_FREE(grp_name);
VIR_FREE(usr_name);
@@ -125,22 +124,15 @@ static int
virStorageBackendVzPoolStop(virStoragePoolObjPtr pool)
{
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
- virCommandPtr cmd = NULL;
- int ret = -1;
int rc;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
/* Short-circuit if already unmounted */
if ((rc = virStorageBackendVzIsMounted(pool)) != 1)
return rc;
cmd = virCommandNewArgList(UMOUNT, def->target.path, NULL);
- if (virCommandRun(cmd, NULL) < 0)
- goto cleanup;
-
- ret = 0;
- cleanup:
- virCommandFree(cmd);
- return ret;
+ return virCommandRun(cmd, NULL);
}
diff --git a/src/storage/storage_backend_zfs.c b/src/storage/storage_backend_zfs.c
index 4235b48c14..7d1a3dd2cd 100644
--- a/src/storage/storage_backend_zfs.c
+++ b/src/storage/storage_backend_zfs.c
@@ -51,9 +51,9 @@ VIR_LOG_INIT("storage.storage_backend_zfs");
static int
virStorageBackendZFSVolModeNeeded(void)
{
- virCommandPtr cmd = NULL;
int ret = -1, exit_code = -1;
char *error = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
/* 'zfs get' without arguments prints out
* usage information to stderr, including
@@ -77,7 +77,6 @@ virStorageBackendZFSVolModeNeeded(void)
ret = 0;
cleanup:
- virCommandFree(cmd);
VIR_FREE(error);
return ret;
}
@@ -179,10 +178,10 @@ virStorageBackendZFSFindVols(virStoragePoolObjPtr pool,
virStorageVolDefPtr vol)
{
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
- virCommandPtr cmd = NULL;
char *volumes_list = NULL;
size_t i;
VIR_AUTOPTR(virString) lines = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
/**
* $ zfs list -Hp -t volume -o name,volsize -r test
@@ -218,7 +217,6 @@ virStorageBackendZFSFindVols(virStoragePoolObjPtr pool,
}
cleanup:
- virCommandFree(cmd);
VIR_FREE(volumes_list);
return 0;
@@ -228,9 +226,9 @@ static int
virStorageBackendZFSRefreshPool(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED)
{
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
- virCommandPtr cmd = NULL;
char *zpool_props = NULL;
size_t i;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
VIR_AUTOPTR(virString) lines = NULL;
VIR_AUTOPTR(virString) tokens = NULL;
@@ -292,7 +290,6 @@ virStorageBackendZFSRefreshPool(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED)
goto cleanup;
cleanup:
- virCommandFree(cmd);
VIR_FREE(zpool_props);
return 0;
@@ -303,9 +300,9 @@ virStorageBackendZFSCreateVol(virStoragePoolObjPtr pool,
virStorageVolDefPtr vol)
{
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
- virCommandPtr cmd = NULL;
int ret = -1;
int volmode_needed = -1;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
if (vol->target.encryption != NULL) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -364,7 +361,6 @@ virStorageBackendZFSCreateVol(virStoragePoolObjPtr pool,
ret = 0;
cleanup:
- virCommandFree(cmd);
return ret;
}
@@ -374,9 +370,8 @@ virStorageBackendZFSDeleteVol(virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
unsigned int flags)
{
- int ret = -1;
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
- virCommandPtr destroy_cmd = NULL;
+ VIR_AUTOPTR(virCommand) destroy_cmd = NULL;
virCheckFlags(0, -1);
@@ -385,13 +380,7 @@ virStorageBackendZFSDeleteVol(virStoragePoolObjPtr pool,
virCommandAddArgFormat(destroy_cmd, "%s/%s",
def->source.name, vol->name);
- if (virCommandRun(destroy_cmd, NULL) < 0)
- goto cleanup;
-
- ret = 0;
- cleanup:
- virCommandFree(destroy_cmd);
- return ret;
+ return virCommandRun(destroy_cmd, NULL);
}
static int
@@ -399,9 +388,8 @@ virStorageBackendZFSBuildPool(virStoragePoolObjPtr pool,
unsigned int flags)
{
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
- virCommandPtr cmd = NULL;
size_t i;
- int ret = -1;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
virCheckFlags(0, -1);
@@ -417,14 +405,7 @@ virStorageBackendZFSBuildPool(virStoragePoolObjPtr pool,
for (i = 0; i < def->source.ndevice; i++)
virCommandAddArg(cmd, def->source.devices[i].path);
- if (virCommandRun(cmd, NULL) < 0)
- goto cleanup;
-
- ret = 0;
-
- cleanup:
- virCommandFree(cmd);
- return ret;
+ return virCommandRun(cmd, NULL);
}
static int
@@ -432,22 +413,14 @@ virStorageBackendZFSDeletePool(virStoragePoolObjPtr pool,
unsigned int flags)
{
virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
- virCommandPtr cmd = NULL;
- int ret = -1;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
virCheckFlags(0, -1);
cmd = virCommandNewArgList(ZPOOL, "destroy",
def->source.name, NULL);
- if (virCommandRun(cmd, NULL) < 0)
- goto cleanup;
-
- ret = 0;
-
- cleanup:
- virCommandFree(cmd);
- return ret;
+ return virCommandRun(cmd, NULL);
}
virStorageBackend virStorageBackendZFS = {
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index c684f44475..a71a16add1 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -2207,9 +2207,9 @@ static int
virStorageBackendPloopRestoreDesc(char *path)
{
int ret = -1;
- virCommandPtr cmd = NULL;
char *refresh_tool = NULL;
char *desc = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
if (virAsprintf(&desc, "%s/DiskDescriptor.xml", path) < 0)
return ret;
@@ -2238,7 +2238,6 @@ virStorageBackendPloopRestoreDesc(char *path)
cleanup:
VIR_FREE(refresh_tool);
- virCommandFree(cmd);
VIR_FREE(desc);
return ret;
}
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index 1770d21f33..fa364941c5 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -617,9 +617,9 @@ storageBackendCreatePloop(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
unsigned int flags)
{
int ret = -1;
- virCommandPtr cmd = NULL;
char *create_tool = NULL;
bool created = false;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
virCheckFlags(0, -1);
@@ -677,7 +677,6 @@ storageBackendCreatePloop(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
created = true;
ret = virCommandRun(cmd, NULL);
cleanup:
- virCommandFree(cmd);
VIR_FREE(create_tool);
if (ret < 0 && created)
virFileDeleteTree(vol->target.path);
@@ -690,8 +689,8 @@ storagePloopResize(virStorageVolDefPtr vol,
unsigned long long capacity)
{
int ret = -1;
- virCommandPtr cmd = NULL;
char *resize_tool = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
resize_tool = virFindFileInPath("ploop");
if (!resize_tool) {
@@ -705,7 +704,6 @@ storagePloopResize(virStorageVolDefPtr vol,
virCommandAddArgFormat(cmd, "%s/DiskDescriptor.xml", vol->target.path);
ret = virCommandRun(cmd, NULL);
- virCommandFree(cmd);
VIR_FREE(resize_tool);
return ret;
}
@@ -1319,8 +1317,7 @@ storageBackendDoCreateQemuImg(virStoragePoolObjPtr pool,
const char *inputSecretPath,
virStorageVolEncryptConvertStep convertStep)
{
- int ret;
- virCommandPtr cmd;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
cmd = virStorageBackendCreateQemuImgCmdFromVol(pool, vol, inputvol,
flags, create_tool,
@@ -1329,11 +1326,7 @@ storageBackendDoCreateQemuImg(virStoragePoolObjPtr pool,
if (!cmd)
return -1;
- ret = virStorageBackendCreateExecCommand(pool, vol, cmd);
-
- virCommandFree(cmd);
-
- return ret;
+ return virStorageBackendCreateExecCommand(pool, vol, cmd);
}
@@ -2324,11 +2317,11 @@ storageBackendResizeQemuImg(virStoragePoolObjPtr pool,
{
int ret = -1;
char *img_tool = NULL;
- virCommandPtr cmd = NULL;
const char *type;
char *secretPath = NULL;
char *secretAlias = NULL;
virStorageEncryptionPtr enc = vol->target.encryption;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
if (enc && (enc->format == VIR_STORAGE_ENCRYPTION_FORMAT_QCOW ||
enc->format == VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT) &&
@@ -2395,7 +2388,6 @@ storageBackendResizeQemuImg(virStoragePoolObjPtr pool,
VIR_FREE(secretPath);
}
VIR_FREE(secretAlias);
- virCommandFree(cmd);
return ret;
}
@@ -2449,10 +2441,10 @@ virStorageBackendVolResizeLocal(virStoragePoolObjPtr pool,
static int
storageBackendPloopHasSnapshots(char *path)
{
- virCommandPtr cmd = NULL;
char *output = NULL;
char *snap_tool = NULL;
int ret = -1;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
snap_tool = virFindFileInPath("ploop");
if (!snap_tool) {
@@ -2477,7 +2469,6 @@ storageBackendPloopHasSnapshots(char *path)
cleanup:
VIR_FREE(output);
- virCommandFree(cmd);
return ret;
}
@@ -2685,7 +2676,7 @@ storageBackendVolWipeLocalFile(const char *path,
int ret = -1, fd = -1;
const char *alg_char = NULL;
struct stat st;
- virCommandPtr cmd = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
fd = open(path, O_RDWR);
if (fd == -1) {
@@ -2763,7 +2754,6 @@ storageBackendVolWipeLocalFile(const char *path,
}
cleanup:
- virCommandFree(cmd);
VIR_FORCE_CLOSE(fd);
return ret;
}
@@ -2773,10 +2763,10 @@ static int
storageBackendVolWipePloop(virStorageVolDefPtr vol,
unsigned int algorithm)
{
- virCommandPtr cmd = NULL;
char *target_path = NULL;
char *disk_desc = NULL;
char *create_tool = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
int ret = -1;
@@ -2820,7 +2810,6 @@ storageBackendVolWipePloop(virStorageVolDefPtr vol,
VIR_FREE(disk_desc);
VIR_FREE(target_path);
VIR_FREE(create_tool);
- virCommandFree(cmd);
return ret;
}
@@ -3034,8 +3023,8 @@ virStorageBackendFindGlusterPoolSources(const char *host,
{
char *glusterpath = NULL;
char *outbuf = NULL;
- virCommandPtr cmd = NULL;
int rc;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
int ret = -1;
@@ -3069,7 +3058,6 @@ virStorageBackendFindGlusterPoolSources(const char *host,
cleanup:
VIR_FREE(outbuf);
- virCommandFree(cmd);
VIR_FREE(glusterpath);
return ret;
}
@@ -3309,12 +3297,13 @@ virStorageBackendPARTEDFindLabel(const char *device,
const char *const args[] = {
device, "print", "--script", NULL,
};
- virCommandPtr cmd = virCommandNew(PARTED);
char *output = NULL;
char *error = NULL;
char *start, *end;
int ret = VIR_STORAGE_PARTED_ERROR;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
+ cmd = virCommandNew(PARTED);
virCommandAddArgSet(cmd, args);
virCommandAddEnvString(cmd, "LC_ALL=C");
virCommandSetOutputBuffer(cmd, &output);
@@ -3359,7 +3348,6 @@ virStorageBackendPARTEDFindLabel(const char *device,
ret = VIR_STORAGE_PARTED_DIFFERENT;
cleanup:
- virCommandFree(cmd);
VIR_FREE(output);
VIR_FREE(error);
return ret;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index fc26c2f22e..828e95d5d3 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1376,13 +1376,14 @@ int virStorageFileGetLVMKey(const char *path,
* 06UgP5-2rhb-w3Bo-3mdR-WeoL-pytO-SAa2ky
*/
int status;
- virCommandPtr cmd = virCommandNewArgList(LVS, "--noheadings",
- "--unbuffered", "--nosuffix",
- "--options", "uuid", path,
- NULL
- );
int ret = -1;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
+ cmd = virCommandNewArgList(LVS, "--noheadings",
+ "--unbuffered", "--nosuffix",
+ "--options", "uuid", path,
+ NULL
+ );
*key = NULL;
/* Run the program and capture its output */
@@ -1417,8 +1418,6 @@ int virStorageFileGetLVMKey(const char *path,
if (*key && STREQ(*key, ""))
VIR_FREE(*key);
- virCommandFree(cmd);
-
return ret;
}
#else
@@ -1451,20 +1450,20 @@ virStorageFileGetSCSIKey(const char *path,
bool ignoreError ATTRIBUTE_UNUSED)
{
int status;
- virCommandPtr cmd = virCommandNewArgList("/lib/udev/scsi_id",
- "--replace-whitespace",
- "--whitelisted",
- "--device", path,
- NULL
- );
- int ret = -2;
-
+ VIR_AUTOPTR(virCommand) cmd = NULL;
+
+ cmd = virCommandNewArgList("/lib/udev/scsi_id",
+ "--replace-whitespace",
+ "--whitelisted",
+ "--device", path,
+ NULL
+ );
*key = NULL;
/* Run the program and capture its output */
virCommandSetOutputBuffer(cmd, key);
if (virCommandRun(cmd, &status) < 0)
- goto cleanup;
+ return -2;
/* Explicitly check status == 0, rather than passing NULL
* to virCommandRun because we don't want to raise an actual
@@ -1476,15 +1475,10 @@ virStorageFileGetSCSIKey(const char *path,
*nl = '\0';
}
- ret = 0;
-
- cleanup:
if (*key && STREQ(*key, ""))
VIR_FREE(*key);
- virCommandFree(cmd);
-
- return ret;
+ return 0;
}
#else
int virStorageFileGetSCSIKey(const char *path,
@@ -1521,24 +1515,24 @@ virStorageFileGetNPIVKey(const char *path,
char **key)
{
int status;
- VIR_AUTOFREE(char *) outbuf = NULL;
const char *serial;
const char *port;
- virCommandPtr cmd = virCommandNewArgList("/lib/udev/scsi_id",
- "--replace-whitespace",
- "--whitelisted",
- "--export",
- "--device", path,
- NULL
- );
- int ret = -2;
-
+ VIR_AUTOFREE(char *) outbuf = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
+
+ cmd = virCommandNewArgList("/lib/udev/scsi_id",
+ "--replace-whitespace",
+ "--whitelisted",
+ "--export",
+ "--device", path,
+ NULL
+ );
*key = NULL;
/* Run the program and capture its output */
virCommandSetOutputBuffer(cmd, &outbuf);
if (virCommandRun(cmd, &status) < 0)
- goto cleanup;
+ return -2;
/* Explicitly check status == 0, rather than passing NULL
* to virCommandRun because we don't want to raise an actual
@@ -1562,12 +1556,7 @@ virStorageFileGetNPIVKey(const char *path,
ignore_value(virAsprintf(key, "%s_PORT%s", serial, port));
}
- ret = 0;
-
- cleanup:
- virCommandFree(cmd);
-
- return ret;
+ return 0;
}
#else
int virStorageFileGetNPIVKey(const char *path ATTRIBUTE_UNUSED,
diff --git a/tests/storagepoolxml2argvtest.c b/tests/storagepoolxml2argvtest.c
index e7f42dc0f8..c9ba9b3fde 100644
--- a/tests/storagepoolxml2argvtest.c
+++ b/tests/storagepoolxml2argvtest.c
@@ -22,12 +22,12 @@ testCompareXMLToArgvFiles(bool shouldFail,
const char *poolxml,
const char *cmdline)
{
- VIR_AUTOFREE(char *) actualCmdline = NULL;
- VIR_AUTOFREE(char *) src = NULL;
int ret = -1;
- virCommandPtr cmd = NULL;
virStoragePoolDefPtr def = NULL;
virStoragePoolObjPtr pool = NULL;
+ VIR_AUTOFREE(char *) actualCmdline = NULL;
+ VIR_AUTOFREE(char *) src = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
if (!(def = virStoragePoolDefParseFile(poolxml)))
goto cleanup;
@@ -83,7 +83,6 @@ testCompareXMLToArgvFiles(bool shouldFail,
ret = 0;
cleanup:
- virCommandFree(cmd);
VIR_FREE(actualCmdline);
virStoragePoolObjEndAPI(&pool);
if (shouldFail) {
diff --git a/tests/storagevolxml2argvtest.c b/tests/storagevolxml2argvtest.c
index edff8d8477..d4ac02b31d 100644
--- a/tests/storagevolxml2argvtest.c
+++ b/tests/storagevolxml2argvtest.c
@@ -45,14 +45,12 @@ testCompareXMLToArgvFiles(bool shouldFail,
char *actualCmdline = NULL;
virStorageVolEncryptConvertStep convertStep = VIR_STORAGE_VOL_ENCRYPT_NONE;
int ret = -1;
-
- virCommandPtr cmd = NULL;
-
virStoragePoolDefPtr def = NULL;
virStoragePoolObjPtr obj = NULL;
VIR_AUTOPTR(virStorageVolDef) vol = NULL;
VIR_AUTOPTR(virStorageVolDef) inputvol = NULL;
VIR_AUTOPTR(virStoragePoolDef) inputpool = NULL;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
if (!(def = virStoragePoolDefParseFile(poolxml)))
goto cleanup;
@@ -90,6 +88,7 @@ testCompareXMLToArgvFiles(bool shouldFail,
convertStep = VIR_STORAGE_VOL_ENCRYPT_CREATE;
do {
+ virCommandFree(cmd);
cmd = virStorageBackendCreateQemuImgCmdFromVol(obj, vol,
inputvol, flags,
create_tool,
@@ -139,7 +138,6 @@ testCompareXMLToArgvFiles(bool shouldFail,
ret = 0;
cleanup:
- virCommandFree(cmd);
VIR_FREE(actualCmdline);
virStoragePoolObjEndAPI(&obj);
return ret;
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index c7c40b16f8..7272df7e33 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -128,9 +128,9 @@ static int
testPrepImages(void)
{
int ret = EXIT_FAILURE;
- virCommandPtr cmd = NULL;
char *buf = NULL;
bool compat = false;
+ VIR_AUTOPTR(virCommand) cmd = NULL;
qemuimg = virFindFileInPath("qemu-img");
if (!qemuimg)
@@ -246,7 +246,6 @@ testPrepImages(void)
ret = 0;
cleanup:
VIR_FREE(buf);
- virCommandFree(cmd);
if (ret)
testCleanupImages();
return ret;
@@ -713,7 +712,6 @@ static int
mymain(void)
{
int ret;
- virCommandPtr cmd = NULL;
struct testChainData data;
struct testLookupData data2;
struct testPathCanonicalizeData data3;
@@ -722,6 +720,7 @@ mymain(void)
virStorageSourcePtr chain = NULL;
virStorageSourcePtr chain2; /* short for chain->backingStore */
virStorageSourcePtr chain3; /* short for chain2->backingStore */
+ VIR_AUTOPTR(virCommand) cmd = NULL;
if (storageRegisterAll() < 0)
return EXIT_FAILURE;
@@ -1604,7 +1603,6 @@ mymain(void)
/* Final cleanup */
virStorageSourceFree(chain);
testCleanupImages();
- virCommandFree(cmd);
return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Feb 08, 2019 at 01:37:05PM -0500, John Ferlan wrote:
>Let's make use of the auto __cleanup capabilities cleaning up any
>now unnecessary goto paths.
>
>Signed-off-by: John Ferlan <jferlan@redhat.com>
>Reviewed-by: Erik Skultety <eskultet@redhat.com>
>---
> src/storage/storage_backend_disk.c | 85 +++++++++------------
> src/storage/storage_backend_fs.c | 39 +++-------
> src/storage/storage_backend_logical.c | 101 +++++++------------------
> src/storage/storage_backend_sheepdog.c | 59 ++++++---------
> src/storage/storage_backend_vstorage.c | 14 +---
> src/storage/storage_backend_zfs.c | 47 +++---------
> src/storage/storage_driver.c | 3 +-
> src/storage/storage_util.c | 34 +++------
> src/util/virstoragefile.c | 67 +++++++---------
> tests/storagepoolxml2argvtest.c | 7 +-
> tests/storagevolxml2argvtest.c | 6 +-
> tests/virstoragetest.c | 6 +-
> 12 files changed, 156 insertions(+), 312 deletions(-)
>
>@@ -502,51 +500,40 @@ virStorageBackendDiskBuildPool(virStoragePoolObjPtr pool,
> virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
> int format = def->source.format;
> const char *fmt;
>- bool ok_to_mklabel = false;
>- int ret = -1;
>- virCommandPtr cmd = NULL;
>+ VIR_AUTOPTR(virCommand) cmd = NULL;
>
> virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE |
>- VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret);
>+ VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, -1);
>
>- VIR_EXCLUSIVE_FLAGS_GOTO(VIR_STORAGE_POOL_BUILD_OVERWRITE,
>- VIR_STORAGE_POOL_BUILD_NO_OVERWRITE,
>- error);
>+ VIR_EXCLUSIVE_FLAGS_RET(VIR_STORAGE_POOL_BUILD_OVERWRITE,
>+ VIR_STORAGE_POOL_BUILD_NO_OVERWRITE,
>+ -1);
>
> fmt = virStoragePoolFormatDiskTypeToString(format);
>- if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) {
>- ok_to_mklabel = true;
>- } else {
>- if (virStorageBackendDeviceIsEmpty(def->source.devices[0].path,
>- fmt, true))
>- ok_to_mklabel = true;
>- }
>+ if (!(flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) &&
>+ !(virStorageBackendDeviceIsEmpty(def->source.devices[0].path,
>+ fmt, true)))
>+ return -1;
>
>- if (ok_to_mklabel) {
>- if (virStorageBackendZeroPartitionTable(def->source.devices[0].path,
>- 1024 * 1024) < 0)
>- goto error;
>+ if (virStorageBackendZeroPartitionTable(def->source.devices[0].path,
>+ 1024 * 1024) < 0)
>+ return -1;
>
>- /* eg parted /dev/sda mklabel --script msdos */
>- if (format == VIR_STORAGE_POOL_DISK_UNKNOWN)
>- format = def->source.format = VIR_STORAGE_POOL_DISK_DOS;
>- if (format == VIR_STORAGE_POOL_DISK_DOS)
>- fmt = "msdos";
>- else
>- fmt = virStoragePoolFormatDiskTypeToString(format);
>-
>- cmd = virCommandNewArgList(PARTED,
>- def->source.devices[0].path,
>- "mklabel",
>- "--script",
>- fmt,
>- NULL);
>- ret = virCommandRun(cmd, NULL);
>- }
>+ /* eg parted /dev/sda mklabel --script msdos */
>+ if (format == VIR_STORAGE_POOL_DISK_UNKNOWN)
>+ format = def->source.format = VIR_STORAGE_POOL_DISK_DOS;
>+ if (format == VIR_STORAGE_POOL_DISK_DOS)
>+ fmt = "msdos";
>+ else
>+ fmt = virStoragePoolFormatDiskTypeToString(format);
>
>- error:
>- virCommandFree(cmd);
>- return ret;
>+ cmd = virCommandNewArgList(PARTED,
>+ def->source.devices[0].path,
>+ "mklabel",
>+ "--script",
>+ fmt,
>+ NULL);
>+ return virCommandRun(cmd, NULL);
> }
Apart from the usual mixing of the ret->goto changes with adding
AUTOFREE, this also removes the 'ok_to_mklabel' bool.
Those changes really should be separated.
>@@ -341,33 +332,30 @@ static int
> virStorageBackendSheepdogRefreshVol(virStoragePoolObjPtr pool,
> virStorageVolDefPtr vol)
> {
>- int ret;
> char *output = NULL;
> virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
>- virCommandPtr cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "list", vol->name, "-r", NULL);
>+ VIR_AUTOPTR(virCommand) cmd = NULL;
>
>+ cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "list", vol->name, "-r", NULL);
> virStorageBackendSheepdogAddHostArg(cmd, pool);
> virCommandSetOutputBuffer(cmd, &output);
>- ret = virCommandRun(cmd, NULL);
>-
>- if (ret < 0)
>- goto cleanup;
>+ if (virCommandRun(cmd, NULL) < 0)
>+ return -1;
>
>- if ((ret = virStorageBackendSheepdogParseVdiList(vol, output)) < 0)
>- goto cleanup;
>+ if (virStorageBackendSheepdogParseVdiList(vol, output) < 0)
>+ return -1;
>
> vol->type = VIR_STORAGE_VOL_NETWORK;
>
> VIR_FREE(vol->key);
> if (virAsprintf(&vol->key, "%s/%s",
> def->source.name, vol->name) < 0)
>- goto cleanup;
>+ return -1;
Before, '0' from the virStorageBackendSheepdogParseVdiList would be
returned. While correct, it would look better in a separate patch.
>
> VIR_FREE(vol->target.path);
> ignore_value(VIR_STRDUP(vol->target.path, vol->name));
>- cleanup:
>- virCommandFree(cmd);
>- return ret;
>+
>+ return 0;
> }
>
>
To everything apart from virStorageBackendDiskBuildPool:
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 2/11/19 7:33 AM, Ján Tomko wrote:
> On Fri, Feb 08, 2019 at 01:37:05PM -0500, John Ferlan wrote:
>> Let's make use of the auto __cleanup capabilities cleaning up any
>> now unnecessary goto paths.
>>
>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>> Reviewed-by: Erik Skultety <eskultet@redhat.com>
>> ---
>> src/storage/storage_backend_disk.c | 85 +++++++++------------
>> src/storage/storage_backend_fs.c | 39 +++-------
>> src/storage/storage_backend_logical.c | 101 +++++++------------------
>> src/storage/storage_backend_sheepdog.c | 59 ++++++---------
>> src/storage/storage_backend_vstorage.c | 14 +---
>> src/storage/storage_backend_zfs.c | 47 +++---------
>> src/storage/storage_driver.c | 3 +-
>> src/storage/storage_util.c | 34 +++------
>> src/util/virstoragefile.c | 67 +++++++---------
>> tests/storagepoolxml2argvtest.c | 7 +-
>> tests/storagevolxml2argvtest.c | 6 +-
>> tests/virstoragetest.c | 6 +-
>> 12 files changed, 156 insertions(+), 312 deletions(-)
>>
>> @@ -502,51 +500,40 @@
>> virStorageBackendDiskBuildPool(virStoragePoolObjPtr pool,
>> virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
>> int format = def->source.format;
>> const char *fmt;
>> - bool ok_to_mklabel = false;
>> - int ret = -1;
>> - virCommandPtr cmd = NULL;
>> + VIR_AUTOPTR(virCommand) cmd = NULL;
>>
>> virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE |
>> - VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret);
>> + VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, -1);
>>
>> - VIR_EXCLUSIVE_FLAGS_GOTO(VIR_STORAGE_POOL_BUILD_OVERWRITE,
>> - VIR_STORAGE_POOL_BUILD_NO_OVERWRITE,
>> - error);
>> + VIR_EXCLUSIVE_FLAGS_RET(VIR_STORAGE_POOL_BUILD_OVERWRITE,
>> + VIR_STORAGE_POOL_BUILD_NO_OVERWRITE,
>> + -1);
>>
>> fmt = virStoragePoolFormatDiskTypeToString(format);
>> - if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) {
>> - ok_to_mklabel = true;
>> - } else {
>> - if (virStorageBackendDeviceIsEmpty(def->source.devices[0].path,
>> - fmt, true))
>> - ok_to_mklabel = true;
>> - }
>> + if (!(flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) &&
>> + !(virStorageBackendDeviceIsEmpty(def->source.devices[0].path,
>> + fmt, true)))
>> + return -1;
>>
>> - if (ok_to_mklabel) {
>> - if
>> (virStorageBackendZeroPartitionTable(def->source.devices[0].path,
>> - 1024 * 1024) < 0)
>> - goto error;
>> + if (virStorageBackendZeroPartitionTable(def->source.devices[0].path,
>> + 1024 * 1024) < 0)
>> + return -1;
>>
>> - /* eg parted /dev/sda mklabel --script msdos */
>> - if (format == VIR_STORAGE_POOL_DISK_UNKNOWN)
>> - format = def->source.format = VIR_STORAGE_POOL_DISK_DOS;
>> - if (format == VIR_STORAGE_POOL_DISK_DOS)
>> - fmt = "msdos";
>> - else
>> - fmt = virStoragePoolFormatDiskTypeToString(format);
>> -
>> - cmd = virCommandNewArgList(PARTED,
>> - def->source.devices[0].path,
>> - "mklabel",
>> - "--script",
>> - fmt,
>> - NULL);
>> - ret = virCommandRun(cmd, NULL);
>> - }
>> + /* eg parted /dev/sda mklabel --script msdos */
>> + if (format == VIR_STORAGE_POOL_DISK_UNKNOWN)
>> + format = def->source.format = VIR_STORAGE_POOL_DISK_DOS;
>> + if (format == VIR_STORAGE_POOL_DISK_DOS)
>> + fmt = "msdos";
>> + else
>> + fmt = virStoragePoolFormatDiskTypeToString(format);
>>
>> - error:
>> - virCommandFree(cmd);
>> - return ret;
>> + cmd = virCommandNewArgList(PARTED,
>> + def->source.devices[0].path,
>> + "mklabel",
>> + "--script",
>> + fmt,
>> + NULL);
>> + return virCommandRun(cmd, NULL);
>> }
>
> Apart from the usual mixing of the ret->goto changes with adding
> AUTOFREE, this also removes the 'ok_to_mklabel' bool.
> Those changes really should be separated.
>
Just so it's clear what's being requested, does this mean taking the
current code and adding the:
+ if (!(flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) &&
+ !(virStorageBackendDeviceIsEmpty(def->source.devices[0].path,
+ fmt, true)))
+ goto error;
and then reformatting the rest inline as is done here (more or less)?
John
>> @@ -341,33 +332,30 @@ static int
>> virStorageBackendSheepdogRefreshVol(virStoragePoolObjPtr pool,
>> virStorageVolDefPtr vol)
>> {
>> - int ret;
>> char *output = NULL;
>> virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
>> - virCommandPtr cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi",
>> "list", vol->name, "-r", NULL);
>> + VIR_AUTOPTR(virCommand) cmd = NULL;
>>
>> + cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "list", vol->name,
>> "-r", NULL);
>> virStorageBackendSheepdogAddHostArg(cmd, pool);
>> virCommandSetOutputBuffer(cmd, &output);
>> - ret = virCommandRun(cmd, NULL);
>> -
>> - if (ret < 0)
>> - goto cleanup;
>> + if (virCommandRun(cmd, NULL) < 0)
>> + return -1;
>>
>> - if ((ret = virStorageBackendSheepdogParseVdiList(vol, output)) < 0)
>> - goto cleanup;
>> + if (virStorageBackendSheepdogParseVdiList(vol, output) < 0)
>> + return -1;
>>
>> vol->type = VIR_STORAGE_VOL_NETWORK;
>>
>> VIR_FREE(vol->key);
>> if (virAsprintf(&vol->key, "%s/%s",
>> def->source.name, vol->name) < 0)
>> - goto cleanup;
>> + return -1;
>
> Before, '0' from the virStorageBackendSheepdogParseVdiList would be
> returned. While correct, it would look better in a separate patch.
>
>>
>> VIR_FREE(vol->target.path);
>> ignore_value(VIR_STRDUP(vol->target.path, vol->name));
>> - cleanup:
>> - virCommandFree(cmd);
>> - return ret;
>> +
>> + return 0;
>> }
>>
>>
>
> To everything apart from virStorageBackendDiskBuildPool:
> Reviewed-by: Ján Tomko <jtomko@redhat.com>
>
> Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Feb 11, 2019 at 07:41:41AM -0500, John Ferlan wrote:
>
>
>On 2/11/19 7:33 AM, Ján Tomko wrote:
>> On Fri, Feb 08, 2019 at 01:37:05PM -0500, John Ferlan wrote:
>>> Let's make use of the auto __cleanup capabilities cleaning up any
>>> now unnecessary goto paths.
>>>
>>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>>> Reviewed-by: Erik Skultety <eskultet@redhat.com>
>>> ---
>>> src/storage/storage_backend_disk.c | 85 +++++++++------------
>>> src/storage/storage_backend_fs.c | 39 +++-------
>>> src/storage/storage_backend_logical.c | 101 +++++++------------------
>>> src/storage/storage_backend_sheepdog.c | 59 ++++++---------
>>> src/storage/storage_backend_vstorage.c | 14 +---
>>> src/storage/storage_backend_zfs.c | 47 +++---------
>>> src/storage/storage_driver.c | 3 +-
>>> src/storage/storage_util.c | 34 +++------
>>> src/util/virstoragefile.c | 67 +++++++---------
>>> tests/storagepoolxml2argvtest.c | 7 +-
>>> tests/storagevolxml2argvtest.c | 6 +-
>>> tests/virstoragetest.c | 6 +-
>>> 12 files changed, 156 insertions(+), 312 deletions(-)
>>>
>>> @@ -502,51 +500,40 @@
>>> virStorageBackendDiskBuildPool(virStoragePoolObjPtr pool,
>>> virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
>>> int format = def->source.format;
>>> const char *fmt;
>>> - bool ok_to_mklabel = false;
>>> - int ret = -1;
>>> - virCommandPtr cmd = NULL;
>>> + VIR_AUTOPTR(virCommand) cmd = NULL;
>>>
>>> virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE |
>>> - VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret);
>>> + VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, -1);
>>>
>>> - VIR_EXCLUSIVE_FLAGS_GOTO(VIR_STORAGE_POOL_BUILD_OVERWRITE,
>>> - VIR_STORAGE_POOL_BUILD_NO_OVERWRITE,
>>> - error);
>>> + VIR_EXCLUSIVE_FLAGS_RET(VIR_STORAGE_POOL_BUILD_OVERWRITE,
>>> + VIR_STORAGE_POOL_BUILD_NO_OVERWRITE,
>>> + -1);
>>>
>>> fmt = virStoragePoolFormatDiskTypeToString(format);
>>> - if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) {
>>> - ok_to_mklabel = true;
>>> - } else {
>>> - if (virStorageBackendDeviceIsEmpty(def->source.devices[0].path,
>>> - fmt, true))
>>> - ok_to_mklabel = true;
>>> - }
>>> + if (!(flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) &&
>>> + !(virStorageBackendDeviceIsEmpty(def->source.devices[0].path,
>>> + fmt, true)))
>>> + return -1;
>>>
>>> - if (ok_to_mklabel) {
>>> - if
>>> (virStorageBackendZeroPartitionTable(def->source.devices[0].path,
>>> - 1024 * 1024) < 0)
>>> - goto error;
>>> + if (virStorageBackendZeroPartitionTable(def->source.devices[0].path,
>>> + 1024 * 1024) < 0)
>>> + return -1;
>>>
>>> - /* eg parted /dev/sda mklabel --script msdos */
>>> - if (format == VIR_STORAGE_POOL_DISK_UNKNOWN)
>>> - format = def->source.format = VIR_STORAGE_POOL_DISK_DOS;
>>> - if (format == VIR_STORAGE_POOL_DISK_DOS)
>>> - fmt = "msdos";
>>> - else
>>> - fmt = virStoragePoolFormatDiskTypeToString(format);
>>> -
>>> - cmd = virCommandNewArgList(PARTED,
>>> - def->source.devices[0].path,
>>> - "mklabel",
>>> - "--script",
>>> - fmt,
>>> - NULL);
>>> - ret = virCommandRun(cmd, NULL);
>>> - }
>>> + /* eg parted /dev/sda mklabel --script msdos */
>>> + if (format == VIR_STORAGE_POOL_DISK_UNKNOWN)
>>> + format = def->source.format = VIR_STORAGE_POOL_DISK_DOS;
>>> + if (format == VIR_STORAGE_POOL_DISK_DOS)
>>> + fmt = "msdos";
>>> + else
>>> + fmt = virStoragePoolFormatDiskTypeToString(format);
>>>
>>> - error:
>>> - virCommandFree(cmd);
>>> - return ret;
>>> + cmd = virCommandNewArgList(PARTED,
>>> + def->source.devices[0].path,
>>> + "mklabel",
>>> + "--script",
>>> + fmt,
>>> + NULL);
>>> + return virCommandRun(cmd, NULL);
>>> }
>>
>> Apart from the usual mixing of the ret->goto changes with adding
>> AUTOFREE, this also removes the 'ok_to_mklabel' bool.
>> Those changes really should be separated.
>>
>
>Just so it's clear what's being requested, does this mean taking the
>current code and adding the:
>
>+ if (!(flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) &&
>+ !(virStorageBackendDeviceIsEmpty(def->source.devices[0].path,
>+ fmt, true)))
>+ goto error;
>
>and then reformatting the rest inline as is done here (more or less)?
>
Yes. Also, in that case we seem to exit without logging an error (as
opposed to calling virCommandRun which should log one).
Jano
>John
>
>>> @@ -341,33 +332,30 @@ static int
>>> virStorageBackendSheepdogRefreshVol(virStoragePoolObjPtr pool,
>>> virStorageVolDefPtr vol)
>>> {
>>> - int ret;
>>> char *output = NULL;
>>> virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
>>> - virCommandPtr cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi",
>>> "list", vol->name, "-r", NULL);
>>> + VIR_AUTOPTR(virCommand) cmd = NULL;
>>>
>>> + cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "list", vol->name,
>>> "-r", NULL);
>>> virStorageBackendSheepdogAddHostArg(cmd, pool);
>>> virCommandSetOutputBuffer(cmd, &output);
>>> - ret = virCommandRun(cmd, NULL);
>>> -
>>> - if (ret < 0)
>>> - goto cleanup;
>>> + if (virCommandRun(cmd, NULL) < 0)
>>> + return -1;
>>>
>>> - if ((ret = virStorageBackendSheepdogParseVdiList(vol, output)) < 0)
>>> - goto cleanup;
>>> + if (virStorageBackendSheepdogParseVdiList(vol, output) < 0)
>>> + return -1;
>>>
>>> vol->type = VIR_STORAGE_VOL_NETWORK;
>>>
>>> VIR_FREE(vol->key);
>>> if (virAsprintf(&vol->key, "%s/%s",
>>> def->source.name, vol->name) < 0)
>>> - goto cleanup;
>>> + return -1;
>>
>> Before, '0' from the virStorageBackendSheepdogParseVdiList would be
>> returned. While correct, it would look better in a separate patch.
>>
>>>
>>> VIR_FREE(vol->target.path);
>>> ignore_value(VIR_STRDUP(vol->target.path, vol->name));
>>> - cleanup:
>>> - virCommandFree(cmd);
>>> - return ret;
>>> +
>>> + return 0;
>>> }
>>>
>>>
>>
>> To everything apart from virStorageBackendDiskBuildPool:
>> Reviewed-by: Ján Tomko <jtomko@redhat.com>
>>
>> Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 2/11/19 9:52 AM, Ján Tomko wrote:
> On Mon, Feb 11, 2019 at 07:41:41AM -0500, John Ferlan wrote:
>>
>>
>> On 2/11/19 7:33 AM, Ján Tomko wrote:
>>> On Fri, Feb 08, 2019 at 01:37:05PM -0500, John Ferlan wrote:
>>>> Let's make use of the auto __cleanup capabilities cleaning up any
>>>> now unnecessary goto paths.
>>>>
>>>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>>>> Reviewed-by: Erik Skultety <eskultet@redhat.com>
>>>> ---
>>>> src/storage/storage_backend_disk.c | 85 +++++++++------------
>>>> src/storage/storage_backend_fs.c | 39 +++-------
>>>> src/storage/storage_backend_logical.c | 101 +++++++------------------
>>>> src/storage/storage_backend_sheepdog.c | 59 ++++++---------
>>>> src/storage/storage_backend_vstorage.c | 14 +---
>>>> src/storage/storage_backend_zfs.c | 47 +++---------
>>>> src/storage/storage_driver.c | 3 +-
>>>> src/storage/storage_util.c | 34 +++------
>>>> src/util/virstoragefile.c | 67 +++++++---------
>>>> tests/storagepoolxml2argvtest.c | 7 +-
>>>> tests/storagevolxml2argvtest.c | 6 +-
>>>> tests/virstoragetest.c | 6 +-
>>>> 12 files changed, 156 insertions(+), 312 deletions(-)
>>>>
>>>> @@ -502,51 +500,40 @@
>>>> virStorageBackendDiskBuildPool(virStoragePoolObjPtr pool,
>>>> virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
>>>> int format = def->source.format;
>>>> const char *fmt;
>>>> - bool ok_to_mklabel = false;
>>>> - int ret = -1;
>>>> - virCommandPtr cmd = NULL;
>>>> + VIR_AUTOPTR(virCommand) cmd = NULL;
>>>>
>>>> virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE |
>>>> - VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret);
>>>> + VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, -1);
>>>>
>>>> - VIR_EXCLUSIVE_FLAGS_GOTO(VIR_STORAGE_POOL_BUILD_OVERWRITE,
>>>> - VIR_STORAGE_POOL_BUILD_NO_OVERWRITE,
>>>> - error);
>>>> + VIR_EXCLUSIVE_FLAGS_RET(VIR_STORAGE_POOL_BUILD_OVERWRITE,
>>>> + VIR_STORAGE_POOL_BUILD_NO_OVERWRITE,
>>>> + -1);
>>>>
>>>> fmt = virStoragePoolFormatDiskTypeToString(format);
>>>> - if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) {
>>>> - ok_to_mklabel = true;
>>>> - } else {
>>>> - if
>>>> (virStorageBackendDeviceIsEmpty(def->source.devices[0].path,
>>>> - fmt, true))
>>>> - ok_to_mklabel = true;
>>>> - }
>>>> + if (!(flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) &&
>>>> + !(virStorageBackendDeviceIsEmpty(def->source.devices[0].path,
>>>> + fmt, true)))
>>>> + return -1;
>>>>
>>>> - if (ok_to_mklabel) {
>>>> - if
>>>> (virStorageBackendZeroPartitionTable(def->source.devices[0].path,
>>>> - 1024 * 1024) < 0)
>>>> - goto error;
>>>> + if
>>>> (virStorageBackendZeroPartitionTable(def->source.devices[0].path,
>>>> + 1024 * 1024) < 0)
>>>> + return -1;
>>>>
>>>> - /* eg parted /dev/sda mklabel --script msdos */
>>>> - if (format == VIR_STORAGE_POOL_DISK_UNKNOWN)
>>>> - format = def->source.format = VIR_STORAGE_POOL_DISK_DOS;
>>>> - if (format == VIR_STORAGE_POOL_DISK_DOS)
>>>> - fmt = "msdos";
>>>> - else
>>>> - fmt = virStoragePoolFormatDiskTypeToString(format);
>>>> -
>>>> - cmd = virCommandNewArgList(PARTED,
>>>> - def->source.devices[0].path,
>>>> - "mklabel",
>>>> - "--script",
>>>> - fmt,
>>>> - NULL);
>>>> - ret = virCommandRun(cmd, NULL);
>>>> - }
>>>> + /* eg parted /dev/sda mklabel --script msdos */
>>>> + if (format == VIR_STORAGE_POOL_DISK_UNKNOWN)
>>>> + format = def->source.format = VIR_STORAGE_POOL_DISK_DOS;
>>>> + if (format == VIR_STORAGE_POOL_DISK_DOS)
>>>> + fmt = "msdos";
>>>> + else
>>>> + fmt = virStoragePoolFormatDiskTypeToString(format);
>>>>
>>>> - error:
>>>> - virCommandFree(cmd);
>>>> - return ret;
>>>> + cmd = virCommandNewArgList(PARTED,
>>>> + def->source.devices[0].path,
>>>> + "mklabel",
>>>> + "--script",
>>>> + fmt,
>>>> + NULL);
>>>> + return virCommandRun(cmd, NULL);
>>>> }
>>>
>>> Apart from the usual mixing of the ret->goto changes with adding
>>> AUTOFREE, this also removes the 'ok_to_mklabel' bool.
>>> Those changes really should be separated.
>>>
>>
>> Just so it's clear what's being requested, does this mean taking the
>> current code and adding the:
>>
>> + if (!(flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) &&
>> + !(virStorageBackendDeviceIsEmpty(def->source.devices[0].path,
>> + fmt, true)))
>> + goto error;
>>
>> and then reformatting the rest inline as is done here (more or less)?
>>
>
> Yes. Also, in that case we seem to exit without logging an error (as
> opposed to calling virCommandRun which should log one).
>
> Jano
Not sure what was meant about we seem to exit without logging an error.
If virStorageBackendDeviceIsEmpty an error message is generated if false
is returned. Here's what I have for the difference (attached with any
luck):
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Feb 11, 2019 at 09:33:53PM -0500, John Ferlan wrote:
>
>
>On 2/11/19 9:52 AM, Ján Tomko wrote:
>> On Mon, Feb 11, 2019 at 07:41:41AM -0500, John Ferlan wrote:
>>>
>>>
>>> On 2/11/19 7:33 AM, Ján Tomko wrote:
>>>> On Fri, Feb 08, 2019 at 01:37:05PM -0500, John Ferlan wrote:
>>>>> Let's make use of the auto __cleanup capabilities cleaning up any
>>>>> now unnecessary goto paths.
>>>>>
>>>>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>>>>> Reviewed-by: Erik Skultety <eskultet@redhat.com>
>>>>> ---
>>>>> src/storage/storage_backend_disk.c | 85 +++++++++------------
>>>>> src/storage/storage_backend_fs.c | 39 +++-------
>>>>> src/storage/storage_backend_logical.c | 101 +++++++------------------
>>>>> src/storage/storage_backend_sheepdog.c | 59 ++++++---------
>>>>> src/storage/storage_backend_vstorage.c | 14 +---
>>>>> src/storage/storage_backend_zfs.c | 47 +++---------
>>>>> src/storage/storage_driver.c | 3 +-
>>>>> src/storage/storage_util.c | 34 +++------
>>>>> src/util/virstoragefile.c | 67 +++++++---------
>>>>> tests/storagepoolxml2argvtest.c | 7 +-
>>>>> tests/storagevolxml2argvtest.c | 6 +-
>>>>> tests/virstoragetest.c | 6 +-
>>>>> 12 files changed, 156 insertions(+), 312 deletions(-)
>>>>>
>>>>> @@ -502,51 +500,40 @@
>>>>> virStorageBackendDiskBuildPool(virStoragePoolObjPtr pool,
>>>>> virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
>>>>> int format = def->source.format;
>>>>> const char *fmt;
>>>>> - bool ok_to_mklabel = false;
>>>>> - int ret = -1;
>>>>> - virCommandPtr cmd = NULL;
>>>>> + VIR_AUTOPTR(virCommand) cmd = NULL;
>>>>>
>>>>> virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE |
>>>>> - VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret);
>>>>> + VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, -1);
>>>>>
>>>>> - VIR_EXCLUSIVE_FLAGS_GOTO(VIR_STORAGE_POOL_BUILD_OVERWRITE,
>>>>> - VIR_STORAGE_POOL_BUILD_NO_OVERWRITE,
>>>>> - error);
>>>>> + VIR_EXCLUSIVE_FLAGS_RET(VIR_STORAGE_POOL_BUILD_OVERWRITE,
>>>>> + VIR_STORAGE_POOL_BUILD_NO_OVERWRITE,
>>>>> + -1);
>>>>>
>>>>> fmt = virStoragePoolFormatDiskTypeToString(format);
>>>>> - if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) {
>>>>> - ok_to_mklabel = true;
>>>>> - } else {
>>>>> - if
>>>>> (virStorageBackendDeviceIsEmpty(def->source.devices[0].path,
>>>>> - fmt, true))
>>>>> - ok_to_mklabel = true;
>>>>> - }
>>>>> + if (!(flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) &&
>>>>> + !(virStorageBackendDeviceIsEmpty(def->source.devices[0].path,
>>>>> + fmt, true)))
>>>>> + return -1;
>>>>>
>>>>> - if (ok_to_mklabel) {
>>>>> - if
>>>>> (virStorageBackendZeroPartitionTable(def->source.devices[0].path,
>>>>> - 1024 * 1024) < 0)
>>>>> - goto error;
>>>>> + if
>>>>> (virStorageBackendZeroPartitionTable(def->source.devices[0].path,
>>>>> + 1024 * 1024) < 0)
>>>>> + return -1;
>>>>>
>>>>> - /* eg parted /dev/sda mklabel --script msdos */
>>>>> - if (format == VIR_STORAGE_POOL_DISK_UNKNOWN)
>>>>> - format = def->source.format = VIR_STORAGE_POOL_DISK_DOS;
>>>>> - if (format == VIR_STORAGE_POOL_DISK_DOS)
>>>>> - fmt = "msdos";
>>>>> - else
>>>>> - fmt = virStoragePoolFormatDiskTypeToString(format);
>>>>> -
>>>>> - cmd = virCommandNewArgList(PARTED,
>>>>> - def->source.devices[0].path,
>>>>> - "mklabel",
>>>>> - "--script",
>>>>> - fmt,
>>>>> - NULL);
>>>>> - ret = virCommandRun(cmd, NULL);
>>>>> - }
>>>>> + /* eg parted /dev/sda mklabel --script msdos */
>>>>> + if (format == VIR_STORAGE_POOL_DISK_UNKNOWN)
>>>>> + format = def->source.format = VIR_STORAGE_POOL_DISK_DOS;
>>>>> + if (format == VIR_STORAGE_POOL_DISK_DOS)
>>>>> + fmt = "msdos";
>>>>> + else
>>>>> + fmt = virStoragePoolFormatDiskTypeToString(format);
>>>>>
>>>>> - error:
>>>>> - virCommandFree(cmd);
>>>>> - return ret;
>>>>> + cmd = virCommandNewArgList(PARTED,
>>>>> + def->source.devices[0].path,
>>>>> + "mklabel",
>>>>> + "--script",
>>>>> + fmt,
>>>>> + NULL);
>>>>> + return virCommandRun(cmd, NULL);
>>>>> }
>>>>
>>>> Apart from the usual mixing of the ret->goto changes with adding
>>>> AUTOFREE, this also removes the 'ok_to_mklabel' bool.
>>>> Those changes really should be separated.
>>>>
>>>
>>> Just so it's clear what's being requested, does this mean taking the
>>> current code and adding the:
>>>
>>> + if (!(flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) &&
>>> + !(virStorageBackendDeviceIsEmpty(def->source.devices[0].path,
>>> + fmt, true)))
>>> + goto error;
>>>
>>> and then reformatting the rest inline as is done here (more or less)?
>>>
>>
>> Yes. Also, in that case we seem to exit without logging an error (as
>> opposed to calling virCommandRun which should log one).
>>
>> Jano
>
>Not sure what was meant about we seem to exit without logging an error.
>If virStorageBackendDeviceIsEmpty an error message is generated if false
>is returned.
False alarm. Looking at the old code it seemed it was possible to fall
through with ok_to_mklabel = false, but the new code makes it more
readable.
>Here's what I have for the difference (attached with any
>luck):
Please use git send-email to post patches, they're easier to apply.
>
>John
>
>From 41861fe512b5d7d71e50601f8d090e55f0293f26 Mon Sep 17 00:00:00 2001
>From: John Ferlan <jferlan@redhat.com>
>Date: Mon, 11 Feb 2019 21:29:29 -0500
>Subject: [PATCH] storage: Rework logic in virStorageBackendDiskBuildPool
>
>Rework the logic to remove the need for the @ok_to_mklabel boolean.
>
>Signed-off-by: John Ferlan <jferlan@redhat.com>
>---
> src/storage/storage_backend_disk.c | 51 ++++++++++++++----------------
> 1 file changed, 23 insertions(+), 28 deletions(-)
>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Jano
>diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c
>index 061c494b7d..abbe1c3e8b 100644
>--- a/src/storage/storage_backend_disk.c
>+++ b/src/storage/storage_backend_disk.c
>@@ -502,7 +502,6 @@ virStorageBackendDiskBuildPool(virStoragePoolObjPtr pool,
> virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
> int format = def->source.format;
> const char *fmt;
>- bool ok_to_mklabel = false;
> int ret = -1;
> virCommandPtr cmd = NULL;
>
>@@ -514,35 +513,31 @@ virStorageBackendDiskBuildPool(virStoragePoolObjPtr pool,
> error);
>
> fmt = virStoragePoolFormatDiskTypeToString(format);
>- if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) {
>- ok_to_mklabel = true;
>- } else {
>- if (virStorageBackendDeviceIsEmpty(def->source.devices[0].path,
>- fmt, true))
>- ok_to_mklabel = true;
>- }
>
>- if (ok_to_mklabel) {
>- if (virStorageBackendZeroPartitionTable(def->source.devices[0].path,
>- 1024 * 1024) < 0)
>- goto error;
>+ if (!(flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) &&
>+ !(virStorageBackendDeviceIsEmpty(def->source.devices[0].path,
>+ fmt, true)))
>+ goto error;
>
>- /* eg parted /dev/sda mklabel --script msdos */
>- if (format == VIR_STORAGE_POOL_DISK_UNKNOWN)
>- format = def->source.format = VIR_STORAGE_POOL_DISK_DOS;
>- if (format == VIR_STORAGE_POOL_DISK_DOS)
>- fmt = "msdos";
>- else
>- fmt = virStoragePoolFormatDiskTypeToString(format);
>-
>- cmd = virCommandNewArgList(PARTED,
>- def->source.devices[0].path,
>- "mklabel",
>- "--script",
>- fmt,
>- NULL);
>- ret = virCommandRun(cmd, NULL);
>- }
>+ if (virStorageBackendZeroPartitionTable(def->source.devices[0].path,
>+ 1024 * 1024) < 0)
>+ goto error;
>+
>+ /* eg parted /dev/sda mklabel --script msdos */
>+ if (format == VIR_STORAGE_POOL_DISK_UNKNOWN)
>+ format = def->source.format = VIR_STORAGE_POOL_DISK_DOS;
>+ if (format == VIR_STORAGE_POOL_DISK_DOS)
>+ fmt = "msdos";
>+ else
>+ fmt = virStoragePoolFormatDiskTypeToString(format);
>+
>+ cmd = virCommandNewArgList(PARTED,
>+ def->source.devices[0].path,
>+ "mklabel",
>+ "--script",
>+ fmt,
>+ NULL);
>+ ret = virCommandRun(cmd, NULL);
>
> error:
> virCommandFree(cmd);
>--
>2.20.1
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.