9:47 a.m.
On 22.05.2015 10:59, Andrea Bolognani wrote:
---
tests/vcpupin | 4 +-
tools/virsh-domain-monitor.c | 9 +--
tools/virsh-domain.c | 134 +++++++------------------------------------
tools/virsh-host.c | 57 +++---------------
tools/virsh-interface.c | 6 +-
tools/virsh-network.c | 6 +-
tools/virsh-volume.c | 24 ++------
tools/virsh.c | 111 +++++++++++++++++++++--------------
8 files changed, 104 insertions(+), 247 deletions(-)
Nice cleanup.
diff --git a/tools/virsh.c b/tools/virsh.c
index 9f44793..db9354c 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -1517,23 +1517,27 @@ vshCommandOpt(const vshCmd *cmd,
* <0 in all other cases (@value untouched)
*/
Does it makes sense to note in comments that this function (and others that you're
fixing) is reporting an error?
int
-vshCommandOptInt(vshControl *ctl ATTRIBUTE_UNUSED, const vshCmd *cmd,
+vshCommandOptInt(vshControl *ctl, const vshCmd *cmd,
const char *name, int *value)
{
vshCmdOpt *arg;
int ret;
- ret = vshCommandOpt(cmd, name, &arg, true);
- if (ret <= 0)
+ if ((ret = vshCommandOpt(cmd, name, &arg, true)) <= 0)
return ret;
- if (virStrToLong_i(arg->data, NULL, 10, value) < 0)
- return -1;
- return 1;
+ if ((ret = virStrToLong_i(arg->data, NULL, 10, value)) < 0)
+ vshError(ctl,
+ _("Numeric value '%s' for <%s> option is malformed
or out of range"),
+ arg->data, name);
+ else
+ ret = 1;
+
+ return ret;
}
While reworking this, you've missed vshCommandOptTimeoutToMs() which in case of
something like following '--timeout blah' will report error twice: from both
OptInt() and OptTimeoutToMs():
error: Numeric value 'blah' for <timeout> option is malformed or out of
range
error: invalid timeout
I think this should be squashed in:
@@ -1906,11 +1907,13 @@ vshCommandOptTimeoutToMs(vshControl *ctl, const vshCmd *cmd,
{
int rv = vshCommandOptInt(ctl, cmd, "timeout", timeout);
- if (rv < 0 || (rv > 0 && *timeout < 1)) {
- vshError(ctl, "%s", _("invalid timeout"));
+ if (rv < 0)
return -1;
- }
if (rv > 0) {
+ if (*timeout < 1) {
+ vshError(ctl, "%s", _("invalid timeout"));
+ return -1;
+ }
/* Ensure that we can multiply by 1000 without overflowing. */
if (*timeout > INT_MAX / 1000) {
vshError(ctl, "%s", _("timeout is too big"));
static int
-vshCommandOptULongLongInternal(vshControl *ctl ATTRIBUTE_UNUSED, const vshCmd *cmd,
+vshCommandOptULongLongInternal(vshControl *ctl, const vshCmd *cmd,
const char *name, unsigned long long *value,
bool wrap)
{
@@ -1754,15 +1767,18 @@ vshCommandOptULongLongInternal(vshControl *ctl ATTRIBUTE_UNUSED,
const vshCmd *c
if ((ret = vshCommandOpt(cmd, name, &arg, true)) <= 0)
return ret;
- if (wrap) {
- if (virStrToLong_ull(arg->data, NULL, 10, value) < 0)
- return -1;
- } else {
- if (virStrToLong_ullp(arg->data, NULL, 10, value) < 0)
- return -1;
- }
+ if (wrap)
+ ret = virStrToLong_ull(arg->data, NULL, 10, value);
+ else
+ ret = virStrToLong_ullp(arg->data, NULL, 10, value);
+ if (ret < 0)
+ vshError(ctl,
+ _("Numeric value '%s' for <%s> option is malformed
or out of range"),
+ arg->data, name);
+ else
+ ret = 1;
- return 1;
+ return ret;
}
You've missed one ocurrance of vshCommandOptULongLong in cmdAllocpages().
/**
@@ -1812,7 +1828,7 @@ vshCommandOptULongLongWrap(vshControl *ctl, const vshCmd *cmd,
* See vshCommandOptInt()
*/
int
-vshCommandOptScaledInt(vshControl *ctl ATTRIBUTE_UNUSED, const vshCmd *cmd,
+vshCommandOptScaledInt(vshControl *ctl, const vshCmd *cmd,
const char *name, unsigned long long *value,
int scale, unsigned long long max)
{
@@ -1825,9 +1841,16 @@ vshCommandOptScaledInt(vshControl *ctl ATTRIBUTE_UNUSED, const
vshCmd *cmd,
if (virStrToLong_ull(arg->data, &end, 10, value) < 0 ||
virScaleInteger(value, end, scale, max) < 0)
- return -1;
+ {
+ vshError(ctl,
+ _("Numeric value '%s' for <%s> option is malformed
or out of range"),
+ arg->data, name);
+ ret = -1;
+ } else {
+ ret = 1;
+ }
- return 1;
+ return ret;
}
Interestingly vshCommandOptString() is missing. So far, the only way for the function to
fail is if one uses --option "" and VSH_OFLAG_EMPTY_OK is not specified. So if
we come up with good error message for that case, we are okay to drop all the other error
messages.
Otherwise looking good.
Michal