Re: [libvirt] [PATCH 1/3] virsh: fix memtune to also accept 0 as valid value

On Wed, Mar 04, 2015 at 17:17:05 +0100, Pavel Hrdina wrote: Commit message is too sparse. > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1146539 Also I doubt that this on itself resolves this bug.

> > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; > --- > tools/virsh-domain.c | 62 +++++++++++++++++----------------------------------- > 1 file changed, 20 insertions(+), 42 deletions(-) > > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c > index 55c269c..773f9f1 100644 > --- a/tools/virsh-domain.c > +++ b/tools/virsh-domain.c > @@ -8276,7 +8276,7 @@ vshMemtuneGetSize(const vshCmd *cmd, const char *name, long long *value) > if (virScaleInteger(&tmp, end, 1024, LLONG_MAX) < 0) > return -1; > *value = VIR_DIV_UP(tmp, 1024); > - return 0; > + return 1; Now that the return values from this function actually make sense it would be worth adding a comment what they mean.

> } > > static bool > @@ -8294,6 +8294,7 @@ cmdMemtune(vshControl *ctl, const vshCmd *cmd) > bool current = vshCommandOptBool(cmd, "current"); > bool config = vshCommandOptBool(cmd, "config"); > bool live = vshCommandOptBool(cmd, "live"); > + int rc = 0; rc doesn't need to be initialized as the macro below sets and tests it. > > VSH_EXCLUSIVE_OPTIONS_VAR(current, live); > VSH_EXCLUSIVE_OPTIONS_VAR(current, config); > @@ -8306,50 +8307,26 @@ cmdMemtune(vshControl *ctl, const vshCmd *cmd) > if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) > return false; > > - if (vshMemtuneGetSize(cmd, "hard-limit", &hard_limit) < 0 || > - vshMemtuneGetSize(cmd, "soft-limit", &soft_limit) < 0 || > - vshMemtuneGetSize(cmd, "swap-hard-limit", &swap_hard_limit) < 0 || > - vshMemtuneGetSize(cmd, "min-guarantee", &min_guarantee) < 0) { > - vshError(ctl, "%s", > - _("Unable to parse integer parameter")); > - goto cleanup; > - } > +#define PARSE_MEMTUNE_PARAM(NAME, VALUE, FIELD) \ > + if ((rc = vshMemtuneGetSize(cmd, NAME, &VALUE)) < 0) { \ > + vshError(ctl, "%s", _("Unable to parse integer parameter 'NAME'")); \ > + goto cleanup; \ > + } \ > + if (rc == 1) { \ > + if (virTypedParamsAddULLong(&params, &nparams, &maxparams, \ > + FIELD, VALUE) < 0) \ > + goto save_error; \ > + } \ > > - if (hard_limit) { > - if (hard_limit == -1) > - hard_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; > - if (virTypedParamsAddULLong(&params, &nparams, &maxparams, > - VIR_DOMAIN_MEMORY_HARD_LIMIT, > - hard_limit) < 0) > - goto save_error; > - } > > - if (soft_limit) { > - if (soft_limit == -1) > - soft_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; > - if (virTypedParamsAddULLong(&params, &nparams, &maxparams, > - VIR_DOMAIN_MEMORY_SOFT_LIMIT, > - soft_limit) < 0) > - goto save_error; > - } > - > - if (swap_hard_limit) { > - if (swap_hard_limit == -1) > - swap_hard_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; > - if (virTypedParamsAddULLong(&params, &nparams, &maxparams, > - VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT, > - swap_hard_limit) < 0) > - goto save_error; > - } > + PARSE_MEMTUNE_PARAM("hard-limit", hard_limit, VIR_DOMAIN_MEMORY_HARD_LIMIT); > + PARSE_MEMTUNE_PARAM("soft-limit", soft_limit, VIR_DOMAIN_MEMORY_SOFT_LIMIT); > + PARSE_MEMTUNE_PARAM("swap-hard-limit", swap_hard_limit, > + VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT); > + PARSE_MEMTUNE_PARAM("min-guarantee", min_guarantee, > + VIR_DOMAIN_MEMORY_MIN_GUARANTEE); > > - if (min_guarantee) { > - if (min_guarantee == -1) > - min_guarantee = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; > - if (virTypedParamsAddULLong(&params, &nparams, &maxparams, > - VIR_DOMAIN_MEMORY_MIN_GUARANTEE, > - min_guarantee) < 0) > - goto save_error; > - } > +#undef PARSE_MEMTUNE_PARAM Um ... > > if (nparams == 0) { > /* get the number of memory parameters */ > @@ -8390,6 +8367,7 @@ cmdMemtune(vshControl *ctl, const vshCmd *cmd) > ret = true; > > cleanup: > +#undef PARSE_MEMTUNE_PARAM ... why are you undefing it twice?

> virTypedParamsFree(params, nparams); > virDomainFree(dom); > return ret; Also the virsh man page would benefit from adding a statement that -1 should be used as unlimited.

Peter