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 14:34:58 +0000 Message-ID: <50BF5B92.6000702@eu.citrix.com> References: <1354631744.15296.8.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1354631744.15296.8.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org 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 >> # 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. > >