Re: [Xen-devel] [PATCH v3 3/4] libxl: add libxl_get_parameters() function

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, May 09, 2019 at 05:41:27PM +0200, Vasilis Liaskovitis wrote:
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index ec71574e99..124033e5a3 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -669,6 +669,21 @@ int libxl_set_parameters(libxl_ctx *ctx, char *params)
>      return 0;
>  }
>  
> +int libxl_get_parameters(libxl_ctx *ctx, char *params, char *values)
> +{
> +    int ret;
> +    GC_INIT(ctx);
> +
> +    ret = xc_get_parameters(ctx->xch, params, values);

Please name the variable `r' instead of 'ret'. See CODING_STYLE as for
why.

> +    if (ret < 0) {
> +        LOGEV(ERROR, ret, "getting parameters");

LOGEV takes `errno' as second parameter, the value of `ret' seems to be
-1 or 0, and xc_get_parameters should set `errno'.  So, using the macro
LOGE() should be enough.

> +        GC_FREE;
> +        return ret;//ERROR_FAIL;

Almost! I think ERROR_FAIL should be returned here, not a return value
from a libxc call.

> +    }
> +    GC_FREE;
> +    return 0;

Instead of having to write GC_FREE twice, you could:

    if (r < 0) {
       LOG...
       rc = ERROR_FAIL;
       goto out;
    }
    rc=0;
  out:
    GC_FREE;
    return rc;

> +}
> +

Thanks,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel