[2/3] libxl/remus: Move the assert before the info is used.
diff mbox

Message ID 1453756017-8747-3-git-send-email-konrad.wilk@oracle.com
State New, archived
Headers show

Commit Message

Konrad Rzeszutek Wilk Jan. 25, 2016, 9:06 p.m. UTC
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.

Move it earlier so that it guards before we try using
the 'info' structure.

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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Ian Campbell Jan. 26, 2016, 4:21 p.m. UTC | #1
On Mon, 2016-01-25 at 16:06 -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.
> 
> Move it earlier so that it guards before we try using
> the 'info' structure.

That assert (wherever it is placed) is rather aggressive for an application
provided argument. ERROR_INVALID would be more normal I think.

> 
> 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 | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2bde0f5..60974cc 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -855,6 +855,8 @@ int libxl_domain_remus_start(libxl_ctx *ctx,
> libxl_domain_remus_info *info,
>          goto out;
>      }
>  
> +    assert(info);
> +
>      libxl_defbool_setdefault(&info->allow_unsafe, false);
>      libxl_defbool_setdefault(&info->blackhole, false);
>      libxl_defbool_setdefault(&info->compression, true);
> @@ -883,8 +885,6 @@ int libxl_domain_remus_start(libxl_ctx *ctx,
> libxl_domain_remus_info *info,
>      dss->debug = 0;
>      dss->remus = info;
>  
> -    assert(info);
> -
>      /* Point of no return */
>      libxl__remus_setup(egc, dss);
>      return AO_INPROGRESS;
Ian Jackson Jan. 26, 2016, 4:23 p.m. UTC | #2
Ian Campbell writes ("Re: [PATCH 2/3] libxl/remus: Move the assert before the info is used."):
> On Mon, 2016-01-25 at 16:06 -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.
> > 
> > Move it earlier so that it guards before we try using
> > the 'info' structure.
> 
> That assert (wherever it is placed) is rather aggressive for an application
> provided argument. ERROR_INVALID would be more normal I think.

I think the assert should simply be removed.  We don't assert() other
pointer parameters for non-NULL-ness.

Certainly turning null pointer bugs into ERROR_INVALID is very
unfriendly.

Ian.
Ian Jackson Feb. 1, 2016, 2:14 p.m. UTC | #3
Wei Liu writes ("Re: [PATCH 3/4] libxl/remus: Change the assert for info to an return"):
> 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.

I don't think I agree.

As I wrote in response to a previous version:

Ian Jackson writes ("Re: [PATCH 2/3] libxl/remus: Move the assert
before the info is used."):
> I think the assert should simply be removed.  We don't assert() other
> pointer parameters for non-NULL-ness.
> 
> Certainly turning null pointer bugs into ERROR_INVALID is very
> unfriendly.

Ian.

Patch
diff mbox

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2bde0f5..60974cc 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -855,6 +855,8 @@  int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
         goto out;
     }
 
+    assert(info);
+
     libxl_defbool_setdefault(&info->allow_unsafe, false);
     libxl_defbool_setdefault(&info->blackhole, false);
     libxl_defbool_setdefault(&info->compression, true);
@@ -883,8 +885,6 @@  int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
     dss->debug = 0;
     dss->remus = info;
 
-    assert(info);
-
     /* Point of no return */
     libxl__remus_setup(egc, dss);
     return AO_INPROGRESS;