[libvirt] [PATCH 2/3] blockjob: add virsh blockcommit

[Eric Blake]

On 09/17/2012 05:29 PM, Peter Krempa wrote:
> On 09/18/12 00:08, Eric Blake wrote:
>> The wait loop logic borrows heavily from blockcommit.

I _meant_ to say 'blockpull'.  And because of my cryptic reference,
instead of a proper attribution,

> 
> Maybe a little too sparse on the commit message, but the man page
> addition makes it up.

...I understand why you had a harder time reviewing it.

> 
>> +    {"bandwidth", VSH_OT_DATA, VSH_OFLAG_NONE, N_("bandwidth limit in
>> MiB/s")},
> 
> Megabits look like a reasonable granularity to limit this. Well maybe
> apart from testing purposes where somebody would like to do it really
> slow to be able to poke around.

That, and the existing 'virsh blockjob --bandwidth' command sets MiB/s,
so we really don't have a choice in our units since this is just another
case of a block job.

>> +    } else if (verbose || vshCommandOptBool(cmd, "timeout") ||
>> +               vshCommandOptBool(cmd, "async")) {
>> +        vshError(ctl, "%s", _("blocking control options require
>> --wait"));
> 
> This error message isn't 100% clear on the first read. What about:
> "--verbose and --timeout require --wait" ?

Sure.  Again, this was copied from 'virsh blockpull', so I'll make the
same change there.

>> +        GETTIMEOFDAY(&curr);
>> +        if (intCaught || (timeout &&
>> +                          (((int)(curr.tv_sec - start.tv_sec)  * 1000 +
>> +                            (int)(curr.tv_usec - start.tv_usec) /
>> 1000) >
>> +                           timeout * 1000))) {
> 
> Wouldn't it be better to multiply timeout by 1000 at the beginning and
> not bother re-doing it on every iteration?

Copy and paste strikes again :)

>> +    vshPrint(ctl, "\n%s", quit ? _("Commit aborted") : _("Commit
>> complete"));
> 
> Meh a ternary ... but saves lines.

and again.

> 
> Your docs are always great. ACK

I'll fix the nits, and push this shortly, then.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

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