Re: [libvirt] [PATCH 2/2] virsh: New options for the 3 pool commands to allow pool building

On 08/17/2012 08:58 AM, Osier Yang wrote: s/existed/existing/ Aren't those two descriptions backwards? 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. 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")}, 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"); > + > + 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")}, 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"); > + > + return false; > + } And again. Same question about providing fallback code if flags wasn't recognized. 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(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org