[libvirt] [PATCH] virsh: Error prompt if one passes negative value for scheduler setting
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
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
于 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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
于 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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* 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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 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
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Osier Yang