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

[Ján Tomko]

On Sat, May 26, 2018 at 11:00:26PM +0200, Fabiano Fidêncio wrote:
>From: Fabiano Fidêncio <fidencio at redhat.com>
>
>Signed-off-by: Fabiano Fidêncio <fabiano at fidencio.org>
>---
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180527/dc60e33f/attachment-0001.sig>