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

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



On 17/04/13 10:48, Roger Pau Monne wrote:
+    if (!libxl_usb_path) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot allocate create paths");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
       noperm[0].id = 0;
       noperm[0].perms = XS_PERM_NONE;

@@ -475,6 +482,9 @@ retry_transaction:
       xs_rm(ctx->xsh, t, libxl_path);
       libxl__xs_mkdir(gc, t, libxl_path, noperm, ARRAY_SIZE(noperm));

+    xs_rm(ctx->xsh, t, libxl_usb_path);
You are missing the error check, there's also a helper for xs_rm,
libxl__xs_rm_checked.
I'm just following suit with what all the other xs_rm's do here.  I
think it's actually expected that there will *not* be such a path, but
that it's just cleaning up to be sure.
libxl__xs_rm_checked will not return an error if xs_rm errno is ENOENT,
it will only return an error if something really bad happened.

OK -- well, if you look at that whole function, there are dozens of things removed and added with no checking. I think it's counter-productive to add checking in only one place -- it gives people the impression that this is something new and different, when in fact it's exactly the same as everything else.

The alternate would be to add a patch to add error-checking to every single one. But I think at this point in the release cycle, that's too risky. It's a lot of code churn for no clear benefit, and there's the possibility we'll introduce a bug that will be discovered at the 11th hour (or in fact after the release).

I can add cleaning up this function in my to-do list for the 4.4 dev cycle if you want.



+
+out:
+    return rc;
+}
+
+int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid,
+                         libxl_device_usb *usbdev,
+                         const libxl_asyncop_how *ao_how)
+{
+    AO_CREATE(ctx, domid, ao_how);
+    int rc;
+    rc = libxl__device_usb_add(gc, domid, usbdev);
+    libxl__ao_complete(egc, ao, rc);
+    return AO_INPROGRESS;
+}
+
+/*
+ * USB remove
+ */
+static int do_usb_remove(libxl__gc *gc, uint32_t domid,
+                         libxl__device_usb *usbdev)
+{
+    int rc;
+
+    switch (usbdev->protocol) {
+    case LIBXL_USB_PROTOCOL_DEVICEMODEL:
+        switch (libxl__device_model_version_running(gc, domid)) {
+        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+            LOG(ERROR, "usb-remove not yet implemented for qemu-traditional");
+            return ERROR_INVAL;
+        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+            if ( (rc = libxl__qmp_usb_remove(gc, domid, usbdev)) < 0 )
Spaces
How important are the spaces?  Most of the time I think it makes it
easier to read, in this case particularly so.
It's in libxl CODING_STYLE, since you are already caching the return
value, why don't you split the line:

rc = libxl__qmp_usb_remove(gc, domid, usbdev);
if (rc < 0)
     ...

This makes it even easier to read IMHO.

+out:
+    return rc;
+}
+
+int libxl_device_usb_remove(libxl_ctx *ctx, uint32_t domid,
+                            libxl_device_usb *usbdev,
+                            const libxl_asyncop_how *ao_how)
+{
+    AO_CREATE(ctx, domid, ao_how);
+    int rc;
+    rc = libxl__device_usb_remove(gc, domid, usbdev);
+    libxl__ao_complete(egc, ao, rc);
+    return AO_INPROGRESS;
+}
+
+
+libxl_device_usb *libxl_device_usb_list(libxl_ctx *ctx,
+                                        uint32_t domid, int *nu

m)
+{
+    GC_INIT(ctx);
+    libxl__device_usb *devint;
+    libxl_device_usb *devext = NULL;
+    int n = 0, i, rc;
+
+    rc = get_assigned_devices(gc, domid, &devint, &n);
+    if ( rc ) {
+        LOG(ERROR, "Could not get assigned devices");
+        goto out;
+    }
+
+    if(n == 0)
+        goto out;
+
+    devext = calloc(n, sizeof(libxl_device_usb));
libxl__calloc, also you seem to leak this allocation, which will be
solved by the use of libxl__calloc.
This isn't a leak -- this is giving the list to an external caller, who
is responsible to free the list.  This follows suit with other
libxl_device_.*_list() functions, which must free() the return value
themselves. (See e.g., libxl_device_pci_list(),
libxl_device_pci_assignable_list(), libxl_device_nic_list(), &c.)
OK, then you can use libxl__calloc with NOGC.

Sure, I guess. :-)

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

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