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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list