Remove all use of virRun in storage code

To make it easier to dynamically change the command line ARGV,
switch all storage code over to use virCommandPtr APIs for
running programs
Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>

src/storage/storage_backend.c

@@ -57,7 +57,6 @@
 #include "storage_backend.h"
 #include "logging.h"
 #include "virfile.h"
-#include "command.h"
 
 #if WITH_STORAGE_LVM
 # include "storage_backend_logical.h"
@@ -1418,7 +1417,7 @@ virStorageBackendStablePath(virStoragePoolObjPtr pool,
  */
 int
 virStorageBackendRunProgRegex(virStoragePoolObjPtr pool,
-                              const char *const*prog,
+                              virCommandPtr cmd,
                               int nregex,
                               const char **regex,
                               int *nvars,
@@ -1433,7 +1432,6 @@ virStorageBackendRunProgRegex(virStoragePoolObjPtr pool,
     int maxReg = 0, i, j;
     int totgroups = 0, ngroup = 0, maxvars = 0;
     char **groups;
-    virCommandPtr cmd = NULL;
 
     /* Compile all regular expressions */
     if (VIR_ALLOC_N(reg, nregex) < 0) {
@@ -1470,7 +1468,6 @@ virStorageBackendRunProgRegex(virStoragePoolObjPtr pool,
         goto cleanup;
     }
 
-    cmd = virCommandNewArgs(prog);
     virCommandSetOutputFD(cmd, &fd);
     if (virCommandRunAsync(cmd, NULL) < 0) {
         goto cleanup;
@@ -1541,7 +1538,6 @@ cleanup:
         regfree(&reg[i]);
 
     VIR_FREE(reg);
-    virCommandFree(cmd);
 
     VIR_FORCE_FCLOSE(list);
     VIR_FORCE_CLOSE(fd);
@@ -1562,7 +1558,7 @@ cleanup:
  */
 int
 virStorageBackendRunProgNul(virStoragePoolObjPtr pool,
-                            const char **prog,
+                            virCommandPtr cmd,
                             size_t n_columns,
                             virStorageBackendListVolNulFunc func,
                             void *data)
@@ -1573,7 +1569,6 @@ virStorageBackendRunProgNul(virStoragePoolObjPtr pool,
     char **v;
     int ret = -1;
     int i;
-    virCommandPtr cmd = NULL;
 
     if (n_columns == 0)
         return -1;
@@ -1585,7 +1580,6 @@ virStorageBackendRunProgNul(virStoragePoolObjPtr pool,
     for (i = 0; i < n_columns; i++)
         v[i] = NULL;
 
-    cmd = virCommandNewArgs(prog);
     virCommandSetOutputFD(cmd, &fd);
     if (virCommandRunAsync(cmd, NULL) < 0) {
         goto cleanup;
@@ -1622,9 +1616,9 @@ virStorageBackendRunProgNul(virStoragePoolObjPtr pool,
         }
     }
 
-    if (feof (fp) < 0) {
-        virReportSystemError(errno,
-                             _("read error on pipe to '%s'"), prog[0]);
+    if (feof(fp) < 0) {
+        virReportSystemError(errno, "%s",
+                             _("read error on pipe"));
         goto cleanup;
     }
 
@@ -1633,7 +1627,6 @@ virStorageBackendRunProgNul(virStoragePoolObjPtr pool,
     for (i = 0; i < n_columns; i++)
         VIR_FREE(v[i]);
     VIR_FREE(v);
-    virCommandFree(cmd);
 
     VIR_FORCE_FCLOSE(fp);
     VIR_FORCE_CLOSE(fd);

src/storage/storage_backend.h

@@ -27,6 +27,7 @@
 # include <stdint.h>
 # include "internal.h"
 # include "storage_conf.h"
+# include "command.h"
 
 typedef char * (*virStorageBackendFindPoolSources)(virConnectPtr conn, const char *srcSpec, unsigned int flags);
 typedef int (*virStorageBackendCheckPool)(virConnectPtr conn, virStoragePoolObjPtr pool, bool *active);
@@ -141,7 +142,7 @@ typedef int (*virStorageBackendListVolNulFunc)(virStoragePoolObjPtr pool,
                                                void *data);
 
 int virStorageBackendRunProgRegex(virStoragePoolObjPtr pool,
-                                  const char *const*prog,
+                                  virCommandPtr cmd,
                                   int nregex,
                                   const char **regex,
                                   int *nvars,
@@ -149,7 +150,7 @@ int virStorageBackendRunProgRegex(virStoragePoolObjPtr pool,
                                   void *data, const char *cmd_to_ignore);
 
 int virStorageBackendRunProgNul(virStoragePoolObjPtr pool,
-                                const char **prog,
+                                virCommandPtr cmd,
                                 size_t n_columns,
                                 virStorageBackendListVolNulFunc func,
                                 void *data);

src/storage/storage_backend_disk.c

@@ -269,17 +269,20 @@ virStorageBackendDiskReadPartitions(virStoragePoolObjPtr pool,
      * -              normal   metadata 100027630080 100030242304      2612736
      *
      */
-    const char *prog[] = {
-        PARTHELPER, pool->def->source.devices[0].path, NULL,
-    };
+    virCommandPtr cmd = virCommandNewArgList(PARTHELPER,
+                                             pool->def->source.devices[0].path,
+                                             NULL);
+    int ret;
 
     pool->def->allocation = pool->def->capacity = pool->def->available = 0;
 
-    return virStorageBackendRunProgNul(pool,
-                                       prog,
+    ret = virStorageBackendRunProgNul(pool,
+                                      cmd,
                                       6,
                                       virStorageBackendDiskMakeVol,
                                       vol);
+    virCommandFree(cmd);
+    return ret;
 }
 
 static int
@@ -299,15 +302,19 @@ virStorageBackendDiskMakePoolGeometry(virStoragePoolObjPtr pool,
 static int
 virStorageBackendDiskReadGeometry(virStoragePoolObjPtr pool)
 {
-    const char *prog[] = {
-        PARTHELPER, pool->def->source.devices[0].path, "-g", NULL,
-    };
+    virCommandPtr cmd = virCommandNewArgList(PARTHELPER,
+                                             pool->def->source.devices[0].path,
+                                             "-g",
+                                             NULL);
+    int ret;
 
-    return virStorageBackendRunProgNul(pool,
-                                       prog,
+    ret = virStorageBackendRunProgNul(pool,
+                                      cmd,
                                       3,
                                       virStorageBackendDiskMakePoolGeometry,
                                       NULL);
+    virCommandFree(cmd);
+    return ret;
 }
 
 static int
@@ -379,15 +386,13 @@ virStorageBackendDiskBuildPool(virConnectPtr conn ATTRIBUTE_UNUSED,
     bool ok_to_mklabel = false;
     int ret = -1;
     /* eg parted /dev/sda mklabel msdos */
-    const char *prog[] = {
-        PARTED,
+    virCommandPtr cmd = virCommandNewArgList(PARTED,
                                              pool->def->source.devices[0].path,
                                              "mklabel",
                                              "--script",
                                              ((pool->def->source.format == VIR_STORAGE_POOL_DISK_DOS) ? "msdos" :
                                               virStoragePoolFormatDiskTypeToString(pool->def->source.format)),
-        NULL,
-    };
+                                             NULL);
 
     virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE |
                   VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret);
@@ -419,9 +424,10 @@ virStorageBackendDiskBuildPool(virConnectPtr conn ATTRIBUTE_UNUSED,
     }
 
     if (ok_to_mklabel)
-        ret = virRun(prog, NULL);
+        ret = virCommandRun(cmd, NULL);
 
 error:
+    virCommandFree(cmd);
     return ret;
 }
 
@@ -628,20 +634,13 @@ virStorageBackendDiskCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED,
                                virStorageVolDefPtr vol)
 {
     int res = -1;
-    char *start = NULL;
-    char *end = NULL;
     char *partFormat;
     unsigned long long startOffset = 0, endOffset = 0;
-    const char *cmdargv[] = {
-        PARTED,
+    virCommandPtr cmd = virCommandNewArgList(PARTED,
                                              pool->def->source.devices[0].path,
                                              "mkpart",
                                              "--script",
-        NULL /*partFormat*/,
-        NULL /*start*/,
-        NULL /*end*/,
-        NULL
-    };
+                                             NULL);
 
     if (vol->target.encryption != NULL) {
         virStorageReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -653,7 +652,7 @@ virStorageBackendDiskCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED,
     if (virStorageBackendDiskPartFormat(pool, vol, &partFormat) != 0) {
         return -1;
     }
-    cmdargv[4] = partFormat;
+    virCommandAddArg(cmd, partFormat);
 
     if (virStorageBackendDiskPartBoundries(pool, &startOffset,
                                            &endOffset,
@@ -661,15 +660,10 @@ virStorageBackendDiskCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED,
         goto cleanup;
     }
 
-    if (virAsprintf(&start, "%lluB", startOffset) < 0 ||
-        virAsprintf(&end, "%lluB", endOffset) < 0) {
-        virReportOOMError();
-        goto cleanup;
-    }
-    cmdargv[5] = start;
-    cmdargv[6] = end;
+    virCommandAddArgFormat(cmd, "%lluB", startOffset);
+    virCommandAddArgFormat(cmd, "%lluB", endOffset);
 
-    if (virRun(cmdargv, NULL) < 0)
+    if (virCommandRun(cmd, NULL) < 0)
         goto cleanup;
 
     /* wait for device node to show up */
@@ -690,8 +684,7 @@ virStorageBackendDiskCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED,
 
 cleanup:
     VIR_FREE(partFormat);
-    VIR_FREE(start);
-    VIR_FREE(end);
+    virCommandFree(cmd);
     return res;
 }
 

src/storage/storage_backend_fs.c

@@ -251,10 +251,10 @@ virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSE
             .sources = NULL
         }
     };
-    const char *prog[] = { SHOWMOUNT, "--no-headers", "--exports", NULL, NULL };
     virStoragePoolSourcePtr source = NULL;
     char *retval = NULL;
     unsigned int i;
+    virCommandPtr cmd = NULL;
 
     virCheckFlags(0, NULL);
 
@@ -270,9 +270,14 @@ virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSE
     }
 
     state.host = source->hosts[0].name;
-    prog[3] = source->hosts[0].name;
 
-    if (virStorageBackendRunProgRegex(NULL, prog, 1, regexes, vars,
+    cmd = virCommandNewArgList(SHOWMOUNT,
+                               "--no-headers",
+                               "--exports",
+                               source->hosts[0].name,
+                               NULL);
+
+    if (virStorageBackendRunProgRegex(NULL, cmd, 1, regexes, vars,
                             virStorageBackendFileSystemNetFindPoolSourcesFunc,
                             &state, NULL) < 0)
         goto cleanup;
@@ -289,7 +294,7 @@ virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSE
     VIR_FREE(state.list.sources);
 
     virStoragePoolSourceFree(source);
-
+    virCommandFree(cmd);
     return retval;
 }
 
@@ -337,63 +342,16 @@ virStorageBackendFileSystemIsMounted(virStoragePoolObjPtr pool) {
  */
 static int
 virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) {
-    char *src;
-    const char **mntargv;
-
+    char *src = NULL;
     /* 'mount -t auto' doesn't seem to auto determine nfs (or cifs),
      *  while plain 'mount' does. We have to craft separate argvs to
      *  accommodate this */
-    int netauto = (pool->def->type == VIR_STORAGE_POOL_NETFS &&
+    bool netauto = (pool->def->type == VIR_STORAGE_POOL_NETFS &&
                     pool->def->source.format == VIR_STORAGE_POOL_NETFS_AUTO);
-    int glusterfs = (pool->def->type == VIR_STORAGE_POOL_NETFS &&
+    bool glusterfs = (pool->def->type == VIR_STORAGE_POOL_NETFS &&
                       pool->def->source.format == VIR_STORAGE_POOL_NETFS_GLUSTERFS);
-
-    int source_index;
-
-    const char *netfs_auto_argv[] = {
-        MOUNT,
-        NULL, /* source path */
-        pool->def->target.path,
-        NULL,
-    };
-
-    const char *fs_argv[] =  {
-        MOUNT,
-        "-t",
-        pool->def->type == VIR_STORAGE_POOL_FS ?
-        virStoragePoolFormatFileSystemTypeToString(pool->def->source.format) :
-        virStoragePoolFormatFileSystemNetTypeToString(pool->def->source.format),
-        NULL, /* Fill in shortly - careful not to add extra fields
-                 before this */
-        pool->def->target.path,
-        NULL,
-    };
-
-    const char *glusterfs_argv[] = {
-        MOUNT,
-        "-t",
-        pool->def->type == VIR_STORAGE_POOL_FS ?
-        virStoragePoolFormatFileSystemTypeToString(pool->def->source.format) :
-        virStoragePoolFormatFileSystemNetTypeToString(pool->def->source.format),
-        NULL,
-        "-o",
-        "direct-io-mode=1",
-        pool->def->target.path,
-        NULL,
-    };
-
-    if (netauto) {
-        mntargv = netfs_auto_argv;
-        source_index = 1;
-    } else if (glusterfs) {
-        mntargv = glusterfs_argv;
-        source_index = 3;
-    } else {
-        mntargv = fs_argv;
-        source_index = 3;
-    }
-
-    int ret;
+    virCommandPtr cmd = NULL;
+    int ret = -1;
 
     if (pool->def->type == VIR_STORAGE_POOL_NETFS) {
         if (pool->def->source.nhost != 1) {
@@ -441,14 +399,41 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) {
             return -1;
         }
     }
-    mntargv[source_index] = src;
 
-    if (virRun(mntargv, NULL) < 0) {
-        VIR_FREE(src);
-        return -1;
-    }
+    if (netauto)
+        cmd = virCommandNewArgList(MOUNT,
+                                   src,
+                                   pool->def->target.path,
+                                   NULL);
+    else if (glusterfs)
+        cmd = virCommandNewArgList( MOUNT,
+                                    "-t",
+                                    (pool->def->type == VIR_STORAGE_POOL_FS ?
+                                     virStoragePoolFormatFileSystemTypeToString(pool->def->source.format) :
+                                     virStoragePoolFormatFileSystemNetTypeToString(pool->def->source.format)),
+                                    src,
+                                    "-o",
+                                    "direct-io-mode=1",
+                                    pool->def->target.path,
+                                    NULL);
+    else
+        cmd = virCommandNewArgList(MOUNT,
+                                   "-t",
+                                   (pool->def->type == VIR_STORAGE_POOL_FS ?
+                                    virStoragePoolFormatFileSystemTypeToString(pool->def->source.format) :
+                                    virStoragePoolFormatFileSystemNetTypeToString(pool->def->source.format)),
+                                   src,
+                                   pool->def->target.path,
+                                   NULL);
+
+    if (virCommandRun(cmd, NULL) < 0)
+        goto cleanup;
+
+    ret = 0;
+cleanup:
+    virCommandFree(cmd);
     VIR_FREE(src);
-    return 0;
+    return ret;
 }
 
 /**
@@ -462,8 +447,8 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) {
  */
 static int
 virStorageBackendFileSystemUnmount(virStoragePoolObjPtr pool) {
-    const char *mntargv[3];
-    int ret;
+    virCommandPtr cmd = NULL;
+    int ret = -1;
 
     if (pool->def->type == VIR_STORAGE_POOL_NETFS) {
         if (pool->def->source.nhost != 1) {
@@ -497,14 +482,17 @@ virStorageBackendFileSystemUnmount(virStoragePoolObjPtr pool) {
             return 0;
     }
 
-    mntargv[0] = UMOUNT;
-    mntargv[1] = pool->def->target.path;
-    mntargv[2] = NULL;
+    cmd = virCommandNewArgList(UMOUNT,
+                               pool->def->target.path,
+                               NULL);
 
-    if (virRun(mntargv, NULL) < 0) {
-        return -1;
-    }
-    return 0;
+    if (virCommandRun(cmd, NULL) < 0)
+        goto cleanup;
+
+    ret = 0;
+cleanup:
+    virCommandFree(cmd);
+    return ret;
 }
 #endif /* WITH_STORAGE_FS */
 

src/storage/storage_backend_iscsi.c

@@ -151,31 +151,32 @@ virStorageBackendISCSISession(virStoragePoolObjPtr pool,
     int vars[] = {
         2,
     };
-    const char *const prog[] = {
-        ISCSIADM, "--mode", "session", NULL
-    };
     char *session = NULL;
 
+    virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", "session", NULL);
+
     /* Note that we ignore the exitstatus.  Older versions of iscsiadm tools
      * returned an exit status of > 0, even if they succeeded.  We will just
      * rely on whether session got filled in properly.
      */
     if (virStorageBackendRunProgRegex(pool,
-                                      prog,
+                                      cmd,
                                       1,
                                       regexes,
                                       vars,
                                       virStorageBackendISCSIExtractSession,
                                       &session, NULL) < 0)
-        return NULL;
+        goto cleanup;
 
     if (session == NULL &&
         !probe) {
         virStorageReportError(VIR_ERR_INTERNAL_ERROR,
                               "%s", _("cannot find session"));
-        return NULL;
+        goto cleanup;
     }
 
+cleanup:
+    virCommandFree(cmd);
     return session;
 }
 
@@ -279,42 +280,52 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn,
                                 char **ifacename)
 {
     int ret = -1, exitstatus = -1;
-    char temp_ifacename[32];
-    const char *const cmdargv1[] = {
-        ISCSIADM, "--mode", "iface", "--interface",
-        temp_ifacename, "--op", "new", NULL
-    };
-    const char *const cmdargv2[] = {
-        ISCSIADM, "--mode", "iface", "--interface", temp_ifacename,
-        "--op", "update", "--name", "iface.initiatorname", "--value",
-        initiatoriqn, NULL
-    };
+    char *temp_ifacename;
+    virCommandPtr cmd = NULL;
 
-    snprintf(temp_ifacename, sizeof(temp_ifacename), "libvirt-iface-%08llx",
-             (unsigned long long)virRandomBits(30));
+    if (virAsprintf(&temp_ifacename,
+                    "libvirt-iface-%08llx",
+                    (unsigned long long)virRandomBits(30)) < 0) {
+        virReportOOMError();
+        return -1;
+    }
 
     VIR_DEBUG("Attempting to create interface '%s' with IQN '%s'",
-              &temp_ifacename[0], initiatoriqn);
+              temp_ifacename, initiatoriqn);
 
+    cmd = virCommandNewArgList(ISCSIADM,
+                               "--mode", "iface",
+                               "--interface", temp_ifacename,
+                               "--op", "new",
+                               NULL);
     /* Note that we ignore the exitstatus.  Older versions of iscsiadm
      * tools returned an exit status of > 0, even if they succeeded.
      * We will just rely on whether the interface got created
      * properly. */
-    if (virRun(cmdargv1, &exitstatus) < 0) {
+    if (virCommandRun(cmd, &exitstatus) < 0) {
         virStorageReportError(VIR_ERR_INTERNAL_ERROR,
                               _("Failed to run command '%s' to create new iscsi interface"),
-                              cmdargv1[0]);
-        goto out;
+                              ISCSIADM);
+        goto cleanup;
     }
+    virCommandFree(cmd);
 
+    cmd = virCommandNewArgList(ISCSIADM,
+                               "--mode", "iface",
+                               "--interface", temp_ifacename,
+                               "--op", "update",
+                               "--name", "iface.initiatorname",
+                               "--value",
+                               initiatoriqn,
+                               NULL);
     /* Note that we ignore the exitstatus.  Older versions of iscsiadm tools
      * returned an exit status of > 0, even if they succeeded.  We will just
      * rely on whether iface file got updated properly. */
-    if (virRun(cmdargv2, &exitstatus) < 0) {
+    if (virCommandRun(cmd, &exitstatus) < 0) {
         virStorageReportError(VIR_ERR_INTERNAL_ERROR,
                               _("Failed to run command '%s' to update iscsi interface with IQN '%s'"),
-                              cmdargv2[0], initiatoriqn);
-        goto out;
+                              ISCSIADM, initiatoriqn);
+        goto cleanup;
     }
 
     /* Check again to make sure the interface was created. */
@@ -322,7 +333,7 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn,
         VIR_DEBUG("Failed to find interface '%s' with IQN '%s' "
                   "after attempting to create it",
                   &temp_ifacename[0], initiatoriqn);
-        goto out;
+        goto cleanup;
     } else {
         VIR_DEBUG("Interface '%s' with IQN '%s' was created successfully",
                   *ifacename, initiatoriqn);
@@ -330,7 +341,9 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn,
 
     ret = 0;
 
-out:
+cleanup:
+    virCommandFree(cmd);
+    VIR_FREE(temp_ifacename);
     if (ret != 0)
         VIR_FREE(*ifacename);
     return ret;
@@ -426,14 +439,14 @@ static int
 virStorageBackendISCSIRescanLUNs(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
                                  const char *session)
 {
-    const char *const cmdargv[] = {
-        ISCSIADM, "--mode", "session", "-r", session, "-R", NULL,
-    };
-
-    if (virRun(cmdargv, NULL) < 0)
-        return -1;
-
-    return 0;
+    virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
+                                             "--mode", "session",
+                                             "-r", session,
+                                             "-R",
+                                             NULL);
+    int ret = virCommandRun(cmd, NULL);
+    virCommandFree(cmd);
+    return ret;
 }
 
 struct virStorageBackendISCSITargetList {
@@ -501,24 +514,25 @@ virStorageBackendISCSIScanTargets(const char *portal,
         "^\\s*(\\S+)\\s+(\\S+)\\s*$"
     };
     int vars[] = { 2 };
-    const char *const cmdsendtarget[] = {
-        ISCSIADM, "--mode", "discovery", "--type", "sendtargets",
-        "--portal", portal, NULL
-    };
     struct virStorageBackendISCSITargetList list;
-    int i;
+    size_t i;
+    int ret = -1;
+    virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
+                                             "--mode", "discovery",
+                                             "--type", "sendtargets",
+                                             "--portal", portal,
+                                             NULL);
 
     memset(&list, 0, sizeof(list));
 
     if (virStorageBackendRunProgRegex(NULL, /* No pool for callback */
-                                      cmdsendtarget,
+                                      cmd,
                                       1,
                                       regexes,
                                       vars,
                                       virStorageBackendISCSIGetTargets,
-                                      &list, NULL) < 0) {
-        return -1;
-    }
+                                      &list, NULL) < 0)
+        goto cleanup;
 
     for (i = 0 ; i < list.ntargets ; i++) {
         /* We have to ignore failure, because we can't undo
@@ -542,7 +556,10 @@ virStorageBackendISCSIScanTargets(const char *portal,
         VIR_FREE(list.targets);
     }
 
-    return 0;
+    ret = 0;
+cleanup:
+    virCommandFree(cmd);
+    return ret;
 }
 
 

src/storage/storage_backend_logical.c

@@ -48,17 +48,16 @@ static int
 virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool,
                                   int on)
 {
-    const char *cmdargv[4];
-
-    cmdargv[0] = VGCHANGE;
-    cmdargv[1] = on ? "-aly" : "-aln";
-    cmdargv[2] = pool->def->source.name;
-    cmdargv[3] = NULL;
-
-    if (virRun(cmdargv, NULL) < 0)
-        return -1;
-
-    return 0;
+    int ret;
+    virCommandPtr cmd =
+        virCommandNewArgList(VGCHANGE,
+                             on ? "-aly" : "-aln",
+                             pool->def->source.name,
+                             NULL);
+
+    ret = virCommandRun(cmd, NULL);
+    virCommandFree(cmd);
+    return ret;
 }
 
 
@@ -296,24 +295,31 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool,
     int vars[] = {
         8
     };
-    const char *prog[] = {
-        LVS, "--separator", "#", "--noheadings", "--units", "b",
-        "--unbuffered", "--nosuffix", "--options",
-        "lv_name,origin,uuid,devices,segtype,stripes,seg_size,vg_extent_size",
-        pool->def->source.name, NULL
-    };
-
+    int ret = -1;
+    virCommandPtr cmd;
+
+    cmd = virCommandNewArgList(LVS,
+                               "--separator", "#",
+                               "--noheadings",
+                               "--units", "b",
+                               "--unbuffered",
+                               "--nosuffix",
+                               "--options", "lv_name,origin,uuid,devices,segtype,stripes,seg_size,vg_extent_size",
+                               pool->def->source.name,
+                               NULL);
     if (virStorageBackendRunProgRegex(pool,
-                                      prog,
+                                      cmd,
                                       1,
                                       regexes,
                                       vars,
                                       virStorageBackendLogicalMakeVol,
-                                      vol, "lvs") < 0) {
-        return -1;
-    }
+                                      vol, "lvs") < 0)
+        goto cleanup;
 
-    return 0;
+    ret = 0;
+cleanup:
+    virCommandFree(cmd);
+    return ret;
 }
 
 static int
@@ -405,8 +411,7 @@ virStorageBackendLogicalFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED,
     int vars[] = {
         2
     };
-    const char *const prog[] = { PVS, "--noheadings", "-o", "pv_name,vg_name", NULL };
-    const char *const scanprog[] = { VGSCAN, NULL };
+    virCommandPtr cmd;
     char *retval = NULL;
     virStoragePoolSourceList sourceList;
     int i;
@@ -418,17 +423,25 @@ virStorageBackendLogicalFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED,
      * that might be hanging around, so if this fails for some reason, the
      * worst that happens is that scanning doesn't pick everything up
      */
-    if (virRun(scanprog, NULL) < 0) {
+    cmd = virCommandNew(VGSCAN);
+    if (virCommandRun(cmd, NULL) < 0)
         VIR_WARN("Failure when running vgscan to refresh physical volumes");
-    }
+    virCommandFree(cmd);
 
     memset(&sourceList, 0, sizeof(sourceList));
     sourceList.type = VIR_STORAGE_POOL_LOGICAL;
 
-    if (virStorageBackendRunProgRegex(NULL, prog, 1, regexes, vars,
+    cmd = virCommandNewArgList(PVS,
+                               "--noheadings",
+                               "-o", "pv_name,vg_name",
+                               NULL);
+    if (virStorageBackendRunProgRegex(NULL, cmd, 1, regexes, vars,
                                       virStorageBackendLogicalFindPoolSourcesFunc,
-                                &sourceList, "pvs") < 0)
+                                      &sourceList, "pvs") < 0) {
+        virCommandFree(cmd);
         return NULL;
+    }
+    virCommandFree(cmd);
 
     retval = virStoragePoolSourceListFormat(&sourceList);
     if (retval == NULL) {
@@ -483,26 +496,20 @@ virStorageBackendLogicalBuildPool(virConnectPtr conn ATTRIBUTE_UNUSED,
                                   virStoragePoolObjPtr pool,
                                   unsigned int flags)
 {
-    const char **vgargv;
-    const char *pvargv[3];
-    int n = 0, i, fd;
+    virCommandPtr vgcmd;
+    int fd;
     char zeros[PV_BLANK_SECTOR_SIZE];
+    int ret = -1;
+    size_t i;
 
     virCheckFlags(0, -1);
 
     memset(zeros, 0, sizeof(zeros));
 
-    if (VIR_ALLOC_N(vgargv, 3 + pool->def->source.ndevice) < 0) {
-        virReportOOMError();
-        return -1;
-    }
-
-    vgargv[n++] = VGCREATE;
-    vgargv[n++] = pool->def->source.name;
+    vgcmd = virCommandNewArgList(VGCREATE, pool->def->source.name, NULL);
 
-    pvargv[0] = PVCREATE;
-    pvargv[2] = NULL;
     for (i = 0 ; i < pool->def->source.ndevice ; i++) {
+        virCommandPtr pvcmd;
         /*
          * LVM requires that the first sector is blanked if using
          * a whole disk as a PV. So we just blank them out regardless
@@ -539,25 +546,27 @@ virStorageBackendLogicalBuildPool(virConnectPtr conn ATTRIBUTE_UNUSED,
          * Initialize the physical volume because vgcreate is not
          * clever enough todo this for us :-(
          */
-        vgargv[n++] = pool->def->source.devices[i].path;
-        pvargv[1] = pool->def->source.devices[i].path;
-        if (virRun(pvargv, NULL) < 0)
+        pvcmd = virCommandNewArgList(PVCREATE,
+                                     pool->def->source.devices[i].path,
+                                     NULL);
+        if (virCommandRun(pvcmd, NULL) < 0) {
+            virCommandFree(pvcmd);
             goto cleanup;
         }
+        virCommandFree(pvcmd);
 
-    vgargv[n] = NULL;
+        virCommandAddArg(vgcmd, pool->def->source.devices[i].path);
+    }
 
     /* Now create the volume group itself */
-    if (virRun(vgargv, NULL) < 0)
+    if (virCommandRun(vgcmd, NULL) < 0)
         goto cleanup;
 
-    VIR_FREE(vgargv);
-
-    return 0;
+    ret = 0;
 
- cleanup:
-    VIR_FREE(vgargv);
-    return -1;
+cleanup:
+    virCommandFree(vgcmd);
+    return ret;
 }
 
 
@@ -579,33 +588,42 @@ virStorageBackendLogicalRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED,
     int vars[] = {
         2
     };
-    const char *prog[] = {
-        VGS, "--separator", ":", "--noheadings", "--units", "b", "--unbuffered",
-        "--nosuffix", "--options", "vg_size,vg_free",
-        pool->def->source.name, NULL
-    };
+    virCommandPtr cmd = NULL;
+    int ret = -1;
 
     virFileWaitForDevices();
 
     /* Get list of all logical volumes */
-    if (virStorageBackendLogicalFindLVs(pool, NULL) < 0) {
-        virStoragePoolObjClearVols(pool);
-        return -1;
-    }
+    if (virStorageBackendLogicalFindLVs(pool, NULL) < 0)
+        goto cleanup;
+
+    cmd = virCommandNewArgList(VGS,
+                               "--separator", ":",
+                               "--noheadings",
+                               "--units", "b",
+                               "--unbuffered",
+                               "--nosuffix",
+                               "--options", "vg_size,vg_free",
+                               pool->def->source.name,
+                               NULL);
 
     /* Now get basic volgrp metadata */
     if (virStorageBackendRunProgRegex(pool,
-                                      prog,
+                                      cmd,
                                       1,
                                       regexes,
                                       vars,
                                       virStorageBackendLogicalRefreshPoolFunc,
-                                      NULL, "vgs") < 0) {
-        virStoragePoolObjClearVols(pool);
-        return -1;
-    }
+                                      NULL, "vgs") < 0)
+        goto cleanup;
 
-    return 0;
+    ret = 0;
+
+cleanup:
+    virCommandFree(cmd);
+    if (ret < 0)
+        virStoragePoolObjClearVols(pool);
+    return ret;
 }
 
 /*
@@ -628,31 +646,37 @@ virStorageBackendLogicalDeletePool(virConnectPtr conn ATTRIBUTE_UNUSED,
                                    virStoragePoolObjPtr pool,
                                    unsigned int flags)
 {
-    const char *cmdargv[] = {
-        VGREMOVE, "-f", pool->def->source.name, NULL
-    };
-    const char *pvargv[3];
-    int i, error;
+    virCommandPtr cmd = NULL;
+    size_t i;
+    int ret = -1;
 
     virCheckFlags(0, -1);
 
     /* first remove the volume group */
-    if (virRun(cmdargv, NULL) < 0)
-        return -1;
+    cmd = virCommandNewArgList(VGREMOVE,
+                               "-f", pool->def->source.name,
+                               NULL);
+    if (virCommandRun(cmd, NULL) < 0)
+        goto cleanup;
+    virCommandFree(cmd);
 
     /* now remove the pv devices and clear them out */
-    error = 0;
-    pvargv[0] = PVREMOVE;
-    pvargv[2] = NULL;
+    ret = 0;
     for (i = 0 ; i < pool->def->source.ndevice ; i++) {
-        pvargv[1] = pool->def->source.devices[i].path;
-        if (virRun(pvargv, NULL) < 0) {
-            error = -1;
+        cmd = virCommandNewArgList(PVREMOVE,
+                                   pool->def->source.devices[i].path,
+                                   NULL);
+        if (virCommandRun(cmd, NULL) < 0) {
+            ret = -1;
             break;
         }
+        virCommandFree(cmd);
+        cmd = NULL;
     }
 
-    return error;
+cleanup:
+    virCommandFree(cmd);
+    return ret;
 }
 
 
@@ -669,16 +693,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
                                   virStorageVolDefPtr vol)
 {
     int fdret, fd = -1;
-    char size[100];
-    const char *cmdargvnew[] = {
-        LVCREATE, "--name", vol->name, "-L", size,
-        pool->def->source.name, NULL
-    };
-    const char *cmdargvsnap[] = {
-        LVCREATE, "--name", vol->name, "-L", size,
-        "-s", vol->backingStore.path, NULL
-    };
-    const char **cmdargv = cmdargvnew;
+    virCommandPtr cmd = NULL;
 
     if (vol->target.encryption != NULL) {
         virStorageReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -687,15 +702,6 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
         return -1;
     }
 
-    if (vol->backingStore.path) {
-        cmdargv = cmdargvsnap;
-    }
-
-    snprintf(size, sizeof(size)-1, "%lluK", VIR_DIV_UP(vol->capacity, 1024));
-    size[sizeof(size)-1] = '\0';
-
-    vol->type = VIR_STORAGE_VOL_BLOCK;
-
     if (vol->target.path != NULL) {
         /* A target path passed to CreateVol has no meaning */
         VIR_FREE(vol->target.path);
@@ -708,8 +714,18 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
         return -1;
     }
 
-    if (virRun(cmdargv, NULL) < 0)
-        return -1;
+    cmd = virCommandNewArgList(LVCREATE,
+                               "--name", vol->name,
+                               NULL);
+    virCommandAddArg(cmd, "-L");
+    virCommandAddArgFormat(cmd, "%lluK", VIR_DIV_UP(vol->capacity, 1024));
+    if (vol->backingStore.path)
+        virCommandAddArgPair(cmd, "-s", vol->backingStore.path);
+    else
+        virCommandAddArg(cmd, pool->def->source.name);
+
+    if (virCommandRun(cmd, NULL) < 0)
+        goto cleanup;
 
     if ((fdret = virStorageBackendVolOpen(vol->target.path)) < 0)
         goto cleanup;
@@ -752,6 +768,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
  cleanup:
     VIR_FORCE_CLOSE(fd);
     virStorageBackendLogicalDeleteVol(conn, pool, vol, 0);
+    virCommandFree(cmd);
     return -1;
 }