Re: [libvirt] [PATCHv2 2/3] vmx: convert to typesafe virConf accessors

On Sat, May 26, 2018 at 11:00:26PM +0200, Fabiano Fidêncio wrote: > > From: Fabiano Fidêncio <fidencio(a)redhat.com&gt; > > Signed-off-by: Fabiano Fidêncio <fabiano(a)fidencio.org&gt; > --- > src/vmx/vmx.c | 196 > ++++++++++++++++++++++------------------------------------ > 1 file changed, 73 insertions(+), 123 deletions(-) > > diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c > index df6a58a474..54542c29a6 100644 > --- a/src/vmx/vmx.c > +++ b/src/vmx/vmx.c > @@ -808,47 +786,35 @@ static int > virVMXGetConfigLong(virConfPtr conf, const char *name, long long *number, > long long default_, bool optional) > { > - virConfValuePtr value; > + char *string = NULL; > + int result; Usually we use 'ret' as the name of the variable that will eventually be used to return from the function and 'rc' to store the value returned by the function we call (where they return more values than 0/-1): int ret = -1; int rc;

> > *number = default_; > - value = virConfGetValue(conf, name); > > - if (value == NULL) { > - if (optional) { > - return 0; > - } else { > - virReportError(VIR_ERR_INTERNAL_ERROR, > - _("Missing essential config entry '%s'"), > name); > - return -1; > - } > - } > + result = virVMXGetConfigStringHelper(conf, name, &string, optional); > + if (result <= 0) > + return result; This would become: rc = GetStringHelper(); if (rc <= 0) return rc;

> > - if (value->type == VIR_CONF_STRING) { > - if (value->str == NULL) { > - if (optional) { > - return 0; > - } else { > - virReportError(VIR_ERR_INTERNAL_ERROR, > - _("Missing essential config entry '%s'"), > name); > - return -1; > - } > - } > + if (STRCASEEQ(string, "unlimited")) { > + *number = -1; > + result = 0; > + goto cleanup; > + } > > - if (STRCASEEQ(value->str, "unlimited")) { > - *number = -1; > - } else if (virStrToLong_ll(value->str, NULL, 10, number) < 0) { > - virReportError(VIR_ERR_INTERNAL_ERROR, > - _("Config entry '%s' must represent an integer > value"), > - name); > - return -1; > - } > - } else { if you leave this 'else' here, there's no need to touch 'ret' or goto cleanup above.

> + if (virStrToLong_ll(string, NULL, 10, number) < 0) { > virReportError(VIR_ERR_INTERNAL_ERROR, > - _("Config entry '%s' must be a string"), name); > - return -1; > + _("Config entry '%s' must represent an integer value"), > + name); > + result = -1; This adjustment would also be unnecessary. > + goto cleanup; > } > > - return 0; > + result = 0; > + (NB: while many of the functions in this file use 'result' instead of 'ret', I'd suggest to slowly start replacing the old name instead of striving for consistency. Most of them are the Parse functions that already return a device definition via a pointer argument and the 0/-1 they return is redundant, because it can be replaced by a NULL check on the pointer)

Jano Jano