[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v5 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest

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.

+                   (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) {
+        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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.