Re: [PATCH 3/4] libxl/remus: Change the assert for info to an return

From: Wei Liu <wei.liu2@citrix.com>
To: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: wei.liu2@citrix.com, ian.campbell@citrix.com,
	Wen Congyang <wency@cn.fujitsu.com>,
	Ian.Jackson@eu.citrix.com, xen-devel@lists.xenproject.org,
	Yang Hongyang <hongyang.yang@easystack.cn>
Subject: Re: [PATCH 3/4] libxl/remus: Change the assert for info to an return
Date: Mon, 1 Feb 2016 12:13:53 +0000	[thread overview]
Message-ID: <20160201121353.GU25660@citrix.com> (raw)
In-Reply-To: <1453843860-29591-4-git-send-email-konrad.wilk@oracle.com>

On Tue, Jan 26, 2016 at 04:30:59PM -0500, Konrad Rzeszutek Wilk wrote:
> The assert(info) is after quite a lot of manipulations
> on 'info' - which makes the assert pointless because if
> info was NULL it would have crashed earlier.
> 

This sounds sensible.

> Remove it and make it an return. Also since most of the
> error paths are for the same rc, unify them.
> 
> CC: Wen Congyang <wency@cn.fujitsu.com>
> CC: Yang Hongyang <hongyang.yang@easystack.cn>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/libxl/libxl.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 548e2e2..228b241 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -847,13 +847,14 @@ int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
>  {
>      AO_CREATE(ctx, domid, ao_how);
>      libxl__domain_suspend_state *dss;
> -    int rc;
> +    int rc = ERROR_FAIL;

This violates coding style:

 83   * If the function is to return a libxl error value, `rc' is
 84     used to contain the error code, but it is NOT initialised:
 85             int rc;

>  
>      libxl_domain_type type = libxl__domain_type(gc, domid);
> -    if (type == LIBXL_DOMAIN_TYPE_INVALID) {
> -        rc = ERROR_FAIL;
> +    if (type == LIBXL_DOMAIN_TYPE_INVALID)
> +        goto out;
> +
> +    if (!info)
>          goto out;
> -    }
>  
>      libxl_defbool_setdefault(&info->allow_unsafe, false);
>      libxl_defbool_setdefault(&info->blackhole, false);
> @@ -867,7 +868,6 @@ int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
>           !libxl_defbool_val(info->diskbuf))) {
>          LOG(ERROR, "Unsafe mode must be enabled to replicate to /dev/null,"
>                     "disable network buffering and disk replication");
> -        rc = ERROR_FAIL;
>          goto out;
>      }
>  
> @@ -883,8 +883,7 @@ int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
>      dss->debug = 0;
>      dss->remus = info;
>  
> -    assert(info);
> -
> +    rc = 0;
>      /* Point of no return */
>      libxl__remus_setup(egc, dss);
>      return AO_INPROGRESS;
> -- 
> 2.1.0
> 