https://bugzilla.redhat.com/show_bug.cgi?id=830056 Add flags handling to the virStoragePoolCreate and virStoragePoolCreateXML API's which will allow the caller to provide the capability for the storage pool create API's to also perform a pool build during creation rather than requiring the additional buildPool step. This will allow transient pools to be defined, built, and started. The new flags are: * VIR_STORAGE_POOL_CREATE_WITH_BUILD Perform buildPool without any flags passed. * VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE Perform buildPool using VIR_STORAGE_POOL_BUILD_OVERWRITE flag. * VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE Perform buildPool using VIR_STORAGE_POOL_BUILD_NO_OVERWRITE flag. It is up to the backend to handle the processing of build flags. The overwrite and no-overwrite flags are mutually exclusive. NB: This patch is loosely based upon code originally authored by Osier Yang that were not reviewed and pushed, see: https://www.redhat.com/archives/libvir-list/2012-July/msg01328.html Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- include/libvirt/libvirt-storage.h | 16 ++++++++++++++- src/libvirt-storage.c | 4 ++-- src/storage/storage_driver.c | 42 +++++++++++++++++++++++++++++++++++++-- 3 files changed, 57 insertions(+), 5 deletions(-) diff --git a/include/libvirt/libvirt-storage.h b/include/libvirt/libvirt-storage.h index 9fc3c2d..996a726 100644 --- a/include/libvirt/libvirt-storage.h +++ b/include/libvirt/libvirt-storage.h @@ -57,7 +57,6 @@ typedef enum { # endif } virStoragePoolState; - typedef enum { VIR_STORAGE_POOL_BUILD_NEW = 0, /* Regular build from scratch */ VIR_STORAGE_POOL_BUILD_REPAIR = (1 << 0), /* Repair / reinitialize */ @@ -71,6 +70,21 @@ typedef enum { VIR_STORAGE_POOL_DELETE_ZEROED = 1 << 0, /* Clear all data to zeros (slow) */ } virStoragePoolDeleteFlags; +typedef enum { + VIR_STORAGE_POOL_CREATE_NORMAL = 0, + + /* Create the pool and perform pool build without any flags */ + VIR_STORAGE_POOL_CREATE_WITH_BUILD = 1 << 0, + + /* Create the pool and perform pool build using the + * VIR_STORAGE_POOL_BUILD_OVERWRITE flag */ + VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE = 1 << 1, + + /* Create the pool and perform pool build using the + * VIR_STORAGE_POOL_BUILD_NO_OVERWRITE flag */ + VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE = 1 << 2, +} virStoragePoolCreateFlags; + typedef struct _virStoragePoolInfo virStoragePoolInfo; struct _virStoragePoolInfo { diff --git a/src/libvirt-storage.c b/src/libvirt-storage.c index 66dd9f0..238a6cd 100644 --- a/src/libvirt-storage.c +++ b/src/libvirt-storage.c @@ -505,7 +505,7 @@ virStoragePoolLookupByVolume(virStorageVolPtr vol) * virStoragePoolCreateXML: * @conn: pointer to hypervisor connection * @xmlDesc: XML description for new pool - * @flags: extra flags; not used yet, so callers should always pass 0 + * @flags: bitwise-OR of virStoragePoolCreateFlags * * Create a new storage based on its XML description. The * pool is not persistent, so its definition will disappear @@ -670,7 +670,7 @@ virStoragePoolUndefine(virStoragePoolPtr pool) /** * virStoragePoolCreate: * @pool: pointer to storage pool - * @flags: extra flags; not used yet, so callers should always pass 0 + * @flags: bitwise-OR of virStoragePoolCreateFlags * * Starts an inactive storage pool * diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index bbf21f6..59a18bf 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -671,8 +671,11 @@ storagePoolCreateXML(virConnectPtr conn, virStoragePoolPtr ret = NULL; virStorageBackendPtr backend; char *stateFile = NULL; + unsigned int build_flags = 0; - virCheckFlags(0, NULL); + virCheckFlags(VIR_STORAGE_POOL_CREATE_WITH_BUILD | + VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE | + VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE, NULL); storageDriverLock(); if (!(def = virStoragePoolDefParseString(xml))) @@ -694,6 +697,22 @@ storagePoolCreateXML(virConnectPtr conn, goto cleanup; def = NULL; + if (backend->buildPool) { + if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE) + build_flags |= VIR_STORAGE_POOL_BUILD_OVERWRITE; + else if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE) + build_flags |= VIR_STORAGE_POOL_BUILD_NO_OVERWRITE; + + if (build_flags || + (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD)) { + if (backend->buildPool(conn, pool, build_flags) < 0) { + virStoragePoolObjRemove(&driver->pools, pool); + pool = NULL; + goto cleanup; + } + } + } + if (backend->startPool && backend->startPool(conn, pool) < 0) { virStoragePoolObjRemove(&driver->pools, pool); @@ -845,8 +864,11 @@ storagePoolCreate(virStoragePoolPtr obj, virStorageBackendPtr backend; int ret = -1; char *stateFile = NULL; + unsigned int build_flags = 0; - virCheckFlags(0, -1); + virCheckFlags(VIR_STORAGE_POOL_CREATE_WITH_BUILD | + VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE | + VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE, NULL); if (!(pool = virStoragePoolObjFromStoragePool(obj))) return -1; @@ -864,6 +886,22 @@ storagePoolCreate(virStoragePoolPtr obj, goto cleanup; } + if (backend->buildPool) { + if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE) + build_flags |= VIR_STORAGE_POOL_BUILD_OVERWRITE; + else if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE) + build_flags |= VIR_STORAGE_POOL_BUILD_NO_OVERWRITE; + + if (build_flags || + (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD)) { + if (backend->buildPool(obj->conn, pool, build_flags) < 0) { + virStoragePoolObjRemove(&driver->pools, pool); + pool = NULL; + goto cleanup; + } + } + } + VIR_INFO("Starting up storage pool '%s'", pool->def->name); if (backend->startPool && backend->startPool(obj->conn, pool) < 0) -- 2.5.0

