From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v5 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest Date: Fri, 19 Apr 2013 11:53:07 +0100 Message-ID: <51712213.4040508@eu.citrix.com> References: <1366296408-5660-1-git-send-email-george.dunlap@eu.citrix.com> <20848.12940.847860.437708@mariner.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: <20848.12940.847860.437708@mariner.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 Jackson Cc: "sstanisi@cbnco.com" , Roger Pau Monne , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 18/04/13 18:51, Ian Jackson wrote: > George Dunlap writes ("[PATCH v5 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest"): >> + /* Helpfully, libxl__xs_rm_checked() returns 0 on success... */ >> + rc = libxl__xs_rm_checked(gc, t, libxl_usb_path); >> + if (rc) goto out; >> + /* ...but libxl__xs_mkdir() returns 1 on success, 0 on failure. */ >> + rc = libxl__xs_mkdir(gc, t, libxl_usb_path, noperm, ARRAY_SIZE(noperm)); >> + if (!rc) goto out; > Is this really right ? If mkdir fails, you cause libxl__domain_make > to immediately return success. The out label expects the error code > to be in rc. I think it's a stylistic error to assign the > (deprecated) boolean return value from libxl__xs_mkdir to rc; rc would > normally contain a libxl error code. > >> + id = GCSPRINTF(HOST_USB_QDEV_ID, >> + (uint16_t) dev->u.hostdev.hostbus, >> + (uint16_t) dev->u.hostdev.hostaddr); > I think the style in libxl is to cuddle casts. > >> + switch(usbdev->type) { > Missing space. (etc.) And some overly long lines. > > Most of the rest of this looks plausible although I wish you wouldn't > write things like this: > >> + switch(usbdev->type) { >> + case LIBXL_DEVICE_USB_TYPE_HOSTDEV: >> + if ((rc = read_hostdev_xenstore_entry(gc, path, usbdev)) < 0) >> + goto out; > I guess old habits are hard to break :-). > > I haven't checked the semantics and timing of the xenstore/qmp > protocol in detail. But let me ask a question: > > Suppose attempted attach or remove of a usb device fails halfway > through (eg, the user presses ^C and xl just stops since it doesn't > handle SIGINT). Is the situation now (a) reported in the usb device > listing (b) capable of being cleaned up with a remove operation (c) > tidied up on domain destroy ? (Is this true at all times - ie does it > never go through a state where things won't be cleaned up?) Re domain destroy: AFAIK everything is completely cleaned up on domain destroy. All lf the xenstore directories are removed, and qemu will release the device when it shuts down. This actually has been tested when I messed up something in storing the xenstore stuff; I didn't have to reboot the host, just shutdown the guest, re-build the library, start the guest again and re-test. -George