[libvirt] [PATCHv3 0/2] virsh: Fix negative value parsing in vcpuinfo
Jincheng Miao (1): virsh: forbid negative vcpu argument to vcpupin Peter Krempa (1): virsh: Reject negative numbers in vshCommandOptUInt tests/vcpupin | 29 ++++++++++++++++++++++++++++- tools/virsh-domain.c | 35 ++++++++++++++++++----------------- tools/virsh.c | 4 ++-- 3 files changed, 48 insertions(+), 20 deletions(-) -- 1.9.3
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. Also be explicit about the new semantics in the function docs. --- tools/virsh.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 15f3025..483dc04 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1501,7 +1501,7 @@ vshCommandOptInt(const vshCmd *cmd, const char *name, int *value) * @name option name * @value result * - * Convert option to unsigned int + * Convert option to unsigned int, reject negative numbers * See vshCommandOptInt() */ int @@ -1514,7 +1514,7 @@ vshCommandOptUInt(const vshCmd *cmd, const char *name, unsigned int *value) if (ret <= 0) return ret; - if (virStrToLong_ui(arg->data, NULL, 10, value) < 0) + if (virStrToLong_uip(arg->data, NULL, 10, value) < 0) return -1; return 1; } -- 1.9.3
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
Bumping again - I found a related bz in my backlog... On 05/29/2014 01:15 PM, Eric Blake wrote:
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):
<snip>
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
BZ - https://bugzilla.redhat.com/show_bug.cgi?id=1087104 has a different take on whether negative values should be allowed. In particular: " Additional Info: In manual page: vol-download [--pool pool-or-uuid] [--offset bytes] [--length bytes] --offset is the position in the storage volume at which to start reading the data. --length is an upper bound of the amount of data to be downloaded. It is said that offset is the position in the storage volume at which to start reading the data, so I think the value of offset should be no smaller than 0, option length as well. " So - do we "adjust" the man page to indicate that using a -1 is "OK" and what it would do? Probably similar type action for the changes made (commit id's 0e2d73051 && c62125395)? Or does a negative "really" make sense for offset? Sure -1 makes sense and works because 'lseek()' allows it, but other negative numbers I just tried get an error: virsh vol-download --pool default qcow3-vol2 /home/vm-images/raw --offset -2 --length -1000 error: cannot download from volume qcow3-vol2 error: Unable to seek /home/vm-images/qcow3-vol2 to 18446744073709551614: Invalid argument Not even sure what a negative length file is... Is that the definition of a sparse file? Is the suggestion that we'd be downloading (or uploading) from end of file backwards some number of bytes? Not quite sure that's what's happening as the negative value is turned positive and it seems means "everything". So while, yes, -1 for both makes sense as a sort of pseudonym for maximum - other values don't, but how does one go about distinguishing that? (eg, that a -1 was supplied and it's OK, although other negative values are not). John
On 07/15/14 00:51, John Ferlan wrote:
Bumping again - I found a related bz in my backlog...
On 05/29/2014 01:15 PM, Eric Blake wrote:
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):
<snip>
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
BZ - https://bugzilla.redhat.com/show_bug.cgi?id=1087104 has a different take on whether negative values should be allowed. In particular:
" Additional Info: In manual page: vol-download [--pool pool-or-uuid] [--offset bytes] [--length bytes]
--offset is the position in the storage volume at which to start reading the data. --length is an upper bound of the amount of data to be downloaded.
It is said that offset is the position in the storage volume at which to start reading the data, so I think the value of offset should be no smaller than 0, option length as well. "
So - do we "adjust" the man page to indicate that using a -1 is "OK" and what it would do? Probably similar type action for the changes made (commit id's 0e2d73051 && c62125395)?
Or does a negative "really" make sense for offset? Sure -1 makes sense and works because 'lseek()' allows it, but other negative numbers I just tried get an error:
Well it might make sense semantically but it certainly isn't coded up. We'd need to wrap the number sensibly according to the length of the file so that it would mean the offset from the end of the file as Eric suggested. Also we might add the docs about how the offset is wrapped to virsh but reject those numbers in the API. This said, I'm not a big fan of the negative offset as with the theoretically unlimited file sizes (2^64bytes) you might want to have the full number space of the value we are passing as the offset available for very long offsets. Granted, that will not happen for a while so we might want to sacrifice half of the numbers for negative offsets, but we should've gone with signed types in that case. TL;DR; Taking negative portions of the --offset as offset from the back might not play well with very large files.
virsh vol-download --pool default qcow3-vol2 /home/vm-images/raw --offset -2 --length -1000 error: cannot download from volume qcow3-vol2 error: Unable to seek /home/vm-images/qcow3-vol2 to 18446744073709551614: Invalid argument
Not even sure what a negative length file is... Is that the definition of a sparse file? Is the suggestion that we'd be downloading (or
Eric thought of lenght of -1 as full file lenght.
uploading) from end of file backwards some number of bytes? Not quite sure that's what's happening as the negative value is turned positive and it seems means "everything".
So while, yes, -1 for both makes sense as a sort of pseudonym for maximum - other values don't, but how does one go about distinguishing that? (eg, that a -1 was supplied and it's OK, although other negative values are not).
We should have designed the APIs with signed types if we wanted to take signed numbers. I still think of parsing -1 into a unsigned type as a quirk that we shouldn't abuse very much.
John
Peter
On 07/16/2014 03:18 AM, Peter Krempa wrote:
So - do we "adjust" the man page to indicate that using a -1 is "OK" and what it would do? Probably similar type action for the changes made (commit id's 0e2d73051 && c62125395)?
Or does a negative "really" make sense for offset? Sure -1 makes sense and works because 'lseek()' allows it, but other negative numbers I just tried get an error:
Well it might make sense semantically but it certainly isn't coded up. We'd need to wrap the number sensibly according to the length of the file so that it would mean the offset from the end of the file as Eric suggested.
Also we might add the docs about how the offset is wrapped to virsh but reject those numbers in the API.
This said, I'm not a big fan of the negative offset as with the theoretically unlimited file sizes (2^64bytes) you might want to have the full number space of the value we are passing as the offset
Unlimited file size is 2^63, not 2^64 (off_t is signed; so there is no way the kernel can provide you access to more than 8 exabytes. Anyone that really has 16 exabytes of data to manage [hah!] has to split it into multiple files. Treating a negative value as distance from the end of the file is always possible. Whether or not it is practical and worth coding up is another matter.
available for very long offsets. Granted, that will not happen for a while so we might want to sacrifice half of the numbers for negative offsets, but we should've gone with signed types in that case.
off_t is already signed; the kernel has already sacrificed half the numbers, and lseek() already has a mode to do negative offsets from the end of an arbitrarily large file, up to the 2^63 limit imposed by off_t.
TL;DR; Taking negative portions of the --offset as offset from the back might not play well with very large files.
Not true.
virsh vol-download --pool default qcow3-vol2 /home/vm-images/raw --offset -2 --length -1000 error: cannot download from volume qcow3-vol2 error: Unable to seek /home/vm-images/qcow3-vol2 to 18446744073709551614: Invalid argument
Not even sure what a negative length file is... Is that the definition of a sparse file? Is the suggestion that we'd be downloading (or
Eric thought of lenght of -1 as full file lenght.
Another possibility is special-casing length 0 (not -1) as full file length, since it transferring 0 bytes is already a weird thing to do.
uploading) from end of file backwards some number of bytes? Not quite sure that's what's happening as the negative value is turned positive and it seems means "everything".
So while, yes, -1 for both makes sense as a sort of pseudonym for maximum - other values don't, but how does one go about distinguishing that? (eg, that a -1 was supplied and it's OK, although other negative values are not).
We should have designed the APIs with signed types if we wanted to take signed numbers. I still think of parsing -1 into a unsigned type as a quirk that we shouldn't abuse very much.
It's a nice quirk for anyone familiar with 2s-complement. But I can also agree that not abusing it means fewer corner cases to test. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07/16/2014 08:47 AM, Eric Blake wrote:
On 07/16/2014 03:18 AM, Peter Krempa wrote:
So - do we "adjust" the man page to indicate that using a -1 is "OK" and what it would do? Probably similar type action for the changes made (commit id's 0e2d73051 && c62125395)?
Or does a negative "really" make sense for offset? Sure -1 makes sense and works because 'lseek()' allows it, but other negative numbers I just tried get an error:
Well it might make sense semantically but it certainly isn't coded up. We'd need to wrap the number sensibly according to the length of the file so that it would mean the offset from the end of the file as Eric suggested.
Also we might add the docs about how the offset is wrapped to virsh but reject those numbers in the API.
This said, I'm not a big fan of the negative offset as with the theoretically unlimited file sizes (2^64bytes) you might want to have the full number space of the value we are passing as the offset
Unlimited file size is 2^63, not 2^64 (off_t is signed; so there is no way the kernel can provide you access to more than 8 exabytes. Anyone that really has 16 exabytes of data to manage [hah!] has to split it into multiple files. Treating a negative value as distance from the end of the file is always possible. Whether or not it is practical and worth coding up is another matter.
But yet vol-{upload|download} has explicitly chosen to define offset as an "unsigned unsigned long" - so is the claim then that is incorrect? Should it be be an 'off_t'? thus allowing for a negative offset? If so, that's *a lot* of code impacted compared to perhaps disallowing a negative value for offset.
available for very long offsets. Granted, that will not happen for a while so we might want to sacrifice half of the numbers for negative offsets, but we should've gone with signed types in that case.
off_t is already signed; the kernel has already sacrificed half the numbers, and lseek() already has a mode to do negative offsets from the end of an arbitrarily large file, up to the 2^63 limit imposed by off_t.
In the scope of the vol-{upload|download} does it make sense to lseek to some offset from the (unknown?) end of file in order to copy some set number of bytes (or perhaps "everything" if length was negative)?
TL;DR; Taking negative portions of the --offset as offset from the back might not play well with very large files.
Not true.
virsh vol-download --pool default qcow3-vol2 /home/vm-images/raw --offset -2 --length -1000 error: cannot download from volume qcow3-vol2 error: Unable to seek /home/vm-images/qcow3-vol2 to 18446744073709551614: Invalid argument
Not even sure what a negative length file is... Is that the definition of a sparse file? Is the suggestion that we'd be downloading (or
Eric thought of lenght of -1 as full file lenght.
My intended sarcastic comment didn't translate well...
Another possibility is special-casing length 0 (not -1) as full file length, since it transferring 0 bytes is already a weird thing to do.
I agree a length of 0 means essentially nothing unless you consider some long running program that periodically uses vol-{upload|download} to copy the difference in bytes/length of the file since the previous run. If the previous run had no change, then copying over the entire file because the difference in length was 0 probably wouldn't go over well. I feel I've been down that path before... I think allowing negative length's as the "feature" to mean essentially everything is more reasonable. The bugger is documenting things.
uploading) from end of file backwards some number of bytes? Not quite sure that's what's happening as the negative value is turned positive and it seems means "everything".
So while, yes, -1 for both makes sense as a sort of pseudonym for maximum - other values don't, but how does one go about distinguishing that? (eg, that a -1 was supplied and it's OK, although other negative values are not).
We should have designed the APIs with signed types if we wanted to take signed numbers. I still think of parsing -1 into a unsigned type as a quirk that we shouldn't abuse very much.
It's a nice quirk for anyone familiar with 2s-complement. But I can also agree that not abusing it means fewer corner cases to test.
Quirks keep us employed :-) John
From: Jincheng Miao <jmiao@redhat.com> The vcpupin command allowed specifying a negative number for the --vcpu argument. This would the overflow when the underlying virDomainPinVcpu API was called. $ virsh vcpupin r7 -1 0 error: numerical overflow: input too large: 4294967295 Switch the vCPU variable to a unsigned int and parse it using the corresponding function. Also improve the vcpupin test to cover all the defects. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1101059 Signed-off-by: Jincheng Miao <jmiao@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- Notes: Version 3: - fix multiple problems, improve tests and refactor the error handling a bit better tests/vcpupin | 29 ++++++++++++++++++++++++++++- tools/virsh-domain.c | 35 ++++++++++++++++++----------------- 2 files changed, 46 insertions(+), 18 deletions(-) diff --git a/tests/vcpupin b/tests/vcpupin index f1fb038..9f34ec0 100755 --- a/tests/vcpupin +++ b/tests/vcpupin @@ -34,7 +34,7 @@ fail=0 $abs_top_builddir/tools/virsh --connect test:///default vcpupin test a 0,1 > out 2>&1 test $? = 1 || fail=1 cat <<\EOF > exp || fail=1 -error: vcpupin: Invalid or missing vCPU number. +error: vcpupin: Invalid vCPU number. EOF compare exp out || fail=1 @@ -43,9 +43,36 @@ compare exp out || fail=1 $abs_top_builddir/tools/virsh --connect test:///default vcpupin test 100 0,1 > out 2>&1 test $? = 1 || fail=1 cat <<\EOF > exp || fail=1 +error: vcpupin: vCPU index out of range. + +EOF +compare exp out || fail=1 + +# Negative number +$abs_top_builddir/tools/virsh --connect test:///default vcpupin test -100 0,1 > out 2>&1 +test $? = 1 || fail=1 +cat <<\EOF > exp || fail=1 error: vcpupin: Invalid vCPU number. EOF compare exp out || fail=1 +# missing argument +$abs_top_builddir/tools/virsh --connect test:///default vcpupin test --cpulist 0,1 > out 2>&1 +test $? = 1 || fail=1 +cat <<\EOF > exp || fail=1 +error: vcpupin: Missing vCPU number in pin mode. + +EOF +compare exp out || fail=1 + +# without arguments. This should succeed but the backend function in the +# test driver isn't implemented +$abs_top_builddir/tools/virsh --connect test:///default vcpupin test > out 2>&1 +test $? = 1 || fail=1 +cat <<\EOF > exp || fail=1 +error: this function is not supported by the connection driver: virDomainGetVcpuPinInfo + +EOF +compare exp out || fail=1 (exit $fail); exit $fail diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 84a6706..e76418a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -5797,7 +5797,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) { virDomainInfo info; virDomainPtr dom; - int vcpu = -1; + unsigned int vcpu = 0; const char *cpulist = NULL; bool ret = false; unsigned char *cpumap = NULL; @@ -5809,6 +5809,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) bool live = vshCommandOptBool(cmd, "live"); bool current = vshCommandOptBool(cmd, "current"); bool query = false; /* Query mode if no cpulist */ + int got_vcpu; unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; VSH_EXCLUSIVE_OPTIONS_VAR(current, live); @@ -5830,29 +5831,29 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) query = !cpulist; - /* In query mode, "vcpu" is optional */ - if (vshCommandOptInt(cmd, "vcpu", &vcpu) < !query) { - vshError(ctl, "%s", - _("vcpupin: Invalid or missing vCPU number.")); - virDomainFree(dom); - return false; + if ((got_vcpu = vshCommandOptUInt(cmd, "vcpu", &vcpu)) < 0) { + vshError(ctl, "%s", _("vcpupin: Invalid vCPU number.")); + goto cleanup; } - if ((maxcpu = vshNodeGetCPUCount(ctl->conn)) < 0) { - virDomainFree(dom); - return false; + /* In pin mode, "vcpu" is necessary */ + if (!query && got_vcpu == 0) { + vshError(ctl, "%s", _("vcpupin: Missing vCPU number in pin mode.")); + goto cleanup; } if (virDomainGetInfo(dom, &info) != 0) { vshError(ctl, "%s", _("vcpupin: failed to get domain information.")); - virDomainFree(dom); - return false; + goto cleanup; } if (vcpu >= info.nrVirtCpu) { - vshError(ctl, "%s", _("vcpupin: Invalid vCPU number.")); - virDomainFree(dom); - return false; + vshError(ctl, "%s", _("vcpupin: vCPU index out of range.")); + goto cleanup; + } + + if ((maxcpu = vshNodeGetCPUCount(ctl->conn)) < 0) { + goto cleanup; } cpumaplen = VIR_CPU_MAPLEN(maxcpu); @@ -5871,7 +5872,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) vshPrintExtra(ctl, "%s %s\n", _("VCPU:"), _("CPU Affinity")); vshPrintExtra(ctl, "----------------------------------\n"); for (i = 0; i < ncpus; i++) { - if (vcpu != -1 && i != vcpu) + if (got_vcpu && i != vcpu) continue; vshPrint(ctl, "%4zu: ", i); @@ -5880,8 +5881,8 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) if (!ret) break; } - } + VIR_FREE(cpumaps); goto cleanup; } -- 1.9.3
participants (3)
-
Eric Blake -
John Ferlan -
Peter Krempa