[libvirt] [PATCH] lxcSetSchedulerParameters: reverse order of tests; improve a diagnostic

Investigating something else in the vicinity, I noticed that the ordering of tests was backwards. Here's the fix for that. Also, looking at the following code, I saw that failure of virCgroupSetCpuShares could evoke no diagnostic. Now it does.
From bc3404d9f12c42cf883a43395fee6fc14c952b2c Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 11 May 2010 15:43:32 +0200 Subject: [PATCH] lxcSetSchedulerParameters: reverse order of tests; diagnose a failure
* src/lxc/lxc_driver.c (lxcSetSchedulerParameters): Ensure that "->field" is "cpu_shares" before possibly giving a diagnostic about a type for a "cpu_shares" value. Also, virCgroupSetCpuShares could fail without evoking a diagnostic. Add one. --- src/lxc/lxc_driver.c | 19 ++++++++++++------- 1 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index fc0df37..5636eee 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2054,18 +2054,23 @@ static int lxcSetSchedulerParameters(virDomainPtr domain, for (i = 0; i < nparams; i++) { virSchedParameterPtr param = ¶ms[i]; + + if (STRNEQ(param->field, "cpu_shares")) { + lxcError(VIR_ERR_INVALID_ARG, + _("Invalid parameter `%s'"), param->field); + goto cleanup; + } + if (param->type != VIR_DOMAIN_SCHED_FIELD_ULLONG) { lxcError(VIR_ERR_INVALID_ARG, "%s", - _("Invalid type for cpu_shares tunable, expected a 'ullong'")); + _("Invalid type for cpu_shares tunable, expected a 'ullong'")); goto cleanup; } - if (STREQ(param->field, "cpu_shares")) { - if (virCgroupSetCpuShares(group, params[i].value.ul) != 0) - goto cleanup; - } else { - lxcError(VIR_ERR_INVALID_ARG, - _("Invalid parameter `%s'"), param->field); + int rc = virCgroupSetCpuShares(group, params[i].value.ul); + if (rc != 0) { + virReportSystemError(-rc, _("failed to set cpu_shares=%llu"), + params[i].value.ul); goto cleanup; } } -- 1.7.1.189.g07419

On 05/11/2010 07:51 AM, Jim Meyering wrote:
Investigating something else in the vicinity, I noticed that the ordering of tests was backwards. Here's the fix for that.
Also, looking at the following code, I saw that failure of virCgroupSetCpuShares could evoke no diagnostic. Now it does.
From bc3404d9f12c42cf883a43395fee6fc14c952b2c Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 11 May 2010 15:43:32 +0200 Subject: [PATCH] lxcSetSchedulerParameters: reverse order of tests; diagnose a failure
* src/lxc/lxc_driver.c (lxcSetSchedulerParameters): Ensure that "->field" is "cpu_shares" before possibly giving a diagnostic about a type for a "cpu_shares" value. Also, virCgroupSetCpuShares could fail without evoking a diagnostic. Add one. --- src/lxc/lxc_driver.c | 19 ++++++++++++------- 1 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index fc0df37..5636eee 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2054,18 +2054,23 @@ static int lxcSetSchedulerParameters(virDomainPtr domain,
for (i = 0; i < nparams; i++) { virSchedParameterPtr param = ¶ms[i]; + + if (STRNEQ(param->field, "cpu_shares")) { + lxcError(VIR_ERR_INVALID_ARG, + _("Invalid parameter `%s'"), param->field); + goto cleanup; + } + if (param->type != VIR_DOMAIN_SCHED_FIELD_ULLONG) { lxcError(VIR_ERR_INVALID_ARG, "%s", - _("Invalid type for cpu_shares tunable, expected a 'ullong'")); + _("Invalid type for cpu_shares tunable, expected a 'ullong'"));
Why the indentation change here?
goto cleanup; }
- if (STREQ(param->field, "cpu_shares")) { - if (virCgroupSetCpuShares(group, params[i].value.ul) != 0) - goto cleanup; - } else { - lxcError(VIR_ERR_INVALID_ARG, - _("Invalid parameter `%s'"), param->field); + int rc = virCgroupSetCpuShares(group, params[i].value.ul); + if (rc != 0) { + virReportSystemError(-rc, _("failed to set cpu_shares=%llu"), + params[i].value.ul); goto cleanup; } }
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 05/12/2010 07:23 PM, Eric Blake wrote: [trying to clear out some old mail]
From bc3404d9f12c42cf883a43395fee6fc14c952b2c Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 11 May 2010 15:43:32 +0200 Subject: [PATCH] lxcSetSchedulerParameters: reverse order of tests; diagnose a failure
* src/lxc/lxc_driver.c (lxcSetSchedulerParameters): Ensure that "->field" is "cpu_shares" before possibly giving a diagnostic about a type for a "cpu_shares" value. Also, virCgroupSetCpuShares could fail without evoking a diagnostic. Add one.
lxcError(VIR_ERR_INVALID_ARG, "%s", - _("Invalid type for cpu_shares tunable, expected a 'ullong'")); + _("Invalid type for cpu_shares tunable, expected a 'ullong'"));
Why the indentation change here?
To answer my own question - to fit in 80 columns.
ACK.
I applied this one on Jim's behalf, after validating that everything still builds fine after rebasing on current sources. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

Eric Blake wrote:
On 05/12/2010 07:23 PM, Eric Blake wrote:
[trying to clear out some old mail]
From bc3404d9f12c42cf883a43395fee6fc14c952b2c Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 11 May 2010 15:43:32 +0200 Subject: [PATCH] lxcSetSchedulerParameters: reverse order of tests; diagnose a failure
* src/lxc/lxc_driver.c (lxcSetSchedulerParameters): Ensure that "->field" is "cpu_shares" before possibly giving a diagnostic about a type for a "cpu_shares" value. Also, virCgroupSetCpuShares could fail without evoking a diagnostic. Add one.
lxcError(VIR_ERR_INVALID_ARG, "%s", - _("Invalid type for cpu_shares tunable, expected a 'ullong'")); + _("Invalid type for cpu_shares tunable, expected a 'ullong'"));
Why the indentation change here?
To answer my own question - to fit in 80 columns.
ACK.
I applied this one on Jim's behalf, after validating that everything still builds fine after rebasing on current sources.
Thanks! It'd fallen off my radar.
participants (2)
-
Eric Blake
-
Jim Meyering