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?)