On 12/16/2015 08:18 AM, Michal Privoznik wrote: The disk and fs backends do check as well, but I can add it here. Right - if you want to build prior to start, then use one of the flags. Leaving it up to the backend buildPool function to decide how to handle the overwrite when that option isn't provided. Ah right - dang that cut-n-paste... Sure. tks - John

On 25.11.2015 20:11, John Ferlan wrote: Nice. ACK Should we do something similar to domain, network, ... in the rest of virsh? Michal

Although they both are the same now, a future patch will add new options to pool-create-as. So create a common macro to capture commonality, then use that in the command specific structure. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- tools/virsh-pool.c | 151 ++++++++++++++++++++++++++++------------------------- 1 file changed, 79 insertions(+), 72 deletions(-) diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index ff77039..cb91cd3 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -61,6 +61,74 @@ .help = N_("overwrite any existing data") \ }, \ +#define OPTS_POOL_COMMON_X_AS \ + {.name = "name", \ + .type = VSH_OT_DATA, \ + .flags = VSH_OFLAG_REQ, \ + .help = N_("name of the pool") \ + }, \ + {.name = "type", \ + .type = VSH_OT_DATA, \ + .flags = VSH_OFLAG_REQ, \ + .help = N_("type of the pool") \ + }, \ + {.name = "print-xml", \ + .type = VSH_OT_BOOL, \ + .help = N_("print XML document, but don't define/create") \ + }, \ + {.name = "source-host", \ + .type = VSH_OT_STRING, \ + .help = N_("source-host for underlying storage") \ + }, \ + {.name = "source-path", \ + .type = VSH_OT_STRING, \ + .help = N_("source path for underlying storage") \ + }, \ + {.name = "source-dev", \ + .type = VSH_OT_STRING, \ + .help = N_("source device for underlying storage") \ + }, \ + {.name = "source-name", \ + .type = VSH_OT_STRING, \ + .help = N_("source name for underlying storage") \ + }, \ + {.name = "target", \ + .type = VSH_OT_STRING, \ + .help = N_("target for underlying storage") \ + }, \ + {.name = "source-format", \ + .type = VSH_OT_STRING, \ + .help = N_("format for underlying storage") \ + }, \ + {.name = "auth-type", \ + .type = VSH_OT_STRING, \ + .help = N_("auth type to be used for underlying storage") \ + }, \ + {.name = "auth-username", \ + .type = VSH_OT_STRING, \ + .help = N_("auth username to be used for underlying storage") \ + }, \ + {.name = "secret-usage", \ + .type = VSH_OT_STRING, \ + .help = N_("auth secret usage to be used for underlying storage") \ + }, \ + {.name = "adapter-name", \ + .type = VSH_OT_STRING, \ + .help = N_("adapter name to be used for underlying storage") \ + }, \ + {.name = "adapter-wwnn", \ + .type = VSH_OT_STRING, \ + .help = N_("adapter wwnn to be used for underlying storage") \ + }, \ + {.name = "adapter-wwpn", \ + .type = VSH_OT_STRING, \ + .help = N_("adapter wwpn to be used for underlying storage") \ + }, \ + {.name = "adapter-parent", \ + .type = VSH_OT_STRING, \ + .help = N_("adapter parent to be used for underlying storage") \ + }, \ + virStoragePoolPtr virshCommandOptPoolBy(vshControl *ctl, const vshCmd *cmd, const char *optname, const char **name, unsigned int flags) @@ -200,76 +268,9 @@ cmdPoolCreate(vshControl *ctl, const vshCmd *cmd) return ret; } -/* - * XML Building helper for pool-define-as and pool-create-as - */ -static const vshCmdOptDef opts_pool_X_as[] = { - {.name = "name", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("name of the pool") - }, - {.name = "type", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, - .help = N_("type of the pool") - }, - {.name = "print-xml", - .type = VSH_OT_BOOL, - .help = N_("print XML document, but don't define/create") - }, - {.name = "source-host", - .type = VSH_OT_STRING, - .help = N_("source-host for underlying storage") - }, - {.name = "source-path", - .type = VSH_OT_STRING, - .help = N_("source path for underlying storage") - }, - {.name = "source-dev", - .type = VSH_OT_STRING, - .help = N_("source device for underlying storage") - }, - {.name = "source-name", - .type = VSH_OT_STRING, - .help = N_("source name for underlying storage") - }, - {.name = "target", - .type = VSH_OT_STRING, - .help = N_("target for underlying storage") - }, - {.name = "source-format", - .type = VSH_OT_STRING, - .help = N_("format for underlying storage") - }, - {.name = "auth-type", - .type = VSH_OT_STRING, - .help = N_("auth type to be used for underlying storage") - }, - {.name = "auth-username", - .type = VSH_OT_STRING, - .help = N_("auth username to be used for underlying storage") - }, - {.name = "secret-usage", - .type = VSH_OT_STRING, - .help = N_("auth secret usage to be used for underlying storage") - }, - {.name = "adapter-name", - .type = VSH_OT_STRING, - .help = N_("adapter name to be used for underlying storage") - }, - {.name = "adapter-wwnn", - .type = VSH_OT_STRING, - .help = N_("adapter wwnn to be used for underlying storage") - }, - {.name = "adapter-wwpn", - .type = VSH_OT_STRING, - .help = N_("adapter wwpn to be used for underlying storage") - }, - {.name = "adapter-parent", - .type = VSH_OT_STRING, - .help = N_("adapter parent to be used for underlying storage") - }, +static const vshCmdOptDef opts_pool_define_as[] = { + OPTS_POOL_COMMON_X_AS + {.name = NULL} }; @@ -383,6 +384,12 @@ static const vshCmdInfo info_pool_create_as[] = { {.name = NULL} }; +static const vshCmdOptDef opts_pool_create_as[] = { + OPTS_POOL_COMMON_X_AS + + {.name = NULL} +}; + static bool cmdPoolCreateAs(vshControl *ctl, const vshCmd *cmd) { @@ -1850,7 +1857,7 @@ const vshCmdDef storagePoolCmds[] = { }, {.name = "pool-create-as", .handler = cmdPoolCreateAs, - .opts = opts_pool_X_as, + .opts = opts_pool_create_as, .info = info_pool_create_as, .flags = 0 }, @@ -1862,7 +1869,7 @@ const vshCmdDef storagePoolCmds[] = { }, {.name = "pool-define-as", .handler = cmdPoolDefineAs, - .opts = opts_pool_X_as, + .opts = opts_pool_define_as, .info = info_pool_define_as, .flags = 0 }, -- 2.5.0

