Re: [Xen-devel] [PATCH 2/3] libxl/remus: Move the assert before the info is used. [and 1 more messages]

[Ian Jackson]

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.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/3] libxl/remus: Move the assert before the info is used.

[Ian Campbell]

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 
> CC: Yang Hongyang 
> Signed-off-by: Konrad Rzeszutek Wilk 
> ---
>  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(>allow_unsafe, false);
>  libxl_defbool_setdefault(>blackhole, false);
>  libxl_defbool_setdefault(>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;

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/3] libxl/remus: Move the assert before the info is used.

[Ian Jackson]

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.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 2/3] libxl/remus: Move the assert before the info is used.

[Konrad Rzeszutek Wilk]

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 
CC: Yang Hongyang 
Signed-off-by: Konrad Rzeszutek Wilk 
---
 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(>allow_unsafe, false);
 libxl_defbool_setdefault(>blackhole, false);
 libxl_defbool_setdefault(>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;
-- 
2.4.3


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel