Re: [libvirt] [PATCH v2 07/10] virsh: expose new active commit controls

On 06/06/2014 05:57 AM, Peter Krempa wrote: > On 06/06/14 00:52, Eric Blake wrote: >> Add knobs to virsh to manage a 2-phase active commit of the top >> layer, similar to knobs already present on blockcopy. While this >> code will fail until later patches actually implement the new >> knobs in the qemu driver, doing it now proves that the API is >> usable and also makes it easier for testing the qemu changes as >> they are made. >> >> * tools/virsh-domain.c (cmdBlockCommit): Add --active, --pivot, >> and --finish options, modeled after blockcopy. >> (blockJobImpl): Support --active flag. >> * tools/virsh.pod (blockcommit): Document new flags. >> (blockjob): Mention 2-phase commit interaction. >> + {.name = "finish", >> + .type = VSH_OT_BOOL, >> + .help = N_("with --active --wait, quit when commit is synced") > > Finish seems a bit too generic ... shouldn't we go with something like > --keep-overlay or other more specific flag? blockcopy has the same flag, but yeah, I could go with keep-overlay. It's shorthand for doing a two-command sequence: blockcommit --wait blockjob --abort but with a nicer name for what the abort actually does in the second phase.

> >> + }, >> {.name = "async", >> .type = VSH_OT_BOOL, >> .help = N_("with --wait, don't wait for cancel to finish") > >> @@ -1709,7 +1744,22 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) >> /* printf [100 %] */ >> vshPrintJobProgress(_("Block Commit"), 0, 1); >> } >> - vshPrint(ctl, "\n%s", quit ? _("Commit aborted") : _("Commit complete")); >> + if (!quit && pivot) { >> + abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT; >> + if (virDomainBlockJobAbort(dom, path, abort_flags) < 0) { >> + vshError(ctl, _("failed to pivot job for disk %s"), path); >> + goto cleanup; >> + } >> + } else if (finish && !quit && >> + virDomainBlockJobAbort(dom, path, abort_flags) < 0) { >> + vshError(ctl, _("failed to finish job for disk %s"), path); >> + goto cleanup; >> + } >> + vshPrint(ctl, "\n%s", >> + quit ? _("Commit aborted") : >> + pivot ? _("Successfully pivoted") : >> + !finish ? _("Now in synchronized phase") : >> + _("Commit complete")); > > Uhhh, embedded ternaries? Please change it to hierarchical if's or > something. Copied from blockcopy; I guess I can fix both instances. > > We should discuss the --finish flag name a little bit more. Is it better to have blockcopy and blockcommit look alike, or to make each command have a better description of what it is doing?