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

On Thu, Mar 05, 2015 at 16:10:28 +0100, Pavel Hrdina wrote: > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; Commit message is still too sparse ...

> --- > tools/virsh-domain.c | 83 ++++++++++++++++++++++++---------------------------- > 1 file changed, 38 insertions(+), 45 deletions(-) > > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c > index 55c269c..3836a1b 100644 > --- a/tools/virsh-domain.c > +++ b/tools/virsh-domain.c > @@ -8255,6 +8255,22 @@ static const vshCmdOptDef opts_memtune[] = { > {.name = NULL} > }; > > +/** > + * vshMemtuneGetSize > + * > + * @cmd: pointer to vshCmd > + * @name: name of a parameter for which we would like to get a value > + * @value: pointer to variable where the value will be stored > + * > + * This function will parse virsh command line in order to load a value of > + * specified parameter. If the value is -1 we will handle it as unlimited and > + * use VIR_DOMAIN_MEMORY_PARAM_UNLIMITED instead. > + * > + * Returns: > + * >0 if option found and valid (@value updated) > + * 0 if option not found and not required (@value untouched) > + * <0 in all other cases (@value untouched) The last one is not true. If the scaling function fails @value is touched but the function returns -1. I'd drop the statements altogether.

> + */ > static int > vshMemtuneGetSize(const vshCmd *cmd, const char *name, long long *value) > { > @@ -8276,7 +8292,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; > } > > static bool > @@ -8294,6 +8310,7 @@ cmdMemtune(vshControl *ctl, const vshCmd *cmd) > bool current = vshCommandOptBool(cmd, "current"); > bool config = vshCommandOptBool(cmd, "config"); > bool live = vshCommandOptBool(cmd, "live"); > + int rc; > > VSH_EXCLUSIVE_OPTIONS_VAR(current, live); > VSH_EXCLUSIVE_OPTIONS_VAR(current, config); > @@ -8306,50 +8323,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; > - } > - > - 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; > - } > - > - 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; > - } > +#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; \ > + } \ > + > + > + PARSE_MEMTUNE_PARAM("hard-limit", hard_limit, VIR_DOMAIN_MEMORY_HARD_LIMIT); > + PARSE_MEMTUNE_PARAM("soft-limit", soft_limit, VIR_DOMAIN_MEMORY_SOFT_LIMIT); I just realized that the 'hard_limit', 'soft_limit' ... variables are used just as temp variables in the macro. You could get rid of them and use a single temp variable instead. > + PARSE_MEMTUNE_PARAM("swap-hard-limit", swap_hard_limit, > + VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT); Which would make these fit on one line. > + PARSE_MEMTUNE_PARAM("min-guarantee", min_guarantee, > + VIR_DOMAIN_MEMORY_MIN_GUARANTEE); > + > +#undef PARSE_MEMTUNE_PARAM > > if (nparams == 0) { > /* get the number of memory parameters */ ACK if you drop the parenthesed section in the comment as pointed above. The rest is not mandatory, although I'd prefer if the patch had a proper commit message. Peter

-- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list