[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.

Yes, you're right on both points.  Good catch.

+                   (uint16_t) dev->u.hostdev.hostbus,
+                   (uint16_t) dev->u.hostdev.hostaddr);
I think the style in libxl is to cuddle casts.

Ack.  I've never thought of variables as cuddly before...

+    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 :-).

Well I prefer that way -- having the 'if' on a separate line actually annoys me (mildly). "Do this and handle the error if any" seems like one operation in my mind, not two. But I don't mind changing it.

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

As far as I could determine, there is absolutely no way to query qemu to find out what devices are actually attached at the moment; that's the whole reason for storing stuff in xenstore in the first place. So yes -- if you kill the process after the qmp command has been issued but before writing something in xenstore, then usb-list will not show the device, usb-remove will return an error if you attempt to remove the device. usb-add will issue the qmp command, which will then fail.

I think this is probably true of most of the add/remove functions.

We could add an option that says "attempt to do the remove even if you don't see the device in xenstore" I suppose. But I'm not really sure how to document that in a useful way without basically describing the internal mechanics of the command.

There's nothing we can really do about usb-list though.

On the whole I think I'd rather just say, "Don't do that."


Xen-devel mailing list



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