* [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
@ 2013-04-11 18:51 George Dunlap
2013-04-11 18:51 ` [PATCH v4 2/2] xl: Add commands for usb hot-plug George Dunlap
` (3 more replies)
0 siblings, 4 replies; 22+ messages in thread
From: George Dunlap @ 2013-04-11 18:51 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Ian Jackson, sstanisi, Roger Pau Monne
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 <george.dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Roger Pau Monne <roger.pau@citrix.com>
CC: sstanisi@cbnco.com
---
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);
+ 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);
+ libxl__xs_mkdir(gc, t, libxl_usb_path, noperm, ARRAY_SIZE(noperm));
+
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);
+
+ 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);
+
+ 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 <vincent.hanquez@eu.citrix.com>
+ * Author Stefano Stabellini <stefano.stabellini@eu.citrix.com>
+ *
+ * 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/<domid>/usb/<devid>/{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))
+#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));
+}
+
+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));
+ 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));
+ 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));
+ 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;
+
+ 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));
+
+ 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 )
+ 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;
+
+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 )
+ 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;
+
+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));
+
+ for (i = 0; i < n; i++)
+ usbdev_int_to_ext(devext + i, devint + i);
+
+ *num = n;
+out:
+ GC_FREE;
+ return devext;
+}
diff --git a/tools/ocaml/libs/xl/genwrap.py b/tools/ocaml/libs/xl/genwrap.py
index ea978bf..aab65f3 100644
--- a/tools/ocaml/libs/xl/genwrap.py
+++ b/tools/ocaml/libs/xl/genwrap.py
@@ -286,6 +286,7 @@ if __name__ == '__main__':
"domain_config",
"vcpuinfo",
"event",
+ "device_usb"
]
for t in blacklist:
--
1.7.9.5
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v4 2/2] xl: Add commands for usb hot-plug
2013-04-11 18:51 [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest George Dunlap
@ 2013-04-11 18:51 ` George Dunlap
2013-04-17 10:02 ` Roger Pau Monné
2013-04-11 18:55 ` [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest George Dunlap
` (2 subsequent siblings)
3 siblings, 1 reply; 22+ messages in thread
From: George Dunlap @ 2013-04-11 18:51 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Ian Jackson, sstanisi, Roger Pau Monne
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Roger Pau Monne <roger.pau@citrix.com>
CC: sstanisi@cbnco.com
---
docs/man/xl.pod.1 | 30 +++++++
tools/libxl/xl.h | 3 +
tools/libxl/xl_cmdimpl.c | 219 +++++++++++++++++++++++++++++++++++++++++++++
tools/libxl/xl_cmdtable.c | 15 ++++
4 files changed, 267 insertions(+)
diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index a0e298e..18a8eee 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1110,6 +1110,36 @@ List virtual network interfaces for a domain.
=back
+=head2 USB DEVICES
+
+=over 4
+
+=item B<usb-add> I<-d domain-id> I<-v hosbus.hostaddr>
+
+Passes through the host USB device specified by I<hostbus.hostaddr>. At
+the moment this will only work for HVM domains via qemu.
+
+The best way to find out the information for the device is typically using
+lsusb.
+
+This command is only available for domains using qemu-xen, not
+qemu-traditional.
+
+=item B<usb-remove> I<-d domain-id> I<-v hosbus.hostaddr>
+
+Remove the host USB device from I<domain-id> which is specified
+by <hostbus.hostaddr>. This command only works for devices added
+with usb-add; not for those specified in the config file.
+
+This command is only available for domains using qemu-xen, not
+qemu-traditional.
+
+=item B<usb-list> I<domain-id>
+
+Show host USB devices assigned to the guest.
+
+=back
+
=head2 VTPM DEVICES
=over 4
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index b881f92..5c39fa2 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -35,6 +35,9 @@ int main_info(int argc, char **argv);
int main_sharing(int argc, char **argv);
int main_cd_eject(int argc, char **argv);
int main_cd_insert(int argc, char **argv);
+int main_usb_add(int argc, char **argv);
+int main_usb_remove(int argc, char **argv);
+int main_usb_list(int argc, char **argv);
int main_console(int argc, char **argv);
int main_vncviewer(int argc, char **argv);
int main_pcilist(int argc, char **argv);
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 61f7b96..a690823 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2600,6 +2600,225 @@ int main_cd_insert(int argc, char **argv)
return 0;
}
+
+
+static int parse_usb_hostdev_specifier(libxl_device_usb *dev, const char *s)
+{
+ const char * hostbus, *hostaddr, *p;
+
+ hostbus = s;
+ hostaddr=NULL;
+
+#define is_dec(_c) ((_c) >= '0' && (_c) <= '9')
+#define is_hex(_c) (is_dec(_c) || ((_c) >= 'a' && (_c) <= 'f'))
+
+ /* Match [0-9]+\.[0-9] */
+ if (!is_dec(*hostbus))
+ return -1;
+
+ for(p=s; *p; p++) {
+ if(*p == '.') {
+ if ( !hostaddr )
+ hostaddr = p+1;
+ else {
+ return -1;
+ }
+ } else if (!is_dec(*p)) {
+ return -1;
+ }
+ }
+ if (!hostaddr || !is_dec(*hostaddr))
+ return -1;
+ dev->u.hostdev.hostbus = strtoul(hostbus, NULL, 10);
+ dev->u.hostdev.hostaddr = strtoul(hostaddr, NULL, 10);
+#undef is_dec
+#undef is_hex
+
+ return 0;
+}
+
+static int usb_add(uint32_t domid, libxl_device_usb_type type,
+ const char * device)
+{
+ libxl_device_usb usbdev;
+ int rc;
+
+ libxl_device_usb_init(&usbdev);
+
+ usbdev.type = type;
+
+ switch(type) {
+ case LIBXL_DEVICE_USB_TYPE_HOSTDEV:
+ if ( parse_usb_hostdev_specifier(&usbdev, device) < 0 ) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ break;
+ default:
+ fprintf(stderr, "INTERNAL ERROR: Unimplemented type.\n");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ if ( (rc = libxl_device_usb_add(ctx, domid, &usbdev, NULL)) < 0 )
+ fprintf(stderr, "libxl_usb_add failed.\n");
+
+ libxl_device_usb_dispose(&usbdev);
+
+out:
+ return rc;
+}
+
+int main_usb_add(int argc, char **argv)
+{
+ uint32_t domid = -1;
+ int opt = 0, rc;
+ const char *device = NULL;
+ int type = 0;
+
+ SWITCH_FOREACH_OPT(opt, "d:v:", NULL, "usb-add", 0) {
+ case 'd':
+ domid = find_domain(optarg);
+ break;
+ case 'v':
+ type = LIBXL_DEVICE_USB_TYPE_HOSTDEV;
+ device = optarg;
+ break;
+ }
+
+ if ( domid == -1 ) {
+ fprintf(stderr, "Must specify domid\n\n");
+ help("usb-add");
+ return 2;
+ }
+
+ if ( !device ) {
+ fprintf(stderr, "Must specify a device\n\n");
+ help("usb-add");
+ return 2;
+ }
+
+ rc = usb_add(domid, type, device);
+ if ( rc < 0 )
+ return 1;
+ else
+ return 0;
+}
+
+static int usb_remove(uint32_t domid, libxl_device_usb_type type,
+ const char * device)
+{
+ libxl_device_usb usbdev;
+ int rc;
+
+ libxl_device_usb_init(&usbdev);
+
+ usbdev.type = type;
+
+ switch(type) {
+ case LIBXL_DEVICE_USB_TYPE_HOSTDEV:
+ if ( parse_usb_hostdev_specifier(&usbdev, device) < 0 ) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ break;
+ default:
+ fprintf(stderr, "INTERNAL ERROR: Unimplemented type.\n");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ if ( (rc = libxl_device_usb_remove(ctx, domid, &usbdev, NULL)) < 0 )
+ fprintf(stderr, "libxl_usb_remove failed.\n");
+
+ libxl_device_usb_dispose(&usbdev);
+
+out:
+ return rc;
+}
+
+int main_usb_remove(int argc, char **argv)
+{
+ uint32_t domid = -1;
+ int opt = 0, rc;
+ const char *device = NULL;
+ int type = 0;
+
+ SWITCH_FOREACH_OPT(opt, "d:v:", NULL, "usb-remove", 0) {
+ case 'd':
+ domid = find_domain(optarg);
+ break;
+ case 'v':
+ type = LIBXL_DEVICE_USB_TYPE_HOSTDEV;
+ device = optarg;
+ break;
+ }
+
+ if ( domid == -1 ) {
+ fprintf(stderr, "Must specify domid\n\n");
+ help("usb-remove");
+ return 2;
+ }
+
+ if ( !device ) {
+ fprintf(stderr, "Must specify a device\n\n");
+ help("usb-remove");
+ return 2;
+ }
+
+ rc = usb_remove(domid, type, device);
+ if ( rc < 0 )
+ return 1;
+ else
+ return 0;
+}
+
+static void usb_list(uint32_t domid)
+{
+ libxl_device_usb *dev;
+ int num, i;
+
+ dev = libxl_device_usb_list(ctx, domid, &num);
+ if (dev == NULL)
+ return;
+ printf("protocol backend type device\n");
+ for (i = 0; i < num; i++) {
+ printf("%8s ", (dev[i].protocol==LIBXL_USB_PROTOCOL_PV)?"pv":"dm");
+ printf("%7d ", dev[i].backend_domid);
+ printf("%7s ", (dev[i].type==LIBXL_DEVICE_USB_TYPE_HOSTDEV)?"hostdev":"unknown");
+ if(dev[i].type == LIBXL_DEVICE_USB_TYPE_HOSTDEV)
+ printf("%03d.%03d",
+ dev[i].u.hostdev.hostbus,
+ dev[i].u.hostdev.hostaddr);
+ printf("\n");
+ }
+ free(dev);
+}
+
+
+int main_usb_list(int argc, char **argv)
+{
+ uint32_t domid = -1;
+ int opt;
+
+ SWITCH_FOREACH_OPT(opt, "d:", NULL, "usb-list", 0) {
+ case 'd':
+ domid = find_domain(optarg);
+ break;
+ }
+
+ if ( domid == -1 ) {
+ fprintf(stderr, "Must specify domid\n\n");
+ help("usb-remove");
+ return 2;
+ }
+
+ usb_list(domid);
+ return 0;
+}
+
+
+
int main_console(int argc, char **argv)
{
uint32_t domid;
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index b4a87ca..3cf8e65 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -187,6 +187,21 @@ struct cmd_spec cmd_table[] = {
"Eject a cdrom from a guest's cd drive",
"<Domain> <VirtualDevice>",
},
+ { "usb-add",
+ &main_usb_add, 1, 1,
+ "Hot-plug a usb device to a domain.",
+ "-d <Domain> [-v <hostbus.hostaddr>]",
+ },
+ { "usb-remove",
+ &main_usb_remove, 1, 1,
+ "Hot-unplug a usb device from a domain.",
+ "-d <Domain> [-v <hostbus.hostaddr>]",
+ },
+ { "usb-list",
+ &main_usb_list, 0, 0,
+ "List usb devices for a domain",
+ "<Domain>",
+ },
{ "mem-max",
&main_memmax, 0, 1,
"Set the maximum amount reservation for a domain",
--
1.7.9.5
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
2013-04-11 18:51 [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest George Dunlap
2013-04-11 18:51 ` [PATCH v4 2/2] xl: Add commands for usb hot-plug George Dunlap
@ 2013-04-11 18:55 ` George Dunlap
2013-04-16 15:38 ` George Dunlap
2013-04-16 18:00 ` Roger Pau Monné
3 siblings, 0 replies; 22+ messages in thread
From: George Dunlap @ 2013-04-11 18:55 UTC (permalink / raw)
To: xen-devel@lists.xen.org; +Cc: sstanisi@cbnco.com, Ian Jackson, Roger Pau Monne
On 11/04/13 19: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 <george.dunlap@eu.citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Roger Pau Monne <roger.pau@citrix.com>
> CC: sstanisi@cbnco.com
> ---
> 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
> + */
Dangit! Well, I'll fix this in v5...
-G
> +#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);
> + 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);
> + libxl__xs_mkdir(gc, t, libxl_usb_path, noperm, ARRAY_SIZE(noperm));
> +
> 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);
> +
> + 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);
> +
> + 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 <vincent.hanquez@eu.citrix.com>
> + * Author Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> + *
> + * 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/<domid>/usb/<devid>/{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))
> +#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));
> +}
> +
> +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));
> + 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));
> + 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));
> + 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;
> +
> + 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));
> +
> + 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 )
> + 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;
> +
> +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 )
> + 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;
> +
> +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));
> +
> + for (i = 0; i < n; i++)
> + usbdev_int_to_ext(devext + i, devint + i);
> +
> + *num = n;
> +out:
> + GC_FREE;
> + return devext;
> +}
> diff --git a/tools/ocaml/libs/xl/genwrap.py b/tools/ocaml/libs/xl/genwrap.py
> index ea978bf..aab65f3 100644
> --- a/tools/ocaml/libs/xl/genwrap.py
> +++ b/tools/ocaml/libs/xl/genwrap.py
> @@ -286,6 +286,7 @@ if __name__ == '__main__':
> "domain_config",
> "vcpuinfo",
> "event",
> + "device_usb"
> ]
>
> for t in blacklist:
> --
> 1.7.9.5
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
2013-04-11 18:51 [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest George Dunlap
2013-04-11 18:51 ` [PATCH v4 2/2] xl: Add commands for usb hot-plug George Dunlap
2013-04-11 18:55 ` [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest George Dunlap
@ 2013-04-16 15:38 ` George Dunlap
2013-04-16 18:00 ` Roger Pau Monné
3 siblings, 0 replies; 22+ messages in thread
From: George Dunlap @ 2013-04-16 15:38 UTC (permalink / raw)
To: xen-devel@lists.xen.org
Cc: George Dunlap, Ian Jackson, Stefan, Roger Pau Monne
On Thu, Apr 11, 2013 at 7:51 PM, George Dunlap
<george.dunlap@eu.citrix.com> 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 <george.dunlap@eu.citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Roger Pau Monne <roger.pau@citrix.com>
> CC: sstanisi@cbnco.com
Roger, Ian, any feedback on this? It would be nice to get this in
this week if possible...
-George
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
2013-04-11 18:51 [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest George Dunlap
` (2 preceding siblings ...)
2013-04-16 15:38 ` George Dunlap
@ 2013-04-16 18:00 ` Roger Pau Monné
2013-04-17 9:36 ` George Dunlap
2013-04-17 9:58 ` Ian Campbell
3 siblings, 2 replies; 22+ messages in thread
From: Roger Pau Monné @ 2013-04-16 18:00 UTC (permalink / raw)
To: George Dunlap; +Cc: sstanisi@cbnco.com, Ian Jackson, xen-devel@lists.xen.org
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 <george.dunlap@eu.citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Roger Pau Monne <roger.pau@citrix.com>
> CC: sstanisi@cbnco.com
I'm not familiar with the QMP interface, so I just commented the general
libxl parts of the patch.
> ---
> 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)
> + 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.
> + 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.
> +
> + 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 <vincent.hanquez@eu.citrix.com>
> + * Author Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> + *
> + * 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/<domid>/usb/<devid>/{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).
> +#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.
> + 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
> +
> + 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
> + 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.
> +
> + for (i = 0; i < n; i++)
> + usbdev_int_to_ext(devext + i, devint + i);
> +
> + *num = n;
> +out:
> + GC_FREE;
> + return devext;
> +}
> diff --git a/tools/ocaml/libs/xl/genwrap.py b/tools/ocaml/libs/xl/genwrap.py
> index ea978bf..aab65f3 100644
> --- a/tools/ocaml/libs/xl/genwrap.py
> +++ b/tools/ocaml/libs/xl/genwrap.py
> @@ -286,6 +286,7 @@ if __name__ == '__main__':
> "domain_config",
> "vcpuinfo",
> "event",
> + "device_usb"
> ]
>
> for t in blacklist:
> --
> 1.7.9.5
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
2013-04-16 18:00 ` Roger Pau Monné
@ 2013-04-17 9:36 ` George Dunlap
2013-04-17 9:48 ` Roger Pau Monné
` (2 more replies)
2013-04-17 9:58 ` Ian Campbell
1 sibling, 3 replies; 22+ messages in thread
From: George Dunlap @ 2013-04-17 9:36 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: sstanisi@cbnco.com, Ian Jackson, xen-devel@lists.xen.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 <george.dunlap@eu.citrix.com>
>> CC: Ian Jackson <ian.jackson@citrix.com>
>> CC: Roger Pau Monne <roger.pau@citrix.com>
>> 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 <vincent.hanquez@eu.citrix.com>
>> + * Author Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> + *
>> + * 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/<domid>/usb/<devid>/{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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
2013-04-17 9:36 ` George Dunlap
@ 2013-04-17 9:48 ` Roger Pau Monné
2013-04-17 10:05 ` George Dunlap
2013-04-17 9:54 ` Ian Campbell
2013-04-17 11:41 ` Ian Jackson
2 siblings, 1 reply; 22+ messages in thread
From: Roger Pau Monné @ 2013-04-17 9:48 UTC (permalink / raw)
To: George Dunlap; +Cc: sstanisi@cbnco.com, Ian Jackson, xen-devel@lists.xen.org
>>> + 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.
>>> +
>>> +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 *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.)
OK, then you can use libxl__calloc with NOGC.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
2013-04-17 9:36 ` George Dunlap
2013-04-17 9:48 ` Roger Pau Monné
@ 2013-04-17 9:54 ` Ian Campbell
2013-04-17 9:59 ` George Dunlap
2013-04-17 11:41 ` Ian Jackson
2 siblings, 1 reply; 22+ messages in thread
From: Ian Campbell @ 2013-04-17 9:54 UTC (permalink / raw)
To: George Dunlap
Cc: sstanisi@cbnco.com, xen-devel@lists.xen.org, Ian Jackson,
Roger Pau Monne
On Wed, 2013-04-17 at 10:36 +0100, George Dunlap wrote:
> >> +/*
> >> + * 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.
These symbols should be coming from the IDL, in which case it will have
provided the parsing functions already, please use them instead of
inventing your own.
e.g. The libxl_domain_type enum in the idl generates
libxl_domain_type_from_string and libxl_domain_type_to_string (and
_to_json if that floats your boat). These implement the mapping:
LIBXL_DOMAIN_TYPE_INVALID <=> "invalid"
LIBXL_DOMAIN_TYPE_HVM <=> "hvm"
LIBXL_DOMAIN_TYPE_PV <=> "pv"
(the from function is case insensitive)
"Replace these with strings at some point.", is that an API change,
either for the libxl user or the driver side?
> >> + 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.
Everyone has their own opinions about these thing but the main purpose
of having a Coding Style is consistency and libxl's style is to not
include those spaces.
If you want to make it easier to read I would suggest removing the
assignment from the condition.
Ian.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
2013-04-16 18:00 ` Roger Pau Monné
2013-04-17 9:36 ` George Dunlap
@ 2013-04-17 9:58 ` Ian Campbell
2013-04-17 10:02 ` George Dunlap
1 sibling, 1 reply; 22+ messages in thread
From: Ian Campbell @ 2013-04-17 9:58 UTC (permalink / raw)
To: Roger Pau Monné
Cc: George Dunlap, sstanisi@cbnco.com, Ian Jackson,
xen-devel@lists.xen.org
On Tue, 2013-04-16 at 19:00 +0100, Roger Pau Monné wrote:
> > + libxl_usb_path = libxl__sprintf(gc, "%s/usb", libxl_path);
>
> I would recommend to use GCSPRINTF instead, it already checks for errors
GCSPRINTF and libxl__sprintf have the same amount of error checking.
The more important point is that the libxl memory allocation
functions/macros all panic on ENOMEM so there is no need for code in
general to check for allocation errors.
i.e. This block...
> > + if (!libxl_usb_path) {
> > + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot allocate create paths");
> > + rc = ERROR_FAIL;
> > + goto out;
> > + }
...is unneeded. I didn't check the rest of the patch but I imagine you
are in the habits of checking errors and can now remove a bunch of
code ;-)
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
2013-04-17 9:54 ` Ian Campbell
@ 2013-04-17 9:59 ` George Dunlap
0 siblings, 0 replies; 22+ messages in thread
From: George Dunlap @ 2013-04-17 9:59 UTC (permalink / raw)
To: Ian Campbell
Cc: sstanisi@cbnco.com, xen-devel@lists.xen.org, Ian Jackson,
Roger Pau Monne
On 17/04/13 10:54, Ian Campbell wrote:
> On Wed, 2013-04-17 at 10:36 +0100, George Dunlap wrote:
>>>> +/*
>>>> + * 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.
> These symbols should be coming from the IDL, in which case it will have
> provided the parsing functions already, please use them instead of
> inventing your own.
>
> e.g. The libxl_domain_type enum in the idl generates
> libxl_domain_type_from_string and libxl_domain_type_to_string (and
> _to_json if that floats your boat). These implement the mapping:
> LIBXL_DOMAIN_TYPE_INVALID <=> "invalid"
> LIBXL_DOMAIN_TYPE_HVM <=> "hvm"
> LIBXL_DOMAIN_TYPE_PV <=> "pv"
> (the from function is case insensitive)
>
> "Replace these with strings at some point.", is that an API change,
> either for the libxl user or the driver side?
This is just an internal thing for libxl; it doesn't persist across reboots.
Let me take a look at the functions you mentioned -- if they're really
easy I'll just drop them in. Otherwise, the numeric values will be just
fine, even if for some reason they end up being permanent.
>
>>>> + 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.
> Everyone has their own opinions about these thing but the main purpose
> of having a Coding Style is consistency and libxl's style is to not
> include those spaces.
>
> If you want to make it easier to read I would suggest removing the
> assignment from the condition.
Ack.
-George
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
2013-04-17 9:58 ` Ian Campbell
@ 2013-04-17 10:02 ` George Dunlap
2013-04-17 10:13 ` Ian Campbell
0 siblings, 1 reply; 22+ messages in thread
From: George Dunlap @ 2013-04-17 10:02 UTC (permalink / raw)
To: Ian Campbell
Cc: sstanisi@cbnco.com, xen-devel@lists.xen.org, Ian Jackson,
Roger Pau Monne
On 17/04/13 10:58, Ian Campbell wrote:
> On Tue, 2013-04-16 at 19:00 +0100, Roger Pau Monné wrote:
>
>>> + libxl_usb_path = libxl__sprintf(gc, "%s/usb", libxl_path);
>> I would recommend to use GCSPRINTF instead, it already checks for errors
> GCSPRINTF and libxl__sprintf have the same amount of error checking.
>
> The more important point is that the libxl memory allocation
> functions/macros all panic on ENOMEM so there is no need for code in
> general to check for allocation errors.
>
> i.e. This block...
>>> + if (!libxl_usb_path) {
>>> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot allocate create paths");
>>> + rc = ERROR_FAIL;
>>> + goto out;
>>> + }
> ...is unneeded. I didn't check the rest of the patch but I imagine you
> are in the habits of checking errors and can now remove a bunch of
> code ;-)
OK -- well in that block in particular, it's following suit with the
rest of the surrounding code which does exactly the same thing. As I
said in another e-mail to roger about error checking in this function, I
think that it's more important for programmers to be able to see this is
exactly the same as all the rest. At some point there should probably
be a clean-up to remove the check from all of them, but I don't think
that's worth the risk at this point in the release cycle.
I can put cleaning up this function on my to-do list for the 4.4 dev
cycle if you want.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/2] xl: Add commands for usb hot-plug
2013-04-11 18:51 ` [PATCH v4 2/2] xl: Add commands for usb hot-plug George Dunlap
@ 2013-04-17 10:02 ` Roger Pau Monné
2013-04-17 10:03 ` George Dunlap
` (2 more replies)
0 siblings, 3 replies; 22+ messages in thread
From: Roger Pau Monné @ 2013-04-17 10:02 UTC (permalink / raw)
To: George Dunlap; +Cc: sstanisi@cbnco.com, Ian Jackson, xen-devel@lists.xen.org
On 11/04/13 20:51, George Dunlap wrote:
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Roger Pau Monne <roger.pau@citrix.com>
> CC: sstanisi@cbnco.com
> ---
> docs/man/xl.pod.1 | 30 +++++++
> tools/libxl/xl.h | 3 +
> tools/libxl/xl_cmdimpl.c | 219 +++++++++++++++++++++++++++++++++++++++++++++
> tools/libxl/xl_cmdtable.c | 15 ++++
> 4 files changed, 267 insertions(+)
>
> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> index a0e298e..18a8eee 100644
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -1110,6 +1110,36 @@ List virtual network interfaces for a domain.
>
> =back
>
> +=head2 USB DEVICES
> +
> +=over 4
> +
> +=item B<usb-add> I<-d domain-id> I<-v hosbus.hostaddr>
> +
> +Passes through the host USB device specified by I<hostbus.hostaddr>. At
> +the moment this will only work for HVM domains via qemu.
> +
> +The best way to find out the information for the device is typically using
> +lsusb.
> +
> +This command is only available for domains using qemu-xen, not
> +qemu-traditional.
> +
> +=item B<usb-remove> I<-d domain-id> I<-v hosbus.hostaddr>
> +
> +Remove the host USB device from I<domain-id> which is specified
> +by <hostbus.hostaddr>. This command only works for devices added
> +with usb-add; not for those specified in the config file.
> +
> +This command is only available for domains using qemu-xen, not
> +qemu-traditional.
> +
> +=item B<usb-list> I<domain-id>
> +
> +Show host USB devices assigned to the guest.
> +
> +=back
> +
> =head2 VTPM DEVICES
>
> =over 4
> diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
> index b881f92..5c39fa2 100644
> --- a/tools/libxl/xl.h
> +++ b/tools/libxl/xl.h
> @@ -35,6 +35,9 @@ int main_info(int argc, char **argv);
> int main_sharing(int argc, char **argv);
> int main_cd_eject(int argc, char **argv);
> int main_cd_insert(int argc, char **argv);
> +int main_usb_add(int argc, char **argv);
> +int main_usb_remove(int argc, char **argv);
> +int main_usb_list(int argc, char **argv);
> int main_console(int argc, char **argv);
> int main_vncviewer(int argc, char **argv);
> int main_pcilist(int argc, char **argv);
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 61f7b96..a690823 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -2600,6 +2600,225 @@ int main_cd_insert(int argc, char **argv)
> return 0;
> }
>
> +
> +
> +static int parse_usb_hostdev_specifier(libxl_device_usb *dev, const char *s)
> +{
> + const char * hostbus, *hostaddr, *p;
> +
> + hostbus = s;
> + hostaddr=NULL;
> +
> +#define is_dec(_c) ((_c) >= '0' && (_c) <= '9')
> +#define is_hex(_c) (is_dec(_c) || ((_c) >= 'a' && (_c) <= 'f'))
This are kind of general macros, that could be used elsewhere, might be
suitable to put them outside of this function and name them CHAR_IS_DEC
and CHAR_IS_HEX.
> +
> + /* Match [0-9]+\.[0-9] */
> + if (!is_dec(*hostbus))
> + return -1;
> +
> + for(p=s; *p; p++) {
> + if(*p == '.') {
> + if ( !hostaddr )
> + hostaddr = p+1;
> + else {
> + return -1;
> + }
> + } else if (!is_dec(*p)) {
> + return -1;
> + }
> + }
> + if (!hostaddr || !is_dec(*hostaddr))
> + return -1;
> + dev->u.hostdev.hostbus = strtoul(hostbus, NULL, 10);
> + dev->u.hostdev.hostaddr = strtoul(hostaddr, NULL, 10);
> +#undef is_dec
> +#undef is_hex
> +
> + return 0;
> +}
> +
> +static int usb_add(uint32_t domid, libxl_device_usb_type type,
> + const char * device)
> +{
> + libxl_device_usb usbdev;
> + int rc;
> +
> + libxl_device_usb_init(&usbdev);
> +
> + usbdev.type = type;
> +
> + switch(type) {
> + case LIBXL_DEVICE_USB_TYPE_HOSTDEV:
> + if ( parse_usb_hostdev_specifier(&usbdev, device) < 0 ) {
> + rc = ERROR_FAIL;
> + goto out;
> + }
> + break;
> + default:
> + fprintf(stderr, "INTERNAL ERROR: Unimplemented type.\n");
> + rc = ERROR_FAIL;
> + goto out;
> + }
> +
> + if ( (rc = libxl_device_usb_add(ctx, domid, &usbdev, NULL)) < 0 )
> + fprintf(stderr, "libxl_usb_add failed.\n");
> +
> + libxl_device_usb_dispose(&usbdev);
> +
> +out:
> + return rc;
> +}
> +
> +int main_usb_add(int argc, char **argv)
> +{
> + uint32_t domid = -1;
You could use INVALID_DOMID that is defined early in the file.
> + int opt = 0, rc;
> + const char *device = NULL;
> + int type = 0;
> +
> + SWITCH_FOREACH_OPT(opt, "d:v:", NULL, "usb-add", 0) {
> + case 'd':
> + domid = find_domain(optarg);
> + break;
> + case 'v':
> + type = LIBXL_DEVICE_USB_TYPE_HOSTDEV;
> + device = optarg;
> + break;
> + }
> +
> + if ( domid == -1 ) {
> + fprintf(stderr, "Must specify domid\n\n");
> + help("usb-add");
> + return 2;
> + }
> +
> + if ( !device ) {
> + fprintf(stderr, "Must specify a device\n\n");
> + help("usb-add");
> + return 2;
> + }
> +
> + rc = usb_add(domid, type, device);
> + if ( rc < 0 )
> + return 1;
> + else
> + return 0;
> +}
> +
> +static int usb_remove(uint32_t domid, libxl_device_usb_type type,
> + const char * device)
> +{
> + libxl_device_usb usbdev;
> + int rc;
> +
> + libxl_device_usb_init(&usbdev);
> +
> + usbdev.type = type;
> +
> + switch(type) {
> + case LIBXL_DEVICE_USB_TYPE_HOSTDEV:
> + if ( parse_usb_hostdev_specifier(&usbdev, device) < 0 ) {
> + rc = ERROR_FAIL;
> + goto out;
> + }
> + break;
> + default:
> + fprintf(stderr, "INTERNAL ERROR: Unimplemented type.\n");
> + rc = ERROR_FAIL;
> + goto out;
> + }
> +
> + if ( (rc = libxl_device_usb_remove(ctx, domid, &usbdev, NULL)) < 0 )
> + fprintf(stderr, "libxl_usb_remove failed.\n");
> +
> + libxl_device_usb_dispose(&usbdev);
> +
> +out:
> + return rc;
> +}
> +
> +int main_usb_remove(int argc, char **argv)
> +{
> + uint32_t domid = -1;
INVALID_DOMID
> + int opt = 0, rc;
> + const char *device = NULL;
> + int type = 0;
> +
> + SWITCH_FOREACH_OPT(opt, "d:v:", NULL, "usb-remove", 0) {
> + case 'd':
> + domid = find_domain(optarg);
> + break;
> + case 'v':
> + type = LIBXL_DEVICE_USB_TYPE_HOSTDEV;
> + device = optarg;
> + break;
> + }
> +
> + if ( domid == -1 ) {
> + fprintf(stderr, "Must specify domid\n\n");
> + help("usb-remove");
> + return 2;
> + }
> +
> + if ( !device ) {
> + fprintf(stderr, "Must specify a device\n\n");
> + help("usb-remove");
> + return 2;
> + }
> +
> + rc = usb_remove(domid, type, device);
> + if ( rc < 0 )
> + return 1;
> + else
> + return 0;
> +}
> +
> +static void usb_list(uint32_t domid)
> +{
> + libxl_device_usb *dev;
> + int num, i;
> +
> + dev = libxl_device_usb_list(ctx, domid, &num);
> + if (dev == NULL)
> + return;
> + printf("protocol backend type device\n");
> + for (i = 0; i < num; i++) {
> + printf("%8s ", (dev[i].protocol==LIBXL_USB_PROTOCOL_PV)?"pv":"dm");
> + printf("%7d ", dev[i].backend_domid);
> + printf("%7s ", (dev[i].type==LIBXL_DEVICE_USB_TYPE_HOSTDEV)?"hostdev":"unknown");
> + if(dev[i].type == LIBXL_DEVICE_USB_TYPE_HOSTDEV)
> + printf("%03d.%03d",
> + dev[i].u.hostdev.hostbus,
> + dev[i].u.hostdev.hostaddr);
> + printf("\n");
> + }
> + free(dev);
> +}
> +
> +
> +int main_usb_list(int argc, char **argv)
> +{
> + uint32_t domid = -1;
INVALID_DOMID
> + int opt;
> +
> + SWITCH_FOREACH_OPT(opt, "d:", NULL, "usb-list", 0) {
> + case 'd':
> + domid = find_domain(optarg);
> + break;
> + }
> +
> + if ( domid == -1 ) {
> + fprintf(stderr, "Must specify domid\n\n");
> + help("usb-remove");
> + return 2;
> + }
> +
> + usb_list(domid);
> + return 0;
> +}
> +
> +
> +
> int main_console(int argc, char **argv)
> {
> uint32_t domid;
> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> index b4a87ca..3cf8e65 100644
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
> @@ -187,6 +187,21 @@ struct cmd_spec cmd_table[] = {
> "Eject a cdrom from a guest's cd drive",
> "<Domain> <VirtualDevice>",
> },
> + { "usb-add",
> + &main_usb_add, 1, 1,
> + "Hot-plug a usb device to a domain.",
> + "-d <Domain> [-v <hostbus.hostaddr>]",
> + },
> + { "usb-remove",
> + &main_usb_remove, 1, 1,
> + "Hot-unplug a usb device from a domain.",
> + "-d <Domain> [-v <hostbus.hostaddr>]",
> + },
> + { "usb-list",
> + &main_usb_list, 0, 0,
> + "List usb devices for a domain",
> + "<Domain>",
> + },
> { "mem-max",
> &main_memmax, 0, 1,
> "Set the maximum amount reservation for a domain",
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/2] xl: Add commands for usb hot-plug
2013-04-17 10:02 ` Roger Pau Monné
@ 2013-04-17 10:03 ` George Dunlap
2013-04-17 10:15 ` Ian Campbell
2013-04-17 11:44 ` Ian Jackson
2 siblings, 0 replies; 22+ messages in thread
From: George Dunlap @ 2013-04-17 10:03 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: sstanisi@cbnco.com, Ian Jackson, xen-devel@lists.xen.org
On 17/04/13 11:02, Roger Pau Monne wrote:
> On 11/04/13 20:51, George Dunlap wrote:
>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>> CC: Ian Jackson <ian.jackson@citrix.com>
>> CC: Roger Pau Monne <roger.pau@citrix.com>
>> CC: sstanisi@cbnco.com
>> ---
>> docs/man/xl.pod.1 | 30 +++++++
>> tools/libxl/xl.h | 3 +
>> tools/libxl/xl_cmdimpl.c | 219 +++++++++++++++++++++++++++++++++++++++++++++
>> tools/libxl/xl_cmdtable.c | 15 ++++
>> 4 files changed, 267 insertions(+)
>>
>> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
>> index a0e298e..18a8eee 100644
>> --- a/docs/man/xl.pod.1
>> +++ b/docs/man/xl.pod.1
>> @@ -1110,6 +1110,36 @@ List virtual network interfaces for a domain.
>>
>> =back
>>
>> +=head2 USB DEVICES
>> +
>> +=over 4
>> +
>> +=item B<usb-add> I<-d domain-id> I<-v hosbus.hostaddr>
>> +
>> +Passes through the host USB device specified by I<hostbus.hostaddr>. At
>> +the moment this will only work for HVM domains via qemu.
>> +
>> +The best way to find out the information for the device is typically using
>> +lsusb.
>> +
>> +This command is only available for domains using qemu-xen, not
>> +qemu-traditional.
>> +
>> +=item B<usb-remove> I<-d domain-id> I<-v hosbus.hostaddr>
>> +
>> +Remove the host USB device from I<domain-id> which is specified
>> +by <hostbus.hostaddr>. This command only works for devices added
>> +with usb-add; not for those specified in the config file.
>> +
>> +This command is only available for domains using qemu-xen, not
>> +qemu-traditional.
>> +
>> +=item B<usb-list> I<domain-id>
>> +
>> +Show host USB devices assigned to the guest.
>> +
>> +=back
>> +
>> =head2 VTPM DEVICES
>>
>> =over 4
>> diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
>> index b881f92..5c39fa2 100644
>> --- a/tools/libxl/xl.h
>> +++ b/tools/libxl/xl.h
>> @@ -35,6 +35,9 @@ int main_info(int argc, char **argv);
>> int main_sharing(int argc, char **argv);
>> int main_cd_eject(int argc, char **argv);
>> int main_cd_insert(int argc, char **argv);
>> +int main_usb_add(int argc, char **argv);
>> +int main_usb_remove(int argc, char **argv);
>> +int main_usb_list(int argc, char **argv);
>> int main_console(int argc, char **argv);
>> int main_vncviewer(int argc, char **argv);
>> int main_pcilist(int argc, char **argv);
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 61f7b96..a690823 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -2600,6 +2600,225 @@ int main_cd_insert(int argc, char **argv)
>> return 0;
>> }
>>
>> +
>> +
>> +static int parse_usb_hostdev_specifier(libxl_device_usb *dev, const char *s)
>> +{
>> + const char * hostbus, *hostaddr, *p;
>> +
>> + hostbus = s;
>> + hostaddr=NULL;
>> +
>> +#define is_dec(_c) ((_c) >= '0' && (_c) <= '9')
>> +#define is_hex(_c) (is_dec(_c) || ((_c) >= 'a' && (_c) <= 'f'))
> This are kind of general macros, that could be used elsewhere, might be
> suitable to put them outside of this function and name them CHAR_IS_DEC
> and CHAR_IS_HEX.
Ack
>
>> +
>> + /* Match [0-9]+\.[0-9] */
>> + if (!is_dec(*hostbus))
>> + return -1;
>> +
>> + for(p=s; *p; p++) {
>> + if(*p == '.') {
>> + if ( !hostaddr )
>> + hostaddr = p+1;
>> + else {
>> + return -1;
>> + }
>> + } else if (!is_dec(*p)) {
>> + return -1;
>> + }
>> + }
>> + if (!hostaddr || !is_dec(*hostaddr))
>> + return -1;
>> + dev->u.hostdev.hostbus = strtoul(hostbus, NULL, 10);
>> + dev->u.hostdev.hostaddr = strtoul(hostaddr, NULL, 10);
>> +#undef is_dec
>> +#undef is_hex
>> +
>> + return 0;
>> +}
>> +
>> +static int usb_add(uint32_t domid, libxl_device_usb_type type,
>> + const char * device)
>> +{
>> + libxl_device_usb usbdev;
>> + int rc;
>> +
>> + libxl_device_usb_init(&usbdev);
>> +
>> + usbdev.type = type;
>> +
>> + switch(type) {
>> + case LIBXL_DEVICE_USB_TYPE_HOSTDEV:
>> + if ( parse_usb_hostdev_specifier(&usbdev, device) < 0 ) {
>> + rc = ERROR_FAIL;
>> + goto out;
>> + }
>> + break;
>> + default:
>> + fprintf(stderr, "INTERNAL ERROR: Unimplemented type.\n");
>> + rc = ERROR_FAIL;
>> + goto out;
>> + }
>> +
>> + if ( (rc = libxl_device_usb_add(ctx, domid, &usbdev, NULL)) < 0 )
>> + fprintf(stderr, "libxl_usb_add failed.\n");
>> +
>> + libxl_device_usb_dispose(&usbdev);
>> +
>> +out:
>> + return rc;
>> +}
>> +
>> +int main_usb_add(int argc, char **argv)
>> +{
>> + uint32_t domid = -1;
> You could use INVALID_DOMID that is defined early in the file.
Sorry, should have done that to begin with.
Thanks,
-George
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
2013-04-17 9:48 ` Roger Pau Monné
@ 2013-04-17 10:05 ` George Dunlap
2013-04-17 11:46 ` Ian Jackson
0 siblings, 1 reply; 22+ messages in thread
From: George Dunlap @ 2013-04-17 10:05 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: sstanisi@cbnco.com, Ian Jackson, xen-devel@lists.xen.org
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
2013-04-17 10:02 ` George Dunlap
@ 2013-04-17 10:13 ` Ian Campbell
2013-04-17 10:25 ` George Dunlap
0 siblings, 1 reply; 22+ messages in thread
From: Ian Campbell @ 2013-04-17 10:13 UTC (permalink / raw)
To: George Dunlap
Cc: sstanisi@cbnco.com, xen-devel@lists.xen.org, Ian Jackson,
Roger Pau Monne
On Wed, 2013-04-17 at 11:02 +0100, George Dunlap wrote:
> On 17/04/13 10:58, Ian Campbell wrote:
> > On Tue, 2013-04-16 at 19:00 +0100, Roger Pau Monné wrote:
> >
> >>> + libxl_usb_path = libxl__sprintf(gc, "%s/usb", libxl_path);
> >> I would recommend to use GCSPRINTF instead, it already checks for errors
> > GCSPRINTF and libxl__sprintf have the same amount of error checking.
> >
> > The more important point is that the libxl memory allocation
> > functions/macros all panic on ENOMEM so there is no need for code in
> > general to check for allocation errors.
> >
> > i.e. This block...
> >>> + if (!libxl_usb_path) {
> >>> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot allocate create paths");
> >>> + rc = ERROR_FAIL;
> >>> + goto out;
> >>> + }
> > ...is unneeded. I didn't check the rest of the patch but I imagine you
> > are in the habits of checking errors and can now remove a bunch of
> > code ;-)
>
> OK -- well in that block in particular, it's following suit with the
> rest of the surrounding code which does exactly the same thing.
That code predates the panic on ENOMEM behaviour.
> As I
> said in another e-mail to roger about error checking in this function, I
> think that it's more important for programmers to be able to see this is
> exactly the same as all the rest. At some point there should probably
> be a clean-up to remove the check from all of them, but I don't think
> that's worth the risk at this point in the release cycle.
Rather than having a massive flag day we are taking the approach of
fixing code as it is changed and part of that is to not introduce new
"incorrect" code.
> I can put cleaning up this function on my to-do list for the 4.4 dev
> cycle if you want.
That would also be good of course.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/2] xl: Add commands for usb hot-plug
2013-04-17 10:02 ` Roger Pau Monné
2013-04-17 10:03 ` George Dunlap
@ 2013-04-17 10:15 ` Ian Campbell
2013-04-17 10:20 ` George Dunlap
2013-04-17 11:48 ` Ian Jackson
2013-04-17 11:44 ` Ian Jackson
2 siblings, 2 replies; 22+ messages in thread
From: Ian Campbell @ 2013-04-17 10:15 UTC (permalink / raw)
To: Roger Pau Monné
Cc: George Dunlap, sstanisi@cbnco.com, Ian Jackson,
xen-devel@lists.xen.org
On Wed, 2013-04-17 at 11:02 +0100, Roger Pau Monné wrote:
> > +#define is_dec(_c) ((_c) >= '0' && (_c) <= '9')
> > +#define is_hex(_c) (is_dec(_c) || ((_c) >= 'a' && (_c) <= 'f'))
>
> This are kind of general macros, that could be used elsewhere, might be
> suitable to put them outside of this function and name them CHAR_IS_DEC
> and CHAR_IS_HEX.
ctype.h already provides isdigit() and isxdigit(), no need for our own.
IMHO.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/2] xl: Add commands for usb hot-plug
2013-04-17 10:15 ` Ian Campbell
@ 2013-04-17 10:20 ` George Dunlap
2013-04-17 11:48 ` Ian Jackson
1 sibling, 0 replies; 22+ messages in thread
From: George Dunlap @ 2013-04-17 10:20 UTC (permalink / raw)
To: Ian Campbell
Cc: sstanisi@cbnco.com, xen-devel@lists.xen.org, Ian Jackson,
Roger Pau Monne
On 17/04/13 11:15, Ian Campbell wrote:
> On Wed, 2013-04-17 at 11:02 +0100, Roger Pau Monné wrote:
>
>>> +#define is_dec(_c) ((_c) >= '0' && (_c) <= '9')
>>> +#define is_hex(_c) (is_dec(_c) || ((_c) >= 'a' && (_c) <= 'f'))
>> This are kind of general macros, that could be used elsewhere, might be
>> suitable to put them outside of this function and name them CHAR_IS_DEC
>> and CHAR_IS_HEX.
> ctype.h already provides isdigit() and isxdigit(), no need for our own.
> IMHO.
Oh right -- sorry, when I was looking at these I didn't see isxdigit().
That's much better.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
2013-04-17 10:13 ` Ian Campbell
@ 2013-04-17 10:25 ` George Dunlap
0 siblings, 0 replies; 22+ messages in thread
From: George Dunlap @ 2013-04-17 10:25 UTC (permalink / raw)
To: Ian Campbell
Cc: sstanisi@cbnco.com, xen-devel@lists.xen.org, Ian Jackson,
Roger Pau Monne
On 17/04/13 11:13, Ian Campbell wrote:
> On Wed, 2013-04-17 at 11:02 +0100, George Dunlap wrote:
>> On 17/04/13 10:58, Ian Campbell wrote:
>>> On Tue, 2013-04-16 at 19:00 +0100, Roger Pau Monné wrote:
>>>
>>>>> + libxl_usb_path = libxl__sprintf(gc, "%s/usb", libxl_path);
>>>> I would recommend to use GCSPRINTF instead, it already checks for errors
>>> GCSPRINTF and libxl__sprintf have the same amount of error checking.
>>>
>>> The more important point is that the libxl memory allocation
>>> functions/macros all panic on ENOMEM so there is no need for code in
>>> general to check for allocation errors.
>>>
>>> i.e. This block...
>>>>> + if (!libxl_usb_path) {
>>>>> + LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot allocate create paths");
>>>>> + rc = ERROR_FAIL;
>>>>> + goto out;
>>>>> + }
>>> ...is unneeded. I didn't check the rest of the patch but I imagine you
>>> are in the habits of checking errors and can now remove a bunch of
>>> code ;-)
>> OK -- well in that block in particular, it's following suit with the
>> rest of the surrounding code which does exactly the same thing.
> That code predates the panic on ENOMEM behaviour.
>
>> As I
>> said in another e-mail to roger about error checking in this function, I
>> think that it's more important for programmers to be able to see this is
>> exactly the same as all the rest. At some point there should probably
>> be a clean-up to remove the check from all of them, but I don't think
>> that's worth the risk at this point in the release cycle.
> Rather than having a massive flag day we are taking the approach of
> fixing code as it is changed and part of that is to not introduce new
> "incorrect" code.
I think that makes sense on a function-by-function, or perhaps
file-by-file basis, but I personally think it's a bad idea to mix
"templates" in one function like that. But if that's the plan, I'll go
with it.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
2013-04-17 9:36 ` George Dunlap
2013-04-17 9:48 ` Roger Pau Monné
2013-04-17 9:54 ` Ian Campbell
@ 2013-04-17 11:41 ` Ian Jackson
2 siblings, 0 replies; 22+ messages in thread
From: Ian Jackson @ 2013-04-17 11:41 UTC (permalink / raw)
To: George Dunlap
Cc: sstanisi@cbnco.com, xen-devel@lists.xen.org, Roger Pau Monne
George Dunlap writes ("Re: [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest"):
> >> + 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.
We would normally write this instead:
+ rc = libxl__qmp_usb_remove(gc, domid, usbdev);
+ if (rc) goto out;
I'm assuming libxl__qmp_usb_remove returns 0 on success. If it
doesn't you and you want to throw away the information it does return,
+ if (rc<0) goto out;
Ian.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/2] xl: Add commands for usb hot-plug
2013-04-17 10:02 ` Roger Pau Monné
2013-04-17 10:03 ` George Dunlap
2013-04-17 10:15 ` Ian Campbell
@ 2013-04-17 11:44 ` Ian Jackson
2 siblings, 0 replies; 22+ messages in thread
From: Ian Jackson @ 2013-04-17 11:44 UTC (permalink / raw)
To: Roger Pau Monne
Cc: George Dunlap, sstanisi@cbnco.com, xen-devel@lists.xen.org
Roger Pau Monne writes ("Re: [PATCH v4 2/2] xl: Add commands for usb hot-plug"):
> On 11/04/13 20:51, George Dunlap wrote:
...
> > +#define is_dec(_c) ((_c) >= '0' && (_c) <= '9')
> > +#define is_hex(_c) (is_dec(_c) || ((_c) >= 'a' && (_c) <= 'f'))
>
> This are kind of general macros, that could be used elsewhere, might be
> suitable to put them outside of this function and name them CHAR_IS_DEC
> and CHAR_IS_HEX.
Also, while we're discussing this: the _'s at the start of the macro
formal parameter names are unnecessary, and not usual in libxl.
Ian.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
2013-04-17 10:05 ` George Dunlap
@ 2013-04-17 11:46 ` Ian Jackson
0 siblings, 0 replies; 22+ messages in thread
From: Ian Jackson @ 2013-04-17 11:46 UTC (permalink / raw)
To: George Dunlap
Cc: sstanisi@cbnco.com, xen-devel@lists.xen.org, Roger Pau Monne
George Dunlap writes ("Re: [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:
> > 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.
Personally I think it's more important not to add new bugs, than to
avoid possibly confusing future readers of the code. It's true that
we haven't gone through everywhere and fixed this up, but I think that
new code should be correct.
Ian.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/2] xl: Add commands for usb hot-plug
2013-04-17 10:15 ` Ian Campbell
2013-04-17 10:20 ` George Dunlap
@ 2013-04-17 11:48 ` Ian Jackson
1 sibling, 0 replies; 22+ messages in thread
From: Ian Jackson @ 2013-04-17 11:48 UTC (permalink / raw)
To: Ian Campbell
Cc: George Dunlap, sstanisi@cbnco.com, xen-devel@lists.xen.org,
Roger Pau Monne
Ian Campbell writes ("Re: [Xen-devel] [PATCH v4 2/2] xl: Add commands for usb hot-plug"):
> On Wed, 2013-04-17 at 11:02 +0100, Roger Pau Monné wrote:
> > This are kind of general macros, that could be used elsewhere, might be
> > suitable to put them outside of this function and name them CHAR_IS_DEC
> > and CHAR_IS_HEX.
>
> ctype.h already provides isdigit() and isxdigit(), no need for our own.
> IMHO.
These functions' behaviour depends on the locale. That may or may not
be desirable.
If you do use isdigit, be sure to invoke it via the CTYPE macro in
libxl_internal.h:
/*
* int CTYPE(ISFOO, char c);
* int CTYPE(toupper, char c);
* int CTYPE(tolower, char c);
*
* This is necessary because passing a simple char to a ctype.h
* is forbidden. ctype.h macros take ints derived from _unsigned_ chars.
*
* If you have a char which might be EOF then you should already have
* it in an int representing an unsigned char, and you can use the
* <ctype.h> macros directly. This generally happens only with values
* from fgetc et al.
*
* For any value known to be a character (eg, anything that came from
* a char[]), use CTYPE.
*/
#define CTYPE(isfoo,c) (isfoo((unsigned char)(c)))
Ian.
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2013-04-17 11:48 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-11 18:51 [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest George Dunlap
2013-04-11 18:51 ` [PATCH v4 2/2] xl: Add commands for usb hot-plug George Dunlap
2013-04-17 10:02 ` Roger Pau Monné
2013-04-17 10:03 ` George Dunlap
2013-04-17 10:15 ` Ian Campbell
2013-04-17 10:20 ` George Dunlap
2013-04-17 11:48 ` Ian Jackson
2013-04-17 11:44 ` Ian Jackson
2013-04-11 18:55 ` [PATCH v4 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest George Dunlap
2013-04-16 15:38 ` George Dunlap
2013-04-16 18:00 ` Roger Pau Monné
2013-04-17 9:36 ` George Dunlap
2013-04-17 9:48 ` Roger Pau Monné
2013-04-17 10:05 ` George Dunlap
2013-04-17 11:46 ` Ian Jackson
2013-04-17 9:54 ` Ian Campbell
2013-04-17 9:59 ` George Dunlap
2013-04-17 11:41 ` Ian Jackson
2013-04-17 9:58 ` Ian Campbell
2013-04-17 10:02 ` George Dunlap
2013-04-17 10:13 ` Ian Campbell
2013-04-17 10:25 ` George Dunlap
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.