On 04/02/2014 02:43 PM, Michal Privoznik wrote:
Currently, the virsh code is plenty of the following pattern:
if (vshCommandOptUInt(..) < 0) { vshError(...); goto cleanup; }
It doesn't make much sense to repeat the code everywhere. Moreover, some functions from the family already report error some of them don't.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/vcpupin | 1 + tools/virsh.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++------
vshCommandOptTimeoutToMs also calls vshCommandOptInt
tools/virsh.h | 2 +- 3 files changed, 67 insertions(+), 8 deletions(-)
diff --git a/tests/vcpupin b/tests/vcpupin index f1fb038..a616216 100755 --- a/tests/vcpupin +++ b/tests/vcpupin @@ -34,6 +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: Unable to parse integer parameter to --vcpu error: vcpupin: Invalid or missing vCPU number.
EOF diff --git a/tools/virsh.c b/tools/virsh.c index f2e4c4a..1371abb 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1489,8 +1489,18 @@ vshCommandOptInt(const vshCmd *cmd, const char *name, int *value) if (ret <= 0) return ret;
- if (virStrToLong_i(arg->data, NULL, 10, value) < 0) + if (virStrToLong_i(arg->data, NULL, 10, value) < 0) { + if (arg->def->flags & VSH_OFLAG_REQ) { + vshError(NULL,
ctl should be used for reporting errors, just like in vshCommandOptStringReq.
+ _("missing --%s parameter value"), + name);
Missing values are caught in vshCommandParse. The error should be the same regardless of VSH_OFLAG_REQ.
+ } else { + vshError(NULL, + _("Unable to parse integer parameter to --%s"), + name);
@@ -1692,9 +1742,17 @@ vshCommandOptScaledInt(const vshCmd *cmd, const char *name, ret = vshCommandOptString(cmd, name, &str); if (ret <= 0) return ret; - if (virStrToLong_ull(str, &end, 10, value) < 0 || - virScaleInteger(value, end, scale, max) < 0) + if (virStrToLong_ull(str, &end, 10, value) < 0) { + vshError(NULL, + _("Unable to parse integer parameter to --%s"), + name); return -1; + } + + if (virScaleInteger(value, end, scale, max) < 0) { + /* Error reported by the helper. */
Needs vshReportError to propagate the error.
+ return -1; + } return 1; }
diff --git a/tools/virsh.h b/tools/virsh.h index 3e0251b..6eb17b3 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -168,7 +168,7 @@ struct _vshCmdInfo { struct _vshCmdOptDef { const char *name; /* the name of option, or NULL for list end */ vshCmdOptType type; /* option type */ - unsigned int flags; /* flags */ + unsigned int flags; /* bitwise OR of VSH_OFLAG_**/ const char *help; /* non-NULL help string; or for VSH_OT_ALIAS * the name of a later public option */ vshCompleter completer; /* option completer */
You can push this hunk separately as trivial. Jan