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

[Peter Krempa]

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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140606/bf3a8de6/attachment-0001.sig>