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