7:54 p.m.
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
7:54 p.m.
New subject: [libvirt] [PATCHv3 1/2] virsh: Reject negative numbers in vshCommandOptUInt
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
1:15 a.m.
New subject: [libvirt] [PATCHv3 1/2] virsh: Reject negative numbers in vshCommandOptUInt
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
6:51 a.m.
New subject: [libvirt] [PATCHv3 1/2] virsh: Reject negative numbers in vshCommandOptUInt
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
5:18 p.m.
New subject: [libvirt] [PATCHv3 1/2] virsh: Reject negative numbers in vshCommandOptUInt
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
8:47 p.m.
New subject: [libvirt] [PATCHv3 1/2] virsh: Reject negative numbers in vshCommandOptUInt
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
9:48 p.m.
New subject: [libvirt] [PATCHv3 1/2] virsh: Reject negative numbers in vshCommandOptUInt
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
7:54 p.m.
New subject: [libvirt] [PATCHv3 2/2] virsh: forbid negative vcpu argument to vcpupin
From: Jincheng Miao <jmiao(a)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(a)redhat.com>
Signed-off-by: Peter Krempa <pkrempa(a)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
3706
days inactive
3754
days old
7 comments
3 participants
participants (3)
-
Eric Blake -
John Ferlan -
Peter Krempa