On 06/06/14 14:12, Eric Blake wrote:
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.
Well, the --keep-overlay (or keep-*) option is better in my opinion as you can guess what it does according to the name, where I couldn't guess that --finish does it.
+ }, {.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?
They aren't that much semantically similar from a high-level point of view so it should be okay if we diverge there. On the other hand, those options aren't used that wildly that it should matter that much. I probably don't care that much so I'd force you to change it but it would be better understandable if you do.
ACK to this patch with or without --finish changed. (the change of the ternary to something more readable is still required though). Peter