Re: [Xen-devel] [PATCH] libxl: Make an internal function explicitly check existence of expected paths

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

On 04/12/12 14:35, Ian Campbell wrote:
> On Fri, 2012-11-30 at 17:13 +0000, George Dunlap wrote:
> > # HG changeset patch
> > # User George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> > # Date 1354294821 0
> > # Node ID bc2a7645dc2be4a01f2b5ee30d7453cc3d7339aa
> > # Parent  bd041b7426fe10a730994edd98708ff98ae1cb74
> > libxl: Make an internal function explicitly check existence of expected paths
> >
> > libxl__device_disk_from_xs_be() was failing without error for some
> > missing xenstore nodes in a backend, while assuming (without checking)
> > that other nodes were valid, causing a crash when another internal
> > error wrote these nodes in the wrong place.
> >
> > Make this function consistent by:
> > * Checking the existence of all nodes before using
> > * Choosing a default only when the node is not written in device_disk_add()
> > * Failing with log msg if any node written by device_disk_add() is not present
> > * Returning an error on failure
> >
> > Also make the callers of the function pay attention to the error and
> > behave appropriately.
> If libxl__device_disk_from_xs_be returns an error then someone needs to
> cleanup the partial allocations in the disk (pdev_path) probably by
> calling libxl_device_disk_dispose.
>
> It's probably easiest to do this in libxl__device_disk_from_xs_be on
> error rather than modifying all the callers?
Well, there are only two callers, only one of which (it looks like) 
needs a clean-up.  It seems like better design to make each caller do 
its own clean-up.  Let me take a look at that.
 -George

> Also libxl__append_disk_list_of_type updates *ndisks early, so if you
> abort half way through initialising the elements of the disks array
> using libxl__device_disk_from_xs_be then the caller will try and free
> some stuff which hasn't been initialised. I think the code needs to
> remember ndisks-in-array separately from
> ndisks-which-I-have-initialised, with the latter becoming the returned
> *ndisks.
>
> > v2:
> >   * Remove "Internal error", as the failure will most likely look internal
> >   * Use LOG(ERROR...) macros for incrased prettiness
> More crass?
>
> > @@ -2186,21 +2187,36 @@ static void libxl__device_disk_from_xs_b
> >       } else {
> >           disk->pdev_path = tmp;
> >       }
> > -    libxl_string_to_backend(ctx,
> > -                        libxl__xs_read(gc, XBT_NULL,
> > -                                       libxl__sprintf(gc, "%s/type", be_path)),
> > -                        &(disk->backend));
> > +
> > +
> > +    tmp = libxl__xs_read(gc, XBT_NULL,
> > +                         libxl__sprintf(gc, "%s/type", be_path));
> > +    if (!tmp) {
> > +        LOG(ERROR, "Missing xenstore node %s/type", be_path);
> > +        return ERROR_FAIL;
> > +    }
> I've just remembered about libxl__xs_read_checked which effectively
> implements the error reporting for you.
>
> Oh, but it accepts ENOENT, so not quite what you need -- nevermind!
>
> Ian.

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