https://bugzilla.redhat.com/show_bug.cgi?id=830056 Utilize recently added VIR_STORAGE_POOL_CREATE_WITH_BUILD* flags in order to pass the flags along to the virStoragePoolCreateXML and virStoragePoolCreate API's. This affects the 'virsh pool-create', 'virsh pool-create-as', and 'virsh pool-start' commands. While it could be argued that pool-start doesn't need the flags, they could prove useful for someone trying to do one command build --overwrite and start command processing or essentially starting with a clean slate. NB: This patch is loosely based upon code originally authored by Osier Yang that were not reviewed and pushed, see: https://www.redhat.com/archives/libvir-list/2012-July/msg00497.html Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- tools/virsh-pool.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++--- tools/virsh.pod | 25 ++++++++++++++++++- 2 files changed, 94 insertions(+), 4 deletions(-) diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index cb91cd3..1bb987d 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -47,6 +47,13 @@ .help = N_("file containing an XML pool description") \ }, \ +#define OPT_BUILD_COMMON \ + {.name = "build", \ + .type = VSH_OT_BOOL, \ + .flags = 0, \ + .help = N_("build the pool as normal") \ + }, \ + #define OPT_NO_OVERWRITE_COMMON \ {.name = "no-overwrite", \ .type = VSH_OT_BOOL, \ @@ -235,6 +242,9 @@ static const vshCmdInfo info_pool_create[] = { static const vshCmdOptDef opts_pool_create[] = { OPT_FILE_COMMON + OPT_BUILD_COMMON + OPT_NO_OVERWRITE_COMMON + OPT_OVERWRITE_COMMON {.name = NULL} }; @@ -246,15 +256,32 @@ cmdPoolCreate(vshControl *ctl, const vshCmd *cmd) const char *from = NULL; bool ret = true; char *buffer; + bool build; + bool overwrite; + bool no_overwrite; + unsigned int flags = 0; virshControlPtr priv = ctl->privData; if (vshCommandOptStringReq(ctl, cmd, "file", &from) < 0) return false; + build = vshCommandOptBool(cmd, "build"); + overwrite = vshCommandOptBool(cmd, "overwrite"); + no_overwrite = vshCommandOptBool(cmd, "no-overwrite"); + + VSH_EXCLUSIVE_OPTIONS_VAR(overwrite, no_overwrite); + + if (build) + flags |= VIR_STORAGE_POOL_CREATE_WITH_BUILD; + if (overwrite) + flags |= VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE; + if (no_overwrite) + flags |= VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE; + if (virFileReadAll(from, VSH_MAX_XML_FILE, &buffer) < 0) return false; - pool = virStoragePoolCreateXML(priv->conn, buffer, 0); + pool = virStoragePoolCreateXML(priv->conn, buffer, flags); VIR_FREE(buffer); if (pool != NULL) { @@ -386,6 +413,9 @@ static const vshCmdInfo info_pool_create_as[] = { static const vshCmdOptDef opts_pool_create_as[] = { OPTS_POOL_COMMON_X_AS + OPT_BUILD_COMMON + OPT_NO_OVERWRITE_COMMON + OPT_OVERWRITE_COMMON {.name = NULL} }; @@ -397,8 +427,25 @@ cmdPoolCreateAs(vshControl *ctl, const vshCmd *cmd) const char *name; char *xml; bool printXML = vshCommandOptBool(cmd, "print-xml"); + bool build; + bool overwrite; + bool no_overwrite; + unsigned int flags = 0; virshControlPtr priv = ctl->privData; + build = vshCommandOptBool(cmd, "build"); + overwrite = vshCommandOptBool(cmd, "overwrite"); + no_overwrite = vshCommandOptBool(cmd, "no-overwrite"); + + VSH_EXCLUSIVE_OPTIONS_VAR(overwrite, no_overwrite); + + if (build) + flags |= VIR_STORAGE_POOL_CREATE_WITH_BUILD; + if (overwrite) + flags |= VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE; + if (no_overwrite) + flags |= VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE; + if (!virshBuildPoolXML(ctl, cmd, &name, &xml)) return false; @@ -406,7 +453,7 @@ cmdPoolCreateAs(vshControl *ctl, const vshCmd *cmd) vshPrint(ctl, "%s", xml); VIR_FREE(xml); } else { - pool = virStoragePoolCreateXML(priv->conn, xml, 0); + pool = virStoragePoolCreateXML(priv->conn, xml, flags); VIR_FREE(xml); if (pool != NULL) { @@ -1657,6 +1704,9 @@ static const vshCmdInfo info_pool_start[] = { static const vshCmdOptDef opts_pool_start[] = { OPT_POOL_COMMON + OPT_BUILD_COMMON + OPT_NO_OVERWRITE_COMMON + OPT_OVERWRITE_COMMON {.name = NULL} }; @@ -1667,11 +1717,28 @@ cmdPoolStart(vshControl *ctl, const vshCmd *cmd) virStoragePoolPtr pool; bool ret = true; const char *name = NULL; + bool build; + bool overwrite; + bool no_overwrite; + unsigned int flags = 0; if (!(pool = virshCommandOptPool(ctl, cmd, "pool", &name))) return false; - if (virStoragePoolCreate(pool, 0) == 0) { + build = vshCommandOptBool(cmd, "build"); + overwrite = vshCommandOptBool(cmd, "overwrite"); + no_overwrite = vshCommandOptBool(cmd, "no-overwrite"); + + VSH_EXCLUSIVE_OPTIONS_VAR(overwrite, no_overwrite); + + if (build) + flags |= VIR_STORAGE_POOL_CREATE_WITH_BUILD; + if (overwrite) + flags |= VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE; + if (no_overwrite) + flags |= VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE; + + if (virStoragePoolCreate(pool, flags) == 0) { vshPrint(ctl, _("Pool %s started\n"), name); } else { vshError(ctl, _("Failed to start pool %s"), name); diff --git a/tools/virsh.pod b/tools/virsh.pod index 21ae4a3..107fc32 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -3181,15 +3181,23 @@ specified or it's been determined that the disk doesn't already have one, by the pool source format type or "dos" if not specified for the pool. =item B<pool-create> I<file> +[I<--build>] [[I<--overwrite>] | [I<--no-overwrite>]] Create and start a pool object from the XML I<file>. +[I<--build>] [[I<--overwrite>] | [I<--no-overwrite>]] perform a +B<pool-build> after creation in order to remove the need for a +follow-up command to build the pool. The I<--overwrite> and +I<--no-overwrite> flags follow the same rules as B<pool-build>. If +just I<--build> is provided, then B<pool-build> is called with no flags. + =item B<pool-create-as> I<name> I<type> [I<--print-xml>] [I<--source-host hostname>] [I<--source-path path>] [I<--source-dev path>] [I<--source-name name>] [I<--target path>] [I<--source-format format>] [I<--auth-type authtype> I<--auth-username username> I<--secret-usage usage>] [[I<--adapter-name name>] | [I<--adapter-wwnn> I<--adapter-wwpn>] [I<--adapter-parent parent>]] +[I<--build>] [[I<--overwrite>] | [I<--no-overwrite>]] Create and start a pool object I<name> from the raw parameters. If @@ -3233,6 +3241,12 @@ the wwnn and wwpn to be used for the fc_host adapter type pool. The parent optionally provides the name of the scsi_hostN node device to be used for the vHBA. +[I<--build>] [[I<--overwrite>] | [I<--no-overwrite>]] perform a +B<pool-build> after creation in order to remove the need for a +follow-up command to build the pool. The I<--overwrite> and +I<--no-overwrite> flags follow the same rules as B<pool-build>. If +just I<--build> is provided, then B<pool-build> is called with no flags. + =item B<pool-define> I<file> Define an inactive persistent storage pool or modify an existing persistent one @@ -3250,7 +3264,8 @@ I<--print-xml> is specified, then print the XML of the pool object without defining the pool. Otherwise, the pool has the specified I<type>. -Use the same arguments as B<pool-create-as>. +Use the same arguments as B<pool-create-as>, except for the I<--build>, +I<--overwrite>, and I<--no-overwrite> options. =item B<pool-destroy> I<pool-or-uuid> @@ -3326,9 +3341,17 @@ Convert the I<uuid> to a pool name. Refresh the list of volumes contained in I<pool>. =item B<pool-start> I<pool-or-uuid> +[I<--build>] [[I<--overwrite>] | [I<--no-overwrite>]] Start the storage I<pool>, which is previously defined but inactive. +[I<--build>] [[I<--overwrite>] | [I<--no-overwrite>]] perform a +B<pool-build> prior to B<pool-start> to ensure the pool environment is +in an expected state rather than needing to run the build command prior +to startup. The I<--overwrite> and I<--no-overwrite> flags follow the +same rules as B<pool-build>. If just I<--build> is provided, then +B<pool-build> is called with no flags. + B<Note>: A storage pool that relies on remote resources such as an "iscsi" or a (v)HBA backed "scsi" pool may need to be refreshed multiple times in order to have all the volumes detected (see B<pool-refresh>). -- 2.5.0

On Wed, Nov 25, 2015 at 14:11:08 -0500, John Ferlan wrote: These look terrible without commas between entries. This should be used only if the name of the variable matches the name of the option flag since the variable name is used in the error message in place of the option flag name. See above. Peter