Re: [PATCH v5 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk

[Ian Campbell <Ian.Campbell@citrix.com>]

On Mon, 2012-05-14 at 14:04 +0100, Stefano Stabellini wrote:
> On Fri, 4 May 2012, Ian Jackson wrote:
> > Stefano Stabellini writes ("[PATCH v5 2/8] libxl: libxl__device_disk_local_attach return a new libxl_device_disk"):
> > > Introduce a new libxl_device_disk** parameter to
> > > libxl__device_disk_local_attach, the parameter is allocated on the gc by
> > > libxl__device_disk_local_attach. It is going to fill it with
> > > informations about the new locally attached disk.  The new
> > > libxl_device_disk should be passed to libxl_device_disk_local_detach
> > > afterwards.
> > ...
> > > diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> > > index fc771ff..55dc55c 100644
> > > --- a/tools/libxl/libxl_internal.c
> > > +++ b/tools/libxl/libxl_internal.c
> > ...
> > >              LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "locally attaching qdisk %s\n",
> > > -                       disk->pdev_path);
> > > +                       in_disk->pdev_path);
> > 
> > I think in_disk->pdev_path can be NULL here ?
> 
> It cannot, unless a wrong configuration was provided.

so it can?

> In that case we'll fail to open the empty disk later on.

But logging the NULL pointer doesn't seem right. If this case is invalid
we should detect it as such and error out, not carry on until some other
secondary error causes us to abort.

Ian.