| Message ID | 1453756017-8747-3-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 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.
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.
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;
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(-)