From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest Date: Wed, 17 Apr 2013 10:36:15 +0100 Message-ID: <516E6D0F.4090606@eu.citrix.com> References: <1365706317-5368-1-git-send-email-george.dunlap@eu.citrix.com> <516D91AE.4030600@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <516D91AE.4030600@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Roger Pau Monne Cc: "sstanisi@cbnco.com" , Ian Jackson , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 16/04/13 19:00, Roger Pau Monne wrote: > On 11/04/13 20:51, George Dunlap wrote: >> This patch exposes a generic interface which can be expanded in the >> future to implement USB for PVUSB, qemu, and stubdoms. It can also be >> extended to include other types of USB other than host USB (for example, >> tablets, mice, or keyboards). >> >> For each device removed or added, one of two protocols is available: >> * PVUSB >> * qemu (DEVICEMODEL) >> >> The caller can additionally specify "AUTO", in which case the library will >> try to determine the best protocol automatically. >> >> At the moment, the only protocol implemented is DEVICEMODEL, and the only >> device type impelmented is HOSTDEV. >> >> This uses the qmp functionality, and is thus only available for >> qemu-xen, not qemu-traditional. >> >> Signed-off-by: George Dunlap >> CC: Ian Jackson >> CC: Roger Pau Monne >> CC: sstanisi@cbnco.com > I'm not familiar with the QMP interface, so I just commented the general > libxl parts of the patch. Thanks -- this will be helpful. > >> --- >> tools/libxl/Makefile | 2 +- >> tools/libxl/libxl.h | 35 +++ >> tools/libxl/libxl_create.c | 12 +- >> tools/libxl/libxl_internal.h | 18 ++ >> tools/libxl/libxl_qmp.c | 66 +++++ >> tools/libxl/libxl_types.idl | 22 ++ >> tools/libxl/libxl_usb.c | 531 ++++++++++++++++++++++++++++++++++++++++ >> tools/ocaml/libs/xl/genwrap.py | 1 + >> 8 files changed, 685 insertions(+), 2 deletions(-) >> create mode 100644 tools/libxl/libxl_usb.c >> >> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile >> index 2984051..866960a 100644 >> --- a/tools/libxl/Makefile >> +++ b/tools/libxl/Makefile >> @@ -74,7 +74,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \ >> libxl_internal.o libxl_utils.o libxl_uuid.o \ >> libxl_json.o libxl_aoutils.o libxl_numa.o \ >> libxl_save_callout.o _libxl_save_msgs_callout.o \ >> - libxl_qmp.o libxl_event.o libxl_fork.o $(LIBXL_OBJS-y) >> + libxl_qmp.o libxl_event.o libxl_fork.o libxl_usb.o $(LIBXL_OBJS-y) >> LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o >> >> $(LIBXL_OBJS): CFLAGS += $(CFLAGS_LIBXL) -include $(XEN_ROOT)/tools/config.h >> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h >> index d18d22c..2c60cc2 100644 >> --- a/tools/libxl/libxl.h >> +++ b/tools/libxl/libxl.h >> @@ -75,6 +75,11 @@ >> #define LIBXL_HAVE_FIRMWARE_PASSTHROUGH 1 >> >> /* >> + * FIXME: Update comment >> + */ >> +#define LIBXL_HAVE_USB 1 >> + >> +/* >> * libxl ABI compatibility >> * >> * The only guarantee which libxl makes regarding ABI compatibility >> @@ -735,6 +740,36 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk, >> const libxl_asyncop_how *ao_how) >> LIBXL_EXTERNAL_CALLERS_ONLY; >> >> +/* >> + * USB >> + * >> + * For each device removed or added, one of three protocols is available: >> + * - PVUSB >> + * - qemu (DEVICEMODEL) >> + * >> + * The caller can additionally specify "AUTO", in which case the library will >> + * try to determine the best protocol automatically. >> + * >> + * At the moment, the only protocol implemented is DEVICEMODEL, and the only >> + * device type impelmented is HOSTDEV. >> + * >> + * This uses the qmp functionality, and is thus only available for >> + * qemu-xen, not qemu-traditional. >> + * >> + */ >> +#define LIBXL_DEVICE_USB_BACKEND_DEFAULT (-1) >> +int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid, >> + libxl_device_usb *dev, >> + const libxl_asyncop_how *ao_how) >> + LIBXL_EXTERNAL_CALLERS_ONLY; >> +int libxl_device_usb_remove(libxl_ctx *ctx, uint32_t domid, >> + libxl_device_usb *dev, >> + const libxl_asyncop_how *ao_how) >> + LIBXL_EXTERNAL_CALLERS_ONLY; >> +libxl_device_usb *libxl_device_usb_list(libxl_ctx *ctx, uint32_t domid, >> + int *num) >> + LIBXL_EXTERNAL_CALLERS_ONLY; >> + >> /* Network Interfaces */ >> int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic, >> const libxl_asyncop_how *ao_how) >> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c >> index 30a4507..c620d6d 100644 >> --- a/tools/libxl/libxl_create.c >> +++ b/tools/libxl/libxl_create.c >> @@ -391,7 +391,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_create_info *info, >> libxl_ctx *ctx = libxl__gc_owner(gc); >> int flags, ret, rc, nb_vm; >> char *uuid_string; >> - char *dom_path, *vm_path, *libxl_path; >> + char *dom_path, *vm_path, *libxl_path, *libxl_usb_path; >> struct xs_permissions roperm[2]; >> struct xs_permissions rwperm[1]; >> struct xs_permissions noperm[1]; >> @@ -452,6 +452,13 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_create_info *info, >> goto out; >> } >> >> + libxl_usb_path = libxl__sprintf(gc, "%s/usb", libxl_path); > I would recommend to use GCSPRINTF instead, it already checks for errors > and allows for smaller lines (altought you don't have to split the line > here) I'm not a fan of ugly macros, but sure I can do that. :-) > >> + 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_mkdir(gc, t, libxl_usb_path, noperm, ARRAY_SIZE(noperm)); > Error checking > >> + >> xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/vm", dom_path), vm_path, strlen(vm_path)); >> rc = libxl__domain_rename(gc, *domid, 0, info->name, t); >> if (rc) >> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h >> index 3ba3a21..d2e00fa 100644 >> --- a/tools/libxl/libxl_internal.h >> +++ b/tools/libxl/libxl_internal.h >> @@ -1412,6 +1412,24 @@ _hidden int libxl__qmp_save(libxl__gc *gc, int domid, const char *filename); >> /* Set dirty bitmap logging status */ >> _hidden int libxl__qmp_set_global_dirty_log(libxl__gc *gc, int domid, bool enable); >> _hidden int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid, const libxl_device_disk *disk); >> +/* Same as normal, but "translated" */ >> +typedef struct libxl__device_usb { >> + libxl_usb_protocol protocol; >> + libxl_domid target_domid; >> + libxl_domid backend_domid; >> + libxl_domid dm_domid; >> + libxl_device_usb_type type; >> + union { >> + struct { >> + int hostbus; >> + int hostaddr; >> + } hostdev; >> + } u; >> +} libxl__device_usb; >> +_hidden int libxl__qmp_usb_add(libxl__gc *gc, int domid, >> + libxl__device_usb *dev); >> +_hidden int libxl__qmp_usb_remove(libxl__gc *gc, int domid, >> + libxl__device_usb *dev); >> /* close and free the QMP handler */ >> _hidden void libxl__qmp_close(libxl__qmp_handler *qmp); >> /* remove the socket file, if the file has already been removed, >> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c >> index 644d2c0..ae55695 100644 >> --- a/tools/libxl/libxl_qmp.c >> +++ b/tools/libxl/libxl_qmp.c >> @@ -42,6 +42,7 @@ >> >> #define QMP_RECEIVE_BUFFER_SIZE 4096 >> #define PCI_PT_QDEV_ID "pci-pt-%02x_%02x.%01x" >> +#define HOST_USB_QDEV_ID "usb-hostdev-%04x.%04x" >> >> typedef int (*qmp_callback_t)(libxl__qmp_handler *qmp, >> const libxl__json_object *tree, >> @@ -929,6 +930,71 @@ int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid, >> } >> } >> >> +static int libxl__qmp_usb_hostdev_add(libxl__gc *gc, int domid, >> + libxl__device_usb *dev) >> +{ >> + libxl__json_object *args = NULL; >> + char *id; >> + >> + id = libxl__sprintf(NOGC, HOST_USB_QDEV_ID, >> + (uint16_t) dev->u.hostdev.hostbus, >> + (uint16_t) dev->u.hostdev.hostaddr); > Are you sure you need to use NOGC here? id is only used to generate the > "args" string, which in turn is using the GC. Ah no, good catch. Originally 'id' was passed back in the libxl_device_usb structure, and cleaned up with the ..._destroy() function. That no longer being the case, we definitely need to GC it. > >> + >> + qmp_parameters_add_string(gc, &args, "driver", "usb-host"); >> + QMP_PARAMETERS_SPRINTF(&args, "hostbus", "0x%x", dev->u.hostdev.hostbus); >> + QMP_PARAMETERS_SPRINTF(&args, "hostaddr", "0x%x", dev->u.hostdev.hostaddr); >> + >> + qmp_parameters_add_string(gc, &args, "id", id); >> + >> + return qmp_run_command(gc, domid, "device_add", args, NULL, NULL); >> +} >> + >> +int libxl__qmp_usb_add(libxl__gc *gc, int domid, >> + libxl__device_usb *usbdev) >> +{ >> + int rc; >> + switch(usbdev->type) { >> + case LIBXL_DEVICE_USB_TYPE_HOSTDEV: >> + rc = libxl__qmp_usb_hostdev_add(gc, domid, usbdev); >> + break; >> + default: >> + return ERROR_INVAL; >> + } >> + return rc; >> +} >> + >> + >> +static int libxl__qmp_usb_hostdev_remove(libxl__gc *gc, int domid, >> + libxl__device_usb *dev) >> +{ >> + libxl__json_object *args = NULL; >> + char *id; >> + >> + id = libxl__sprintf(NOGC, HOST_USB_QDEV_ID, >> + (uint16_t) dev->u.hostdev.hostbus, >> + (uint16_t) dev->u.hostdev.hostaddr); > The same here, I think you can safely use the GC (GCSPRINTF). > >> + >> + qmp_parameters_add_string(gc, &args, "id", id); >> + >> + return qmp_run_command(gc, domid, "device_del", args, NULL, NULL); >> +} >> + >> +int libxl__qmp_usb_remove(libxl__gc *gc, int domid, >> + libxl__device_usb *usbdev) >> +{ >> + int rc; >> + switch(usbdev->type) { >> + case LIBXL_DEVICE_USB_TYPE_HOSTDEV: >> + rc = libxl__qmp_usb_hostdev_remove(gc, domid, usbdev); >> + break; >> + default: >> + return ERROR_INVAL; >> + } >> + return rc; >> +} >> + >> + >> + >> int libxl__qmp_initializations(libxl__gc *gc, uint32_t domid, >> const libxl_domain_config *guest_config) >> { >> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl >> index 6cb6de6..9e0a435 100644 >> --- a/tools/libxl/libxl_types.idl >> +++ b/tools/libxl/libxl_types.idl >> @@ -407,6 +407,28 @@ libxl_device_vtpm = Struct("device_vtpm", [ >> ("uuid", libxl_uuid), >> ]) >> >> +libxl_device_usb_protocol = Enumeration("usb_protocol", [ >> + (0, "AUTO"), >> + (1, "PV"), >> + (2, "DEVICEMODEL"), >> + ]) >> + >> +libxl_device_usb_type = Enumeration("device_usb_type", [ >> + (1, "HOSTDEV"), >> + ]) >> + >> +libxl_device_usb = Struct("device_usb", [ >> + ("protocol", libxl_device_usb_protocol, >> + {'init_val': 'LIBXL_USB_PROTOCOL_AUTO'}), >> + ("backend_domid", libxl_domid, >> + {'init_val': 'LIBXL_DEVICE_USB_BACKEND_DEFAULT'}), >> + ("u", KeyedUnion(None, libxl_device_usb_type, "type", >> + [("hostdev", Struct(None, [ >> + ("hostbus", integer), >> + ("hostaddr", integer) ])) >> + ])) >> + ]) >> + >> libxl_domain_config = Struct("domain_config", [ >> ("c_info", libxl_domain_create_info), >> ("b_info", libxl_domain_build_info), >> diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c >> new file mode 100644 >> index 0000000..50e1571 >> --- /dev/null >> +++ b/tools/libxl/libxl_usb.c >> @@ -0,0 +1,531 @@ >> +/* >> + * Copyright (C) 2009 Citrix Ltd. >> + * Author Vincent Hanquez >> + * Author Stefano Stabellini >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU Lesser General Public License as published >> + * by the Free Software Foundation; version 2.1 only. with the special >> + * exception on linking described in file LICENSE. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU Lesser General Public License for more details. >> + */ >> + >> +#include "libxl_osdeps.h" /* must come before any other headers */ >> + >> +#include "libxl_internal.h" >> + >> +/* >> + * /libxl//usb//{type, protocol, backend, [typeinfo]} >> + */ >> +#define USB_INFO_PATH "%s/usb" >> +#define USB_HOSTDEV_ID "hostdev-%04x-%04x" >> + >> +/* >> + * Just use plain numbers for now. Replace these with strings at some point. >> + */ >> +#define protocol_to_str(gc, t) libxl__sprintf((gc), "%d", (t)) >> +#define str_to_protocol(s) atoi((s)) >> +#define type_to_str(gc, t) libxl__sprintf((gc), "%d", (t)) > GCSPRINTF, although this macros are so simple that you might also > replace them entirely with GCSPRINTF (altough it might make the code > harder to understand). I might do that -- my original plan had actually been to actually print "pv" for LIBXL_DEVICE_USB_BACKEND_PV, "dm" for _DEVICEMODEL, and so on. Doing all the extra parsing was more work than I wanted to try to get done for the code freeze, but I left the functions so I could easily replaced them later. Anyway, we'll see. > >> +#define str_to_typeprotocol(s) atoi((s)) >> + >> +static char * hostdev_xs_path(libxl__gc *gc, uint32_t domid, >> + libxl__device_usb *usbdev) >> +{ >> + return libxl__sprintf(gc, USB_INFO_PATH "/%s", >> + libxl__xs_libxl_path(gc, domid), >> + libxl__sprintf(gc, USB_HOSTDEV_ID, >> + (uint16_t)usbdev->u.hostdev.hostbus, >> + (uint16_t)usbdev->u.hostdev.hostaddr)); > I will stop repeating every time I see libxl__sprintf to replace it with > GCSPRINTF. > >> +} >> + >> +static void hostdev_xs_append_params(libxl__gc *gc, libxl__device_usb *usbdev, >> + flexarray_t *a) >> +{ >> + flexarray_append_pair(a, "hostbus", >> + libxl__sprintf(gc, "%d", >> + usbdev->u.hostdev.hostbus)); >> + flexarray_append_pair(a, "hostaddr", >> + libxl__sprintf(gc, "%d", >> + usbdev->u.hostdev.hostaddr)); >> +} >> + >> +static int read_hostdev_xenstore_entry(libxl__gc *gc, const char * path, >> + libxl__device_usb *usbdev) >> +{ >> + int rc = 0; >> + char * val; >> + >> + val = libxl__xs_read(gc, XBT_NULL, >> + libxl__sprintf(gc, "%s/hostbus", path)); > libxl__xs_read_checked will also print an error message if the read fails. Sounds good > >> + if(!val) { >> + LOG(ERROR, "Internal error (missing hostbus)"); >> + rc = ERROR_FAIL; >> + goto out; >> + } >> + usbdev->u.hostdev.hostbus = atoi(val); >> + >> + val = libxl__xs_read(gc, XBT_NULL, >> + libxl__sprintf(gc, "%s/hostaddr", path)); > Same > >> + if(!val) { >> + LOG(ERROR, "Internal error (missing hostaddr)"); >> + rc = ERROR_FAIL; >> + goto out; >> + } >> + usbdev->u.hostdev.hostaddr = atoi(val); >> + >> +out: >> + return rc; >> +} >> + >> +static int usb_read_xenstore(libxl__gc *gc, const char * path, >> + libxl__device_usb *usbdev) >> +{ >> + char *val; >> + int rc = 0; >> + >> + val = libxl__xs_read(gc, XBT_NULL, >> + libxl__sprintf(gc, "%s/protocol", path)); >> + if(!val) { >> + LOG(ERROR, "Internal error (missing protocol)"); >> + rc = ERROR_FAIL; >> + goto out; >> + } >> + usbdev->protocol = atoi(val); >> + >> + val = libxl__xs_read(gc, XBT_NULL, >> + libxl__sprintf(gc, "%s/backend_domid", path)); >> + if(!val) { >> + LOG(ERROR, "Internal error (missing backend_domid)"); >> + rc = ERROR_FAIL; >> + goto out; >> + } >> + usbdev->backend_domid = atoi(val); >> + >> + val = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/type", path)); >> + if(!val) { >> + LOG(ERROR, "Internal error (missing type)"); >> + rc = ERROR_FAIL; >> + goto out; >> + } >> + usbdev->type = atoi(val); >> + >> + switch(usbdev->type) { >> + case LIBXL_DEVICE_USB_TYPE_HOSTDEV: >> + if ((rc = read_hostdev_xenstore_entry(gc, path, usbdev)) < 0) >> + goto out; >> + break; >> + default: >> + LOG(ERROR, "Internal error (unimplemented type)"); >> + rc = ERROR_FAIL; >> + goto out; >> + } >> + >> +out: >> + return rc; >> +} >> + >> +static int usb_add_xenstore(libxl__gc *gc, uint32_t domid, >> + libxl__device_usb *usbdev) >> +{ >> + libxl_ctx *ctx = libxl__gc_owner(gc); >> + flexarray_t *dev; >> + char *dev_path; >> + xs_transaction_t t; >> + struct xs_permissions noperm[1]; >> + >> + noperm[0].id = 0; >> + noperm[0].perms = XS_PERM_NONE; >> + >> + dev = flexarray_make(gc, 16, 1); >> + >> + flexarray_append_pair(dev, "protocol", >> + libxl__sprintf(gc, "%d", usbdev->protocol)); >> + >> + flexarray_append_pair(dev, "backend_domid", >> + libxl__sprintf(gc, "%d", usbdev->backend_domid)); >> + >> + flexarray_append_pair(dev, "type", >> + libxl__sprintf(gc, "%d", usbdev->type)); >> + >> + switch(usbdev->type) { >> + case LIBXL_DEVICE_USB_TYPE_HOSTDEV: >> + dev_path = hostdev_xs_path(gc, domid, usbdev); >> + hostdev_xs_append_params(gc, usbdev, dev); >> + break; >> + default: >> + LOG(ERROR, "Invalid device type: %d", usbdev->type); >> + return ERROR_FAIL; >> + } >> + >> + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Adding new usb device to xenstore"); >> + >> +retry_transaction: >> + t = xs_transaction_start(ctx->xsh); >> + libxl__xs_mkdir(gc, t, dev_path, noperm, ARRAY_SIZE(noperm)); > Error checking > >> + libxl__xs_writev(gc, t, dev_path, >> + libxl__xs_kvs_of_flexarray(gc, dev, dev->count)); >> + if (!xs_transaction_end(ctx->xsh, t, 0)) >> + if (errno == EAGAIN) >> + goto retry_transaction; > Ian Jackson (if I remember correctly) added some helpers for > transactions, that make the logic a little bit easier (take a look at > libxl__device_destroy for example in libxl_device.c): > > for (;;) { > rc = libxl__xs_transaction_start(gc, &t); > if (rc) goto out; > > /* Do stuff */ > > rc = libxl__xs_transaction_commit(gc, &t); > if (!rc) break; > if (rc < 0) goto out; > } > > out: > libxl__xs_transaction_abort(gc, &t); > >> + >> + return 0; >> +} >> + >> +static int usb_remove_xenstore(libxl__gc *gc, uint32_t domid, >> + libxl__device_usb *usbdev) >> +{ >> + libxl_ctx *ctx = libxl__gc_owner(gc); >> + char *dev_path; >> + >> + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Removing device from xenstore"); >> + >> + switch(usbdev->type) { >> + case LIBXL_DEVICE_USB_TYPE_HOSTDEV: >> + dev_path = hostdev_xs_path(gc, domid, usbdev); >> + break; >> + default: >> + LOG(ERROR, "Invalid device type: %d", usbdev->type); >> + return ERROR_FAIL; >> + } >> + >> + xs_rm(ctx->xsh, XBT_NULL, dev_path); >> + >> + return 0; >> +} >> + >> +static int get_assigned_devices(libxl__gc *gc, uint32_t domid, >> + libxl__device_usb **list, int *num) >> +{ >> + char **devlist, *dompath; >> + unsigned int nd = 0; >> + int i, rc = 0; >> + >> + *list = NULL; >> + *num = 0; >> + >> + dompath = libxl__sprintf(gc, USB_INFO_PATH, >> + libxl__xs_libxl_path(gc, domid)); >> + >> + devlist = libxl__xs_directory(gc, XBT_NULL, dompath, &nd); >> + if ( !devlist ) >> + goto out; >> + >> + *list = calloc(nd, sizeof(libxl__device_usb)); > libxl__calloc ...which adds it to the GC automatically, I take it? Ack. > >> + >> + for(i = 0; i < nd; i++) { >> + char *path; >> + >> + path = libxl__sprintf(gc, "%s/%s", dompath, devlist[i]); >> + if ( usb_read_xenstore(gc, path, (*list) + i) ) { >> + rc = ERROR_INVAL; >> + free(*list); >> + *list = NULL; >> + *num = 0; >> + goto out; >> + } >> + } >> + >> + *num = nd; >> + >> + libxl__ptr_add(gc, *list); >> + >> +out: >> + return rc; >> +} >> + >> +static int is_usbdev_type_hostdev_equal(libxl__device_usb *a, >> + libxl__device_usb *b) >> +{ >> + if ( !memcmp(&a->u.hostdev, &b->u.hostdev, sizeof(a->u.hostdev) ) ) >> + return 1; >> + else >> + return 0; >> +} >> + >> +static int is_usbdev_in_array(libxl__device_usb *assigned, int num_assigned, >> + libxl__device_usb *dev) >> +{ >> + int i; >> + >> + /* Devices are the same if: >> + * - They have the same backend_domid >> + * - They are of the same type >> + * - Their types match >> + * In theory, someone could try to pass the same device through >> + * via both PVUSB and qemu; not comparing protocol will prevent that. >> + */ >> + for(i = 0; i < num_assigned; i++) { >> + if( assigned[i].type != dev->type >> + || assigned[i].backend_domid != dev->backend_domid ) > No spaces between "(" and the condition. > >> + continue; >> + >> + switch(dev->type) { >> + case LIBXL_DEVICE_USB_TYPE_HOSTDEV: >> + if (!is_usbdev_type_hostdev_equal(dev, assigned+i)) >> + continue; >> + } >> + >> + return 1; >> + } >> + >> + return 0; >> +} >> + >> +static void usbdev_ext_to_int(libxl__device_usb *dev_int, >> + libxl_device_usb *dev_ext) >> +{ >> + dev_int->protocol = dev_ext->protocol; >> + >> + if (dev_ext->backend_domid == LIBXL_DEVICE_USB_BACKEND_DEFAULT) >> + dev_int->backend_domid = 0; >> + else >> + dev_int->backend_domid = dev_ext->backend_domid; >> + >> + dev_int->type = dev_ext->type; >> + memcpy(&dev_int->u, &dev_ext->u, sizeof(dev_ext->u)); >> +} >> + >> +static void usbdev_int_to_ext(libxl_device_usb *dev_ext, >> + libxl__device_usb *dev_int) >> +{ >> + dev_ext->protocol = dev_int->protocol; >> + >> + dev_ext->backend_domid = dev_int->backend_domid; >> + >> + dev_ext->type = dev_int->type; >> + memcpy(&dev_ext->u, &dev_int->u, sizeof(dev_ext->u)); >> +} >> + >> +/* >> + * USB add >> + */ >> +static int do_usb_add(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-add not yet implemented for qemu-traditional"); >> + return ERROR_INVAL; >> + case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN: >> + if ( (rc = libxl__qmp_usb_add(gc, domid, usbdev)) < 0 ) >> + goto out; >> + break; >> + default: >> + return ERROR_INVAL; >> + } >> + break; >> + default: >> + return ERROR_FAIL; >> + } >> + >> + rc = usb_add_xenstore(gc, domid, usbdev); >> +out: >> + return rc; >> +} >> + >> + >> + >> +static int libxl__device_usb_add(libxl__gc *gc, uint32_t domid, >> + libxl_device_usb *dev_ext) >> +{ >> + libxl_ctx *ctx = libxl__gc_owner(gc); >> + libxl__device_usb *assigned, _usbdev, *usbdev; >> + int rc = ERROR_FAIL, num_assigned; >> + libxl_domain_type domtype = libxl__domain_type(gc, domid); >> + >> + /* Interpret incoming */ >> + usbdev = &_usbdev; >> + >> + usbdev->target_domid = domid; >> + usbdev->dm_domid = libxl_get_stubdom_id(ctx, domid); >> + >> + usbdev_ext_to_int(usbdev, dev_ext); >> + >> + if ( usbdev->protocol == LIBXL_USB_PROTOCOL_AUTO ) { >> + if ( domtype == LIBXL_DOMAIN_TYPE_PV ) { >> + usbdev->protocol = LIBXL_USB_PROTOCOL_PV; >> + } else if (domtype == LIBXL_DOMAIN_TYPE_HVM) { >> + /* FIXME: See if we can detect PV frontend */ >> + usbdev->protocol = LIBXL_USB_PROTOCOL_DEVICEMODEL; >> + } >> + } >> + >> + /* Check to make sure we're doing something that's impemented */ >> + if ( usbdev->protocol != LIBXL_USB_PROTOCOL_DEVICEMODEL ) { >> + rc = ERROR_FAIL; >> + LOG(ERROR, "Protocol not implemented"); >> + goto out; >> + } >> + >> + if ( usbdev->dm_domid != 0 ) { >> + rc = ERROR_FAIL; >> + LOG(ERROR, "Stubdoms not yet supported"); >> + goto out; >> + } >> + >> + /* Double-check to make sure it's not already assigned */ >> + rc = get_assigned_devices(gc, domid, &assigned, &num_assigned); >> + if ( rc ) { >> + LOG(ERROR, "cannot determine if device is assigned, refusing to continue"); >> + goto out; >> + } >> + if ( is_usbdev_in_array(assigned, num_assigned, usbdev) ) { >> + LOG(ERROR, "USB device already attached to a domain"); >> + rc = ERROR_FAIL; >> + goto out; >> + } >> + >> + /* Do the add */ >> + if ( do_usb_add(gc, domid, usbdev) ) >> + rc = ERROR_FAIL; > Spaces between "(" and conditions in this function. > >> + >> +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. > >> + goto out; >> + break; >> + default: >> + return ERROR_INVAL; >> + } >> + break; >> + default: >> + return ERROR_FAIL; >> + } >> + >> + rc = usb_remove_xenstore(gc, domid, usbdev); >> +out: >> + return rc; >> +} >> +static int libxl__device_usb_remove(libxl__gc *gc, uint32_t domid, >> + libxl_device_usb *dev_ext) >> +{ >> + libxl_ctx *ctx = libxl__gc_owner(gc); >> + libxl__device_usb *assigned, _usbdev, *usbdev; >> + int rc = ERROR_FAIL, num_assigned; >> + libxl_domain_type domtype = libxl__domain_type(gc, domid); >> + >> + /* Interpret incoming */ >> + usbdev = &_usbdev; >> + >> + usbdev->target_domid = domid; >> + usbdev->dm_domid = libxl_get_stubdom_id(ctx, domid); >> + >> + usbdev_ext_to_int(usbdev, dev_ext); >> + >> + if ( usbdev->protocol == LIBXL_USB_PROTOCOL_AUTO ) { >> + if ( domtype == LIBXL_DOMAIN_TYPE_PV ) { >> + usbdev->protocol = LIBXL_USB_PROTOCOL_PV; >> + } else if (domtype == LIBXL_DOMAIN_TYPE_HVM) { >> + /* FIXME: See if we can detect PV frontend */ >> + usbdev->protocol = LIBXL_USB_PROTOCOL_DEVICEMODEL; >> + } >> + } >> + >> + /* Check to make sure we're doing something that's impemented */ >> + if ( usbdev->protocol != LIBXL_USB_PROTOCOL_DEVICEMODEL ) { >> + rc = ERROR_FAIL; >> + LOG(ERROR, "Protocol not implemented"); >> + goto out; >> + } >> + >> + if ( usbdev->dm_domid != 0 ) { >> + rc = ERROR_FAIL; >> + LOG(ERROR, "Stubdoms not yet supported"); >> + goto out; >> + } >> + >> + /* Double-check to make sure it's not already assigned */ >> + rc = get_assigned_devices(gc, domid, &assigned, &num_assigned); >> + if ( rc ) { >> + LOG(ERROR, "cannot determine if device is assigned, refusing to continue"); >> + goto out; >> + } >> + if ( !is_usbdev_in_array(assigned, num_assigned, usbdev) ) { >> + LOG(ERROR, "USB device doesn't seem to be attached to the domain"); >> + rc = ERROR_FAIL; >> + goto out; >> + } >> + >> + /* Do the remove */ >> + if ( do_usb_remove(gc, domid, usbdev) ) >> + rc = ERROR_FAIL; >> + > Again spaces in almost the whole function > >> +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 *num) >> +{ >> + 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.) Thanks, -George