5:53 a.m.
As cgroup doesn't allow one writes negative into files like cpu.shares,
(e.g. echo -1> /cgroup/cpu/libvirt/qemu/rhel6/cpu.shares), user will be
confused if libvirt accepts negative value and converts it into unsigned
int (or long int, etc) silently.
* tools/virsh.c
---
tools/virsh.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index cd54174..a0f2527 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -1652,7 +1652,8 @@ cmdSchedInfoUpdate(vshControl *ctl, const vshCmd *cmd,
}
break;
case VIR_DOMAIN_SCHED_FIELD_UINT:
- if (virStrToLong_ui(val, NULL, 10, ¶m->value.ui) < 0) {
+ if (STRPREFIX(val, "-") ||
+ virStrToLong_ui(val, NULL, 10, ¶m->value.ui) < 0) {
vshError(ctl, "%s",
_("Invalid value for parameter, expecting an unsigned
int"));
return -1;
@@ -1666,7 +1667,8 @@ cmdSchedInfoUpdate(vshControl *ctl, const vshCmd *cmd,
}
break;
case VIR_DOMAIN_SCHED_FIELD_ULLONG:
- if (virStrToLong_ull(val, NULL, 10, ¶m->value.ul) < 0) {
+ if (STRPREFIX(val, "-") ||
+ virStrToLong_ull(val, NULL, 10, ¶m->value.ul) < 0) {
vshError(ctl, "%s",
_("Invalid value for parameter, expecting an unsigned long
long"));
return -1;
--
1.7.3.2
6:02 a.m.
On Fri, Jan 28, 2011 at 07:53:44PM +0800, Osier Yang wrote:
As cgroup doesn't allow one writes negative into files like
cpu.shares,
(e.g. echo -1> /cgroup/cpu/libvirt/qemu/rhel6/cpu.shares), user will be
confused if libvirt accepts negative value and converts it into unsigned
int (or long int, etc) silently.
* tools/virsh.c
---
tools/virsh.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index cd54174..a0f2527 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -1652,7 +1652,8 @@ cmdSchedInfoUpdate(vshControl *ctl, const vshCmd *cmd,
}
break;
case VIR_DOMAIN_SCHED_FIELD_UINT:
- if (virStrToLong_ui(val, NULL, 10, ¶m->value.ui) < 0) {
+ if (STRPREFIX(val, "-") ||
+ virStrToLong_ui(val, NULL, 10, ¶m->value.ui) < 0) {
vshError(ctl, "%s",
_("Invalid value for parameter, expecting an unsigned
int"));
return -1;
@@ -1666,7 +1667,8 @@ cmdSchedInfoUpdate(vshControl *ctl, const vshCmd *cmd,
}
break;
case VIR_DOMAIN_SCHED_FIELD_ULLONG:
- if (virStrToLong_ull(val, NULL, 10, ¶m->value.ul) < 0) {
+ if (STRPREFIX(val, "-") ||
+ virStrToLong_ull(val, NULL, 10, ¶m->value.ul) < 0) {
vshError(ctl, "%s",
_("Invalid value for parameter, expecting an unsigned long
long"));
return -1;
Surely this check should be done inside virStrToLong_{ui,ul,ull}
Daniel
6:11 a.m.
于 2011年01月28日 20:02, Daniel P. Berrange 写道:
On Fri, Jan 28, 2011 at 07:53:44PM +0800, Osier Yang wrote:
> As cgroup doesn't allow one writes negative into files like cpu.shares,
> (e.g. echo -1> /cgroup/cpu/libvirt/qemu/rhel6/cpu.shares), user will be
> confused if libvirt accepts negative value and converts it into unsigned
> int (or long int, etc) silently.
>
> * tools/virsh.c
> ---
> tools/virsh.c | 6 ++++--
> 1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tools/virsh.c b/tools/virsh.c
> index cd54174..a0f2527 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -1652,7 +1652,8 @@ cmdSchedInfoUpdate(vshControl *ctl, const vshCmd *cmd,
> }
> break;
> case VIR_DOMAIN_SCHED_FIELD_UINT:
> - if (virStrToLong_ui(val, NULL, 10,¶m->value.ui)< 0) {
> + if (STRPREFIX(val, "-") ||
> + virStrToLong_ui(val, NULL, 10,¶m->value.ui)< 0) {
> vshError(ctl, "%s",
> _("Invalid value for parameter, expecting an unsigned
int"));
> return -1;
> @@ -1666,7 +1667,8 @@ cmdSchedInfoUpdate(vshControl *ctl, const vshCmd *cmd,
> }
> break;
> case VIR_DOMAIN_SCHED_FIELD_ULLONG:
> - if (virStrToLong_ull(val, NULL, 10,¶m->value.ul)< 0) {
> + if (STRPREFIX(val, "-") ||
> + virStrToLong_ull(val, NULL, 10,¶m->value.ul)< 0) {
> vshError(ctl, "%s",
> _("Invalid value for parameter, expecting an unsigned
long long"));
> return -1;
Surely this check should be done inside virStrToLong_{ui,ul,ull}
Is there some codes that expects virStrToLong_{ui,ul,ull} to convert
the negative to {ui, ul, ull}? I was worried about it, so didn't make
the changes inside those functions, if no codes expect that, would
like make changes those functions with a v2 patch then.
Thanks
Osier
9:33 a.m.
On 01/28/2011 05:11 AM, Osier Yang wrote:
于 2011年01月28日 20:02, Daniel P. Berrange 写道:
> On Fri, Jan 28, 2011 at 07:53:44PM +0800, Osier Yang wrote:
>> As cgroup doesn't allow one writes negative into files like cpu.shares,
>> (e.g. echo -1> /cgroup/cpu/libvirt/qemu/rhel6/cpu.shares), user will be
>> confused if libvirt accepts negative value and converts it into unsigned
>> int (or long int, etc) silently.
But strtoul() is explicitly documented as accepting -1 as shorthand for
ULONG_MAX, and as a command-line convenience, I much prefer that
shorthand over 18446744073709551615. I don't see this as confusing.
>
> Surely this check should be done inside virStrToLong_{ui,ul,ull}
If we want to be sticklers about reject '-' and module 2^32 (2^64)
wraparound, then yes, virStrToLong_u* would be the place to do it. But
I'm not convinced that this is a good change.
Is there some codes that expects virStrToLong_{ui,ul,ull} to convert
the negative to {ui, ul, ull}?
Probably, given the above arguments about -1 being useful shorthand for
the maximum value.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
9:48 a.m.
On Fri, Jan 28, 2011 at 08:33:20AM -0700, Eric Blake wrote:
On 01/28/2011 05:11 AM, Osier Yang wrote:
> 于 2011年01月28日 20:02, Daniel P. Berrange 写道:
>> On Fri, Jan 28, 2011 at 07:53:44PM +0800, Osier Yang wrote:
>>> As cgroup doesn't allow one writes negative into files like cpu.shares,
>>> (e.g. echo -1> /cgroup/cpu/libvirt/qemu/rhel6/cpu.shares), user will be
>>> confused if libvirt accepts negative value and converts it into unsigned
>>> int (or long int, etc) silently.
But strtoul() is explicitly documented as accepting -1 as shorthand for
ULONG_MAX, and as a command-line convenience, I much prefer that
shorthand over 18446744073709551615. I don't see this as confusing.
I didn't know about that behaviour :-) I guess this is at worst
a documentation problem then, not something we need change in
code.
Daniel
8:40 p.m.
于 2011年01月28日 23:33, Eric Blake 写道:
On 01/28/2011 05:11 AM, Osier Yang wrote:
> 于 2011年01月28日 20:02, Daniel P. Berrange 写道:
>> On Fri, Jan 28, 2011 at 07:53:44PM +0800, Osier Yang wrote:
>>> As cgroup doesn't allow one writes negative into files like cpu.shares,
>>> (e.g. echo -1> /cgroup/cpu/libvirt/qemu/rhel6/cpu.shares), user will be
>>> confused if libvirt accepts negative value and converts it into unsigned
>>> int (or long int, etc) silently.
But strtoul() is explicitly documented as accepting -1 as shorthand for
ULONG_MAX, and as a command-line convenience, I much prefer that
shorthand over 18446744073709551615. I don't see this as confusing.
>>
>> Surely this check should be done inside virStrToLong_{ui,ul,ull}
If we want to be sticklers about reject '-' and module 2^32 (2^64)
wraparound, then yes, virStrToLong_u* would be the place to do it. But
I'm not convinced that this is a good change.
I'm afraid changing on virStrToLong_u* will affect other codes, pretty
codes use them.
> Is there some codes that expects virStrToLong_{ui,ul,ull} to convert
> the negative to {ui, ul, ull}?
Probably, given the above arguments about -1 being useful shorthand for
the maximum value.
Yeah, so are you agreed with danpb? making changes on docs?
Regards
Osier
8:49 a.m.
On 01/28/2011 07:40 PM, Osier Yang wrote:
I'm afraid changing on virStrToLong_u* will affect other codes,
pretty
codes use them.
>
>> Is there some codes that expects virStrToLong_{ui,ul,ull} to convert
>> the negative to {ui, ul, ull}?
>
> Probably, given the above arguments about -1 being useful shorthand for
> the maximum value.
>
Yeah, so are you agreed with danpb? making changes on docs?
Yes, I think documenting that negative values are shorthand and wrapped
around to positive is the right thing to do here.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
11:43 p.m.
New subject: [libvirt] [PATCH] docs: update docs for cpu_shares setting
* tools/virsh.pod
---
tools/virsh.pod | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 441ca8a..d3262b6 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -566,7 +566,11 @@ Xen (credit scheduler): weight, cap
ESX (allocation scheduler): reservation, limit, shares
-B<Note>: The cpu_shares parameter has a valid value range of 0-262144.
+B<Note>: The cpu_shares parameter has a valid value range of 0-262144, however,
+negative number and positive number greater than 262144 are allowed, but
+negative number will be converted into positive number, and positive number
+greater than 262144 will be reduced to 262144 by cgroup in the end, e.g. -1
+can be used as useful shorthand for 262144.
B<Note>: The weight and cap parameters are defined only for the
XEN_CREDIT scheduler and are now I<DEPRECATED>.
--
1.7.3.2
11:19 a.m.
New subject: [libvirt] [PATCH] docs: update docs for cpu_shares setting
On 01/29/2011 10:43 PM, Osier Yang wrote:
* tools/virsh.pod
---
tools/virsh.pod | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 441ca8a..d3262b6 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -566,7 +566,11 @@ Xen (credit scheduler): weight, cap
ESX (allocation scheduler): reservation, limit, shares
-B<Note>: The cpu_shares parameter has a valid value range of 0-262144.
+B<Note>: The cpu_shares parameter has a valid value range of 0-262144, however,
+negative number and positive number greater than 262144 are allowed, but
+negative number will be converted into positive number, and positive number
+greater than 262144 will be reduced to 262144 by cgroup in the end, e.g. -1
+can be used as useful shorthand for 262144.
The range of 262144 is specific to cgroups (qemu and lxc); it is
possible that some other hypervisor may add similar support for this
parameter, but have a different range. Then again, we already
documented it is specific to qemu and lxc, so we could update a change
in range at the time of introducing the cpu_shares parameter to another
hypervisor.
However, your attempt is still wordy. Can we go with something more
compact, as in:
B<Note>: The cpu_shares parameter has a valid value range of 0-262144;
negative values are wrapped to positive, and larger values are capped at
the maximum. Therefore, -1 is a useful shorthand for 262144.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
2:51 a.m.
New subject: [libvirt] [PATCH] docs: update docs for cpu_shares setting
于 2011年02月01日 01:19, Eric Blake 写道:
On 01/29/2011 10:43 PM, Osier Yang wrote:
> * tools/virsh.pod
> ---
> tools/virsh.pod | 6 +++++-
> 1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 441ca8a..d3262b6 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -566,7 +566,11 @@ Xen (credit scheduler): weight, cap
>
> ESX (allocation scheduler): reservation, limit, shares
>
> -B<Note>: The cpu_shares parameter has a valid value range of 0-262144.
> +B<Note>: The cpu_shares parameter has a valid value range of 0-262144,
however,
> +negative number and positive number greater than 262144 are allowed, but
> +negative number will be converted into positive number, and positive number
> +greater than 262144 will be reduced to 262144 by cgroup in the end, e.g. -1
> +can be used as useful shorthand for 262144.
The range of 262144 is specific to cgroups (qemu and lxc); it is
possible that some other hypervisor may add similar support for this
parameter, but have a different range. Then again, we already
documented it is specific to qemu and lxc, so we could update a change
in range at the time of introducing the cpu_shares parameter to another
hypervisor.
However, your attempt is still wordy. Can we go with something more
compact, as in:
B<Note>: The cpu_shares parameter has a valid value range of 0-262144;
negative values are wrapped to positive, and larger values are capped at
the maximum. Therefore, -1 is a useful shorthand for 262144.
Yeah, It's more compact, updated with above, and pushed, thanks
Regards
Osier
5045
days inactive
5049
days old
9 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Osier Yang