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