[libvirt] [PATCH 1/2] virsh: Change integer option parsing functions to return tri-state information.
This is needed to detect situations when optional argument was specified with non-integer value: '--int-opt foo'. --- tools/virsh.c | 46 +++++++++++++++++++++++++++------------------- 1 files changed, 27 insertions(+), 19 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 62fca17..e5093a2 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -252,13 +252,14 @@ static const vshCmdGrp *vshCmdGrpSearch(const char *grpname); static int vshCmdGrpHelp(vshControl *ctl, const char *name); static vshCmdOpt *vshCommandOpt(const vshCmd *cmd, const char *name); -static int vshCommandOptInt(const vshCmd *cmd, const char *name, int *found); +static int vshCommandOptInt(const vshCmd *cmd, const char *name, int *found, + int *opt_found); static unsigned long vshCommandOptUL(const vshCmd *cmd, const char *name, - int *found); + int *found, int *opt_found); static char *vshCommandOptString(const vshCmd *cmd, const char *name, int *found); static long long vshCommandOptLongLong(const vshCmd *cmd, const char *name, - int *found); + int *found, int *opt_found); static int vshCommandOptBool(const vshCmd *cmd, const char *name); static char *vshCommandOptArgv(const vshCmd *cmd, int count); @@ -1602,7 +1603,7 @@ cmdSchedInfoUpdate(vshControl *ctl, const vshCmd *cmd, param->type == VIR_DOMAIN_SCHED_FIELD_UINT && vshCommandOptBool(cmd, "weight")) { int val; - val = vshCommandOptInt(cmd, "weight", &found); + val = vshCommandOptInt(cmd, "weight", &found, NULL); if (!found) { vshError(ctl, "%s", _("Invalid value of weight")); return -1; @@ -1617,7 +1618,7 @@ cmdSchedInfoUpdate(vshControl *ctl, const vshCmd *cmd, param->type == VIR_DOMAIN_SCHED_FIELD_UINT && vshCommandOptBool(cmd, "cap")) { int val; - val = vshCommandOptInt(cmd, "cap", &found); + val = vshCommandOptInt(cmd, "cap", &found, NULL); if (!found) { vshError(ctl, "%s", _("Invalid value of cap")); return -1; @@ -2300,7 +2301,7 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd) if (!vshConnectionUsability(ctl, ctl->conn)) return FALSE; - cell = vshCommandOptInt(cmd, "cellno", &cell_given); + cell = vshCommandOptInt(cmd, "cellno", &cell_given, NULL); all_given = vshCommandOptBool(cmd, "all"); if (all_given && cell_given) { @@ -2726,7 +2727,7 @@ cmdVcpupin(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return FALSE; - vcpu = vshCommandOptInt(cmd, "vcpu", &vcpufound); + vcpu = vshCommandOptInt(cmd, "vcpu", &vcpufound, NULL); if (!vcpufound) { vshError(ctl, "%s", _("vcpupin: Invalid or missing vCPU number.")); virDomainFree(dom); @@ -2862,7 +2863,7 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return FALSE; - count = vshCommandOptInt(cmd, "count", &count); + count = vshCommandOptInt(cmd, "count", &count, NULL); if (!flags) { if (virDomainSetVcpus(dom, count) != 0) { @@ -2927,7 +2928,7 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return FALSE; - kilobytes = vshCommandOptUL(cmd, "kilobytes", NULL); + kilobytes = vshCommandOptUL(cmd, "kilobytes", NULL, NULL); if (kilobytes <= 0) { virDomainFree(dom); vshError(ctl, _("Invalid value of %lu for memory size"), kilobytes); @@ -2984,7 +2985,7 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return FALSE; - kilobytes = vshCommandOptInt(cmd, "kilobytes", &kilobytes); + kilobytes = vshCommandOptInt(cmd, "kilobytes", &kilobytes, NULL); if (kilobytes <= 0) { virDomainFree(dom); vshError(ctl, _("Invalid value of %d for memory size"), kilobytes); @@ -3056,22 +3057,22 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd) return FALSE; hard_limit = - vshCommandOptLongLong(cmd, "hard-limit", NULL); + vshCommandOptLongLong(cmd, "hard-limit", NULL, NULL); if (hard_limit) nparams++; soft_limit = - vshCommandOptLongLong(cmd, "soft-limit", NULL); + vshCommandOptLongLong(cmd, "soft-limit", NULL, NULL); if (soft_limit) nparams++; swap_hard_limit = - vshCommandOptLongLong(cmd, "swap-hard-limit", NULL); + vshCommandOptLongLong(cmd, "swap-hard-limit", NULL, NULL); if (swap_hard_limit) nparams++; min_guarantee = - vshCommandOptLongLong(cmd, "min-guarantee", NULL); + vshCommandOptLongLong(cmd, "min-guarantee", NULL, NULL); if (min_guarantee) nparams++; @@ -3670,7 +3671,7 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool (cmd, "live")) live_flag = TRUE; - timeout = vshCommandOptInt(cmd, "timeout", &found); + timeout = vshCommandOptInt(cmd, "timeout", &found, NULL); if (found) { if (! live_flag) { vshError(ctl, "%s", _("migrate: Unexpected timeout for offline migration")); @@ -3807,7 +3808,7 @@ cmdMigrateSetMaxDowntime(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return FALSE; - downtime = vshCommandOptLongLong(cmd, "downtime", &found); + downtime = vshCommandOptLongLong(cmd, "downtime", &found, NULL); if (!found || downtime < 1) { vshError(ctl, "%s", _("migrate: Invalid downtime")); goto done; @@ -10796,7 +10797,7 @@ vshCommandOpt(const vshCmd *cmd, const char *name) * Returns option as INT */ static int -vshCommandOptInt(const vshCmd *cmd, const char *name, int *found) +vshCommandOptInt(const vshCmd *cmd, const char *name, int *found, int *opt_found) { vshCmdOpt *arg = vshCommandOpt(cmd, name); int res = 0, num_found = FALSE; @@ -10811,11 +10812,13 @@ vshCommandOptInt(const vshCmd *cmd, const char *name, int *found) } if (found) *found = num_found; + if (opt_found) + *opt_found = arg ? TRUE : FALSE; return res; } static unsigned long -vshCommandOptUL(const vshCmd *cmd, const char *name, int *found) +vshCommandOptUL(const vshCmd *cmd, const char *name, int *found, int *opt_found) { vshCmdOpt *arg = vshCommandOpt(cmd, name); unsigned long res = 0; @@ -10831,6 +10834,8 @@ vshCommandOptUL(const vshCmd *cmd, const char *name, int *found) } if (found) *found = num_found; + if (opt_found) + *opt_found = arg ? TRUE : FALSE; return res; } @@ -10858,7 +10863,8 @@ vshCommandOptString(const vshCmd *cmd, const char *name, int *found) * Returns option as long long */ static long long -vshCommandOptLongLong(const vshCmd *cmd, const char *name, int *found) +vshCommandOptLongLong(const vshCmd *cmd, const char *name, int *found, + int *opt_found) { vshCmdOpt *arg = vshCommandOpt(cmd, name); int num_found = FALSE; @@ -10869,6 +10875,8 @@ vshCommandOptLongLong(const vshCmd *cmd, const char *name, int *found) num_found = !virStrToLong_ll(arg->data, &end_p, 10, &res); if (found) *found = num_found; + if (opt_found) + *opt_found = arg ? TRUE : FALSE; return res; } -- 1.7.4
--- tools/virsh.c | 18 ++++++++++++++---- 1 files changed, 14 insertions(+), 4 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index e5093a2..c9b4c57 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2285,7 +2285,7 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd) { int func_ret = FALSE; int ret; - int cell, cell_given; + int cell, cell_given, cell_opt_given; unsigned long long memory; xmlNodePtr *nodes = NULL; unsigned long nodes_cnt; @@ -2301,7 +2301,7 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd) if (!vshConnectionUsability(ctl, ctl->conn)) return FALSE; - cell = vshCommandOptInt(cmd, "cellno", &cell_given, NULL); + cell = vshCommandOptInt(cmd, "cellno", &cell_given, &cell_opt_given); all_given = vshCommandOptBool(cmd, "all"); if (all_given && cell_given) { @@ -2310,6 +2310,11 @@ cmdFreecell(vshControl *ctl, const vshCmd *cmd) goto cleanup; } + if (!cell_given && cell_opt_given) { + vshError(ctl, "%s", _("cell number has to be a number")); + goto cleanup; + } + if (all_given) { cap_xml = virConnectGetCapabilities(ctl->conn); if (!cap_xml) { @@ -2848,7 +2853,7 @@ static int cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; - int count; + int count, opt_count; int ret = TRUE; int maximum = vshCommandOptBool(cmd, "maximum"); int config = vshCommandOptBool(cmd, "config"); @@ -2863,7 +2868,12 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return FALSE; - count = vshCommandOptInt(cmd, "count", &count, NULL); + count = vshCommandOptInt(cmd, "count", &count, &opt_count); + + if (!count && opt_count) { + vshError(ctl, "%s", _("count has to be a number")); + goto cleanup; + } if (!flags) { if (virDomainSetVcpus(dom, count) != 0) { -- 1.7.4
On 03/01/2011 03:16 AM, Michal Privoznik wrote:
This is needed to detect situations when optional argument was specified with non-integer value: '--int-opt foo'. --- tools/virsh.c | 46 +++++++++++++++++++++++++++------------------- 1 files changed, 27 insertions(+), 19 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 62fca17..e5093a2 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -252,13 +252,14 @@ static const vshCmdGrp *vshCmdGrpSearch(const char *grpname); static int vshCmdGrpHelp(vshControl *ctl, const char *name);
static vshCmdOpt *vshCommandOpt(const vshCmd *cmd, const char *name); -static int vshCommandOptInt(const vshCmd *cmd, const char *name, int *found); +static int vshCommandOptInt(const vshCmd *cmd, const char *name, int *found, + int *opt_found);
This is awkward. You now have two optional parameters, and still can't provide a default value easily. I'd much rather see this (as pointed out in https://www.redhat.com/archives/libvir-list/2011-January/msg00145.html): int vshCommandOptUL(const vshCmd *cmd, const char *name, unsigned long *value) ATTRIBUTE_NONNULL(3); Return value is <0 on failure (*value untouched), 0 when option is absent (*value untouched), and >0 on success (*value updated). Swapping the API like that also has the benefit that a client can specify a non-zero default: unsigned long value = 1024; if (vshCommandOptUL(cmd, "arg", &value) < 0) { error; return FALSE; } use value That is - the current code returns the parse value, and with your patch would provide tri-state information via two optional pointers; but my preference would be to swap things and provide a tri-state return code and put the parse value in a required pointer. Furthermore, this should be done to _all_ of the vshCommandOpt<int> family of functions. vshCommandOptString may be worth swapping for consistency, since it is also a tri-state (--option not provided, --option provided but with no [or empty] string, and --option provided with string), although I'm not as convinced on that case as I am on the optional integer parsing issue (--option not provided, --option provided but parse as integer failed, --option provided and integer was parsed in range). Fortunately, the size of your patch proves that there weren't that many places to change. I'll see about doing the work myself in the next 15 minutes... -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Michal Privoznik