[libvirt] [PATCHv3 1/2] virsh: Reject negative numbers in vshCommandOptUInt

[Eric Blake]

On 05/29/2014 05:54 AM, Peter Krempa wrote:
> Use virStrToLong_uip instead of virStrToLong_ui to reject negative
> numbers in the helper. None of the callers expects the wraparound
> "feature" for negative numbers.

I had to audit all callers, and found the following (fortunately the
list is fairly small):

screenshot [screen]: no one can support 4 billion screens, so forbidding
wraparound makes sense

send-key [holdtime]: waiting 4 million seconds between keys is unlikely,
so forbidding wraparound makes sense

qemu-attach [pid]: pid_t is signed, but negative pids have special
meaning and cannot be attached, forbidding wraparound makes sense

node-memory-tune [shm-pages-to-scan, shm-sleep-millisecs,
shm-merge-across-nodes]: I'm not sure any of these make sense as an
unlimited value, so forbidding wraparound makes sense

iface-bridge [delay] unlimited delay does not work, so forbidding
wraparound makes sense

> 
> Also be explicit about the new semantics in the function docs.

Unfortunately, we now have a discrepancy between this command and
vshCommandOptULongLong and vshCommandOptUL.  So let's look at those, too:

blkdeviotune [total-bytes-sec, read-bytes-sec, write-bytes-sec,
total-iops-sec, read-iops-sec, write-iops-sec]: Here, we allow 0 as
unlimited, so it's probably okay to forbid -1 for maximum.

blockpull, blockcommit [bandwidth]: Here, 0 means the default bandwidth,
which is DIFFERENT than maximum bandwidth.  Allowing -1 might make sense.

dompmsuspend [duration]: the universe will go cold before maximum
duration, so forbidding negative wraparound makes sense

migrate-compcache [size]: sizing this larger than the host has memory is
not going to work, so forbidding negative wraparound might make sense

migrate-setspeed [bandwidth]: Here, 0 means the default bandwidth, which
is DIFFERENT than maximum bandwidth.  Allowing -1 might make sense

domfstrim [minimum]: sizing this larger than the biggest possible file
in the host makes the command a no-op, so forbidding negative wraparound
might make sense

vol-upload, vol-download [offset, length]: Can't a negative offset be
treated as offset from the end of the file? And doesn't -1 as implying
unlimited length make sense?  Here, allowing -1 might make sense


> ---
>  tools/virsh.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Either we should change all three functions, or you should make it
possible to allow negative in some cases per my audit above.  Probably
needs a v4.  Based on patch 2/2, it would still be nice to reach
consensus and fix the bug before 1.2.5, but we're cutting it close.

-- 
Eric Blake   eblake 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: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140529/9543917f/attachment-0001.sig>