[libvirt] [PATCH v2 0/2] Add error handling to optional arguments in cmdCPUStats
https://bugzilla.redhat.com/show_bug.cgi?id=907732 Also added informational message when count value is larger than number of CPUs present. Original code commit '31047e2b' quietly changes it and continues on. Prior to patch 2/2, no errors were seen for following sequences virsh cpu-stats guest xyz virsh cpu-stats guest --start xyz virsh cpu-stats guest --count xyz virsh cpu-stats guest --count 99999999999 With patch 2/2, the following errors are displayed for the above sequence error: Unable to parse integer parameter for start error: Unable to parse integer parameter for start error: Unable to parse integer parameter for CPUs to show error: Unable to parse integer parameter for CPUs to show Additionally, the following will be displayed on negative value input: virsh cpu-stats guest --start -1 error: Invalid value for start CPU virsh cpu-stats guest --count -1 error: Invalid value for number of CPUs to show Using a --count parameter value larger than the number of vCPUs on the system will display the following: virsh cpu-stats guest --count 8 Only 4 CPUs available to show CPU0: cpu_time 10.770466061 seconds vcpu_time 8.327848192 seconds ... Diff to v1 - Split into 2 patches (one for struct defs, one for argument processing) - Adjust argument processessing to delineate between bad argument and invalid value (eg, negative CPU start number or count) - Only display the > # cpus message when too large a value is supplied John Ferlan (2): Remove extraneous comma in info_cpu_stats and opts_cpu_stats Add error handling to optional arguments in cmdCPUStats tools/virsh-domain.c | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) -- 1.8.1.4
--- tools/virsh-domain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index c088468..e6e6877 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6111,7 +6111,7 @@ static const vshCmdInfo info_cpu_stats[] = { {.name = "desc", .data = N_("Display per-CPU and total statistics about the domain's CPUs") }, - {.name = NULL}, + {.name = NULL} }; static const vshCmdOptDef opts_cpu_stats[] = { @@ -6132,7 +6132,7 @@ static const vshCmdOptDef opts_cpu_stats[] = { .type = VSH_OT_INT, .help = N_("Number of shown CPUs at most") }, - {.name = NULL}, + {.name = NULL} }; static bool -- 1.8.1.4
On 04/08/2013 09:35 AM, John Ferlan wrote:
--- tools/virsh-domain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index c088468..e6e6877 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6111,7 +6111,7 @@ static const vshCmdInfo info_cpu_stats[] = { {.name = "desc", .data = N_("Display per-CPU and total statistics about the domain's CPUs") }, - {.name = NULL}, + {.name = NULL}
No semantic difference, only a matter of which style is more consistent. I generally LIKE trailing commas, _if the enum or struct is designed for extension_, because it is nicer to see a diff adding an element like this: oldelt1, oldelt2, + newelt, } than it is like this: oldelt1, - oldelt2 + oldelt2, + newlt } But here, the {.name = NULL} is a sentinel, and we will NEVER add an element after it, so a trailing comma no longer buys us anything. [Any additions we make would be before the sentinel, and there we are guaranteed to already have a comma because the sentinel still has to come last]. On that argument, this patch is fine. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- tools/virsh-domain.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index e6e6877..6d760f2 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6144,15 +6144,35 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd) bool show_total = false, show_per_cpu = false; unsigned int flags = 0; bool ret = false; + int rv = 0; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; show_total = vshCommandOptBool(cmd, "total"); - if (vshCommandOptInt(cmd, "start", &cpu) > 0) + + if ((rv = vshCommandOptInt(cmd, "start", &cpu)) < 0) { + vshError(ctl, "%s", _("Unable to parse integer parameter for start")); + goto cleanup; + } else if (rv > 0) { + if (cpu < 0) { + vshError(ctl, "%s", _("Invalid value for start CPU")); + goto cleanup; + } show_per_cpu = true; - if (vshCommandOptInt(cmd, "count", &show_count) > 0) + } + + if ((rv = vshCommandOptInt(cmd, "count", &show_count)) < 0) { + vshError(ctl, "%s", + _("Unable to parse integer parameter for CPUs to show")); + goto cleanup; + } else if (rv > 0) { + if (show_count < 0) { + vshError(ctl, "%s", _("Invalid value for number of CPUs to show")); + goto cleanup; + } show_per_cpu = true; + } /* default show per_cpu and total */ if (!show_total && !show_per_cpu) { @@ -6170,8 +6190,11 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd) /* get number of cpus on the node */ if ((max_id = virDomainGetCPUStats(dom, NULL, 0, 0, 0, flags)) < 0) goto failed_stats; - if (show_count < 0 || show_count > max_id) + if (show_count < 0 || show_count > max_id) { + if (show_count > max_id) + vshPrint(ctl, _("Only %d CPUs available to show\n"), max_id); show_count = max_id; + } /* get percpu information */ if ((nparams = virDomainGetCPUStats(dom, NULL, 0, 0, 1, flags)) < 0) -- 1.8.1.4
On 04/08/2013 05:35 PM, John Ferlan wrote:
--- tools/virsh-domain.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index e6e6877..6d760f2 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6144,15 +6144,35 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd) bool show_total = false, show_per_cpu = false; unsigned int flags = 0; bool ret = false; + int rv = 0;
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false;
show_total = vshCommandOptBool(cmd, "total"); - if (vshCommandOptInt(cmd, "start", &cpu) > 0) + + if ((rv = vshCommandOptInt(cmd, "start", &cpu)) < 0) { + vshError(ctl, "%s", _("Unable to parse integer parameter for start")); + goto cleanup; + } else if (rv > 0) { + if (cpu < 0) { + vshError(ctl, "%s", _("Invalid value for start CPU")); + goto cleanup; + }
Since we don't allow negative -start values now, you can initialize cpu to 0 instead of -1 and get rid of this hunk: /* check cpu, show_count, and ignore wrong argument */ if (cpu < 0) cpu = 0;
show_per_cpu = true; - if (vshCommandOptInt(cmd, "count", &show_count) > 0) + } + + if ((rv = vshCommandOptInt(cmd, "count", &show_count)) < 0) { + vshError(ctl, "%s", + _("Unable to parse integer parameter for CPUs to show")); + goto cleanup; + } else if (rv > 0) { + if (show_count < 0) { + vshError(ctl, "%s", _("Invalid value for number of CPUs to show")); + goto cleanup; + } show_per_cpu = true; + }
/* default show per_cpu and total */ if (!show_total && !show_per_cpu) { @@ -6170,8 +6190,11 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd) /* get number of cpus on the node */ if ((max_id = virDomainGetCPUStats(dom, NULL, 0, 0, 0, flags)) < 0) goto failed_stats; - if (show_count < 0 || show_count > max_id) + if (show_count < 0 || show_count > max_id) { + if (show_count > max_id) + vshPrint(ctl, _("Only %d CPUs available to show\n"), max_id); show_count = max_id; + }
/* get percpu information */ if ((nparams = virDomainGetCPUStats(dom, NULL, 0, 0, 1, flags)) < 0)
ACK Jan
Thanks for the review - adjusted/removed the -cpu value check w/r/t: patch 1/2 - every other structure in the module didn't have the trailing comma so that was a consistency thing. pushed John
participants (3)
-
Eric Blake -
John Ferlan -
Ján Tomko