On 08/17/2012 08:58 AM, Osier Yang wrote:
> New options --build, --build-overwrite, and --build-no-overwrite
> are added to commands pool-create/pool-create-as/pool-start.
> Perhaps it's not that necessary to allow pool building for pool-start,
> but it doesn't hurt to have them.
>
> +++ b/tools/virsh-pool.c
> @@ -124,6 +124,10 @@ static const vshCmdInfo info_pool_create[] = {
> static const vshCmdOptDef opts_pool_create[] = {
> {"file", VSH_OT_DATA, VSH_OFLAG_REQ,
> N_("file containing an XML pool description")},
> + {"build", VSH_OT_BOOL, 0, N_("build the pool as normal")},
> + {"build-overwrite", VSH_OT_BOOL, 0, N_("build the pool without overwriting the "
> + "existed pool data")},
s/existed/existing/
> + {"build-no-overwrite", VSH_OT_BOOL, 0, N_("build the pool with overwriting anything")},
Aren't those two descriptions backwards?
> + if (build + build_overwrite + build_no_overwrite > 1) {
> + vshError(ctl, _("build, build-overwrite, and build-no-overwrite must "
> + "be sepcified exclusively"));
s/sepcified/specified/
If they are mutually exclusive, it might be better to enforce that
exclusivity at the src/libvirt.c layer (all callers benefit from the
check) rather than at the virsh.c layer. If they are not mutually
exclusive in the background, then virsh is filtering out a useful
combination.
>
> - pool = virStoragePoolCreateXML(ctl->conn, buffer, 0);
> + pool = virStoragePoolCreateXML(ctl->conn, buffer, flags);
> VIR_FREE(buffer);
I think you need fallback code - if this API fails because of unknown
flags, you should try again with flags==0 then follow up with your own
separate call to virStoragePoolBuild with appropriate flags.
> + {"build", VSH_OT_BOOL, 0, N_("build the pool as normal")},
> + {"build-overwrite", VSH_OT_BOOL, 0, N_("build the pool without overwriting "
> + "the existed pool data")},
> + {"build-no-overwrite", VSH_OT_BOOL, 0, N_("build the pool with overwriting anything")},
Same problems as before.
> @@ -257,11 +303,28 @@ cmdPoolCreateAs(vshControl *ctl, const vshCmd *cmd)
> if (!buildPoolXML(cmd, &name, &xml))
> return false;
>
> + build = vshCommandOptBool(cmd, "build");
> + build_overwrite = vshCommandOptBool(cmd, "build-overwrite");
> + build_no_overwrite = vshCommandOptBool(cmd, "build-no-overwrite");
> +
> + if (build + build_overwrite + build_no_overwrite > 1) {
> + vshError(ctl, _("build, build-overwrite, and build-no-overwrite must "
> + "be sepcified exclusively"));
> + return false;
> + }
Same question about where the mutual exclusive check should live.
> @@ -1244,6 +1307,10 @@ static const vshCmdInfo info_pool_start[] = {
>
> static const vshCmdOptDef opts_pool_start[] = {
> {"pool", VSH_OT_DATA, VSH_OFLAG_REQ, N_("name or uuid of the inactive pool")},
> + {"build", VSH_OT_BOOL, 0, N_("build the pool as normal")},
> + {"build-overwrite", VSH_OT_BOOL, 0, N_("build the pool without overwriting the "
> + "existed pool data")},
> + {"build-no-overwrite", VSH_OT_BOOL, 0, N_("build the pool with overwriting anything")},
And again.
> @@ -1260,7 +1331,24 @@ cmdPoolStart(vshControl *ctl, const vshCmd *cmd)
> if (!(pool = vshCommandOptPool(ctl, cmd, "pool", &name)))
> return false;
>
> - if (virStoragePoolCreate(pool, 0) == 0) {
> + build = vshCommandOptBool(cmd, "build");
> + build_overwrite = vshCommandOptBool(cmd, "build-overwrite");
> + build_no_overwrite = vshCommandOptBool(cmd, "build-no-overwrite");
> +
> + if (build + build_overwrite + build_no_overwrite > 1) {
> + vshError(ctl, _("build, build-overwrite, and build-no-overwrite must "
> + "be sepcified exclusively"));
> + return false;
> + }
And again.
> +
> + if (build)
> + flags |= VIR_STORAGE_POOL_CREATE_WITH_BUILD;
> + if (build_overwrite)
> + flags |= VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE;
> + if (build_no_overwrite)
> + flags |= VIR_STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE;
> +
> + if (virStoragePoolCreate(pool, flags) == 0) {
Same question about providing fallback code if flags wasn't recognized.
> +++ b/tools/virsh.pod
> @@ -2097,19 +2097,33 @@ if exists, or using mkfs to format the target device if not; If
> I<--overwrite> is specified, mkfs is always executed, any existed
> data on the target device is overwritten unconditionally.
>
> -=item B<pool-create> I<file>
> +=item B<pool-create> I<file> [[I<--build>] | [I<--build-overwrite>] |
> +[I<--build-no-overwrite>]]
A bit of bike-shedding - would the virsh interface be better as:
--build={normal,overwrite,no-overwrite}
that is, --build takes a string argument, which has to be one of the
three known modes, rather than having three bool arguments that we don't
allow to be used at the same time? But it's not worth the redesign, I
can live with the approach you've coded. I do think it's worth a v4
before pushing this patch, though.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list