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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list