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