From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH] libxl: Make an internal function explicitly check existence of expected paths Date: Wed, 5 Dec 2012 18:29:46 +0000 Message-ID: <50BF929A.1070606@eu.citrix.com> References: <21504ec56304ada2f093.1354732128@elijah> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <21504ec56304ada2f093.1354732128@elijah> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org I've sent another one with a "v3" to help make it clear what's going on... -George On 05/12/12 18:28, George Dunlap wrote: > # HG changeset patch > # User George Dunlap > # Date 1354732124 0 > # Node ID 21504ec56304ada2f093c30b290ac33c28381ae1 > # Parent 670b07e8d7382229639af0d1df30071e6c1ebb19 > 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 > * Disposing of the structure before returning using libxl_device_disk_displose() > > Also make the callers of the function pay attention to the error and > behave appropriately. In the case of libxl__append_disk_list_of_type(), > this means only incrementing *ndisks as the disk structures are > successfully initialized. > > v3: > * Make a failure path in libxl__device_disk_from_be() to free allocations > from half-initialized disks > * Modify libxl__append_disk_list_of_type to only update *ndisks as they are > completed. This will allow the only caller (libxl_device_disk_list()) to > clean up the completed disks properly on an error. > v2: > * Remove "Internal error", as the failure will most likely look internal > * Use LOG(ERROR...) macros for incrased prettiness > > Signed-off-by: George Dunlap > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -2169,9 +2169,9 @@ void libxl__device_disk_add(libxl__egc * > device_disk_add(egc, domid, disk, aodev, NULL, NULL); > } > > -static void libxl__device_disk_from_xs_be(libxl__gc *gc, > - const char *be_path, > - libxl_device_disk *disk) > +static int libxl__device_disk_from_xs_be(libxl__gc *gc, > + const char *be_path, > + libxl_device_disk *disk) > { > libxl_ctx *ctx = libxl__gc_owner(gc); > unsigned int len; > @@ -2179,6 +2179,7 @@ static void libxl__device_disk_from_xs_b > > libxl_device_disk_init(disk); > > + /* "params" may not be present; but everything else must be. */ > tmp = xs_read(ctx->xsh, XBT_NULL, > libxl__sprintf(gc, "%s/params", be_path), &len); > if (tmp && strchr(tmp, ':')) { > @@ -2187,21 +2188,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); > + goto cleanup; > + } > + libxl_string_to_backend(ctx, tmp, &(disk->backend)); > + > disk->vdev = xs_read(ctx->xsh, XBT_NULL, > libxl__sprintf(gc, "%s/dev", be_path), &len); > + if (!disk->vdev) { > + LOG(ERROR, "Missing xenstore node %s/dev", be_path); > + goto cleanup; > + } > + > tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf > (gc, "%s/removable", be_path)); > - > - if (tmp) > - disk->removable = atoi(tmp); > - else > - disk->removable = 0; > + if (!tmp) { > + LOG(ERROR, "Missing xenstore node %s/removable", be_path); > + goto cleanup; > + } > + disk->removable = atoi(tmp); > > tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/mode", be_path)); > + if (!tmp) { > + LOG(ERROR, "Missing xenstore node %s/mode", be_path); > + goto cleanup; > + } > if (!strcmp(tmp, "w")) > disk->readwrite = 1; > else > @@ -2209,9 +2225,18 @@ static void libxl__device_disk_from_xs_b > > tmp = libxl__xs_read(gc, XBT_NULL, > libxl__sprintf(gc, "%s/device-type", be_path)); > + if (!tmp) { > + LOG(ERROR, "Missing xenstore node %s/device-type", be_path); > + goto cleanup; > + } > disk->is_cdrom = !strcmp(tmp, "cdrom"); > > disk->format = LIBXL_DISK_FORMAT_UNKNOWN; > + > + return 0; > +cleanup: > + libxl_device_disk_dispose(disk); > + return ERROR_FAIL; > } > > int libxl_vdev_to_device_disk(libxl_ctx *ctx, uint32_t domid, > @@ -2237,9 +2262,7 @@ int libxl_vdev_to_device_disk(libxl_ctx > if (!path) > goto out; > > - libxl__device_disk_from_xs_be(gc, path, disk); > - > - rc = 0; > + rc = libxl__device_disk_from_xs_be(gc, path, disk); > out: > GC_FREE; > return rc; > @@ -2256,6 +2279,8 @@ static int libxl__append_disk_list_of_ty > char **dir = NULL; > unsigned int n = 0; > libxl_device_disk *pdisk = NULL, *pdisk_end = NULL; > + int rc=0; > + int initial_disks = *ndisks; > > be_path = libxl__sprintf(gc, "%s/backend/%s/%d", > libxl__xs_get_dompath(gc, 0), type, domid); > @@ -2266,17 +2291,19 @@ static int libxl__append_disk_list_of_ty > if (tmp == NULL) > return ERROR_NOMEM; > *disks = tmp; > - pdisk = *disks + *ndisks; > - *ndisks += n; > - pdisk_end = *disks + *ndisks; > + pdisk = *disks + initial_disks; > + pdisk_end = *disks + initial_disks + n; > for (; pdisk < pdisk_end; pdisk++, dir++) { > const char *p; > p = libxl__sprintf(gc, "%s/%s", be_path, *dir); > - libxl__device_disk_from_xs_be(gc, p, pdisk); > + if ((rc=libxl__device_disk_from_xs_be(gc, p, pdisk))) > + goto out; > pdisk->backend_domid = 0; > + *ndisks += 1; > } > } > - return 0; > +out: > + return rc; > } > > libxl_device_disk *libxl_device_disk_list(libxl_ctx *ctx, uint32_t domid, int *num)