Re: [libvirt] [PATCH 2/3] blockjob: add virsh blockcommit
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 617 bytes)