George Dunlap
2013-Apr-18 14:46 UTC
[PATCH v5 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
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.
History:
v5:
- fix erroneous use of NOGC in qmp functions
- Don''t check return of libxl__sprintf in libxl_create.c
- Check return values of new xs actions in libxl_create.c
- Use updated template for xenstore transactions, do checks
- use xs_read_checked
- Fixed if (x) spacing to match coding style
- use GCSFPRINTF
- use libxl__calloc
- Fix comment for LIBXL_HAVE_USB
- use idl-generated *_from_string() and *_to_string() functions
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 | 37 +++
tools/libxl/libxl_create.c | 11 +-
tools/libxl/libxl_internal.h | 18 ++
tools/libxl/libxl_qmp.c | 66 +++++
tools/libxl/libxl_types.idl | 22 ++
tools/libxl/libxl_usb.c | 522 ++++++++++++++++++++++++++++++++++++++++
tools/ocaml/libs/xl/genwrap.py | 1 +
8 files changed, 677 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 4922313..b6edd15 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -75,6 +75,12 @@
#define LIBXL_HAVE_FIRMWARE_PASSTHROUGH 1
/*
+ * LIBXL_HAVE_USB indicates the functions for doing hot-plug of
+ * USB devices.
+ */
+#define LIBXL_HAVE_USB 1
+
+/*
* libxl ABI compatibility
*
* The only guarantee which libxl makes regarding ABI compatibility
@@ -735,6 +741,37 @@ 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 these protocols is available:
+ * - PV (i.e., PVUSB)
+ * - DEVICEMODEL (i.e, qemu)
+ *
+ * PV is available for either PV or HVM domains. DEVICEMODEL is only
+ * available for HVM domains. 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 ae72f21..3a84bf2 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -393,7 +393,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];
@@ -454,6 +454,8 @@ int libxl__domain_make(libxl__gc *gc,
libxl_domain_create_info *info,
goto out;
}
+ libxl_usb_path = GCSPRINTF("%s/usb", libxl_path);
+
noperm[0].id = 0;
noperm[0].perms = XS_PERM_NONE;
@@ -477,6 +479,13 @@ retry_transaction:
xs_rm(ctx->xsh, t, libxl_path);
libxl__xs_mkdir(gc, t, libxl_path, noperm, ARRAY_SIZE(noperm));
+ /* Helpfully, libxl__xs_rm_checked() returns 0 on success... */
+ rc = libxl__xs_rm_checked(gc, t, libxl_usb_path);
+ if (rc) goto out;
+ /* ...but libxl__xs_mkdir() returns 1 on success, 0 on failure. */
+ rc = libxl__xs_mkdir(gc, t, libxl_usb_path, noperm, ARRAY_SIZE(noperm));
+ if (!rc) goto out;
+
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..6d5f56b 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 = GCSPRINTF(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 = GCSPRINTF(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 fcb1ecd..3c6a709 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -408,6 +408,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..ce52f5e
--- /dev/null
+++ b/tools/libxl/libxl_usb.c
@@ -0,0 +1,522 @@
+/*
+ * 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.
+ */
+static char * hostdev_xs_path(libxl__gc *gc, uint32_t domid,
+ libxl__device_usb *usbdev)
+{
+ return GCSPRINTF(USB_INFO_PATH "/%s",
+ libxl__xs_libxl_path(gc, domid),
+ GCSPRINTF(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",
+ GCSPRINTF("%d",
+ usbdev->u.hostdev.hostbus));
+ flexarray_append_pair(a, "hostaddr",
+ GCSPRINTF("%d",
+ usbdev->u.hostdev.hostaddr));
+}
+
+static int read_hostdev_xenstore_entry(libxl__gc *gc, const char * path,
+ libxl__device_usb *usbdev)
+{
+ int rc;
+ const char * val;
+
+ rc = libxl__xs_read_checked(gc, XBT_NULL,
+ GCSPRINTF("%s/hostbus", path),
&val);
+ if (rc) goto out;
+ usbdev->u.hostdev.hostbus = atoi(val);
+
+ rc = libxl__xs_read_checked(gc, XBT_NULL,
+ GCSPRINTF("%s/hostaddr", path),
&val);
+ if (rc) goto out;
+ usbdev->u.hostdev.hostaddr = atoi(val);
+
+ rc = 0;
+out:
+ return rc;
+}
+
+static int usb_read_xenstore(libxl__gc *gc, const char * path,
+ libxl__device_usb *usbdev)
+{
+ const char *val;
+ int rc;
+
+ rc = libxl__xs_read_checked(gc, XBT_NULL,
+ GCSPRINTF("%s/protocol", path),
&val);
+ if (rc) goto out;
+ rc = libxl_usb_protocol_from_string(val, &usbdev->protocol);
+ if (rc) goto out;
+
+ rc = libxl__xs_read_checked(gc, XBT_NULL,
+ GCSPRINTF("%s/backend_domid", path),
&val);
+ if (rc) goto out;
+ usbdev->backend_domid = atoi(val);
+
+ rc = libxl__xs_read_checked(gc, XBT_NULL,
+ GCSPRINTF("%s/type", path),
&val);
+ if (rc) goto out;
+ rc = libxl_device_usb_type_from_string(val, &usbdev->type);
+ if (rc) goto out;
+
+ 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)");
+ goto out;
+ }
+
+ rc = 0;
+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 = 0;
+ struct xs_permissions noperm[1];
+ int rc = 0;
+
+ noperm[0].id = 0;
+ noperm[0].perms = XS_PERM_NONE;
+
+ dev = flexarray_make(gc, 16, 1);
+
+ flexarray_append_pair(dev, "protocol",
+ GCSPRINTF("%s",
+
libxl_usb_protocol_to_string(usbdev->protocol)));
+
+ flexarray_append_pair(dev, "backend_domid",
+ GCSPRINTF("%d", usbdev->backend_domid));
+
+ flexarray_append_pair(dev, "type",
+ GCSPRINTF("%s",
+
libxl_device_usb_type_to_string(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");
+
+ for(;;) {
+ rc = libxl__xs_transaction_start(gc, &t);
+ if (rc) goto out;
+
+ /* Helpfully, mkdir returns 0 on failure, 1 on success */
+ rc = libxl__xs_mkdir(gc, t, dev_path, noperm, ARRAY_SIZE(noperm));
+ if (!rc) goto out;
+
+ /* And libxl__xs_writev *always* returns 0 no matter what */
+ libxl__xs_writev(gc, t, dev_path,
+ libxl__xs_kvs_of_flexarray(gc, dev, dev->count));
+
+ rc = libxl__xs_transaction_commit(gc, &t);
+ if (!rc) break;
+ if (rc <0) goto out;
+ }
+
+out:
+ return rc;
+}
+
+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 = GCSPRINTF(USB_INFO_PATH,
+ libxl__xs_libxl_path(gc, domid));
+
+ devlist = libxl__xs_directory(gc, XBT_NULL, dompath, &nd);
+ if (!devlist)
+ goto out;
+
+ *list = libxl__calloc(gc, nd, sizeof(libxl__device_usb));
+
+ for(i = 0; i < nd; i++) {
+ char *path;
+
+ path = GCSPRINTF("%s/%s", dompath, devlist[i]);
+
+ rc = usb_read_xenstore(gc, path, (*list) + i);
+ if (rc) {
+ *list = NULL;
+ *num = 0;
+ goto out;
+ }
+ }
+
+ *num = nd;
+
+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:
+ rc = libxl__qmp_usb_add(gc, domid, usbdev);
+ if (rc)
+ 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:
+ rc = libxl__qmp_usb_remove(gc, domid, usbdev);
+ if (rc)
+ 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 = libxl__calloc(NOGC, 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
v5:
- Remove extraneous is_hex
- Replace is_dec with ctype.h isdigit()
- Replace domid -1 with INVALID_DOMID
- Fix up a couple of mistaken function names in strings
- Copy CTYPE macro from libxl_internal.h
- Fix to usb-list manpage (use -d domid)
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 | 21 +++++
tools/libxl/xl_cmdimpl.c | 214 +++++++++++++++++++++++++++++++++++++++++++++
tools/libxl/xl_cmdtable.c | 15 ++++
4 files changed, 280 insertions(+)
diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 57c6a79..f6d61e4 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1147,6 +1147,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<-d 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 771b4af..0ded61e 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);
@@ -164,6 +167,24 @@ extern void printf_info_sexp(int domid, libxl_domain_config
*d_config);
#define XL_GLOBAL_CONFIG XEN_CONFIG_DIR "/xl.conf"
#define XL_LOCK_FILE XEN_LOCK_DIR "/xl"
+/*
+ * 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)))
+
#endif /* XL_H */
/*
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 98ecf67..e357eb1 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2602,6 +2602,220 @@ 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;
+
+ /* Match [0-9]+\.[0-9] */
+ if (!CTYPE(isdigit,*hostbus))
+ return -1;
+
+ for(p=s; *p; p++) {
+ if(*p == ''.'') {
+ if ( !hostaddr )
+ hostaddr = p+1;
+ else {
+ return -1;
+ }
+ } else if (!CTYPE(isdigit,*p)) {
+ return -1;
+ }
+ }
+ if (!hostaddr || !CTYPE(isdigit,*hostaddr))
+ return -1;
+ dev->u.hostdev.hostbus = strtoul(hostbus, NULL, 10);
+ dev->u.hostdev.hostaddr = strtoul(hostaddr, NULL, 10);
+
+ 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_device_usb_add failed.\n");
+
+ libxl_device_usb_dispose(&usbdev);
+
+out:
+ return rc;
+}
+
+int main_usb_add(int argc, char **argv)
+{
+ uint32_t domid = INVALID_DOMID;
+ 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 == INVALID_DOMID ) {
+ 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_device_usb_remove failed.\n");
+
+ libxl_device_usb_dispose(&usbdev);
+
+out:
+ return rc;
+}
+
+int main_usb_remove(int argc, char **argv)
+{
+ uint32_t domid = 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 == INVALID_DOMID ) {
+ 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 = INVALID_DOMID;
+ int opt;
+
+ SWITCH_FOREACH_OPT(opt, "d:", NULL, "usb-list", 0) {
+ case ''d'':
+ domid = find_domain(optarg);
+ break;
+ }
+
+ if ( domid == INVALID_DOMID ) {
+ fprintf(stderr, "Must specify domid\n\n");
+ help("usb-list");
+ 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 00899f5..a21b1d4 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",
+ "-d <Domain>",
+ },
{ "mem-max",
&main_memmax, 0, 1,
"Set the maximum amount reservation for a domain",
--
1.7.9.5
George Dunlap writes ("[PATCH v5 2/2] xl: Add commands for usb
hot-plug"):> +=item B<usb-add> I<-d domain-id> I<-v hosbus.hostaddr>
> +
Shouldn''t these be more like `xl
{disk,network}-{attach,detach}'' ?
I was imagining something like
xl usb-attach debian.guest.osstest host:046d:c014
? (Perhaps with
xl usb-remove --protocol hvm debian.guest.osstest tablet
coming later at some point.)
> +static int parse_usb_hostdev_specifier(libxl_device_usb *dev, const char
*s)
> +{
> + const char * hostbus, *hostaddr, *p;
> +
> + hostbus = s;
> + hostaddr=NULL;
> +
> + /* Match [0-9]+\.[0-9] */
> + if (!CTYPE(isdigit,*hostbus))
> + return -1;
> +
> + for(p=s; *p; p++) {
> + if(*p == ''.'') {
> + if ( !hostaddr )
> + hostaddr = p+1;
> + else {
> + return -1;
> + }
> + } else if (!CTYPE(isdigit,*p)) {
> + return -1;
> + }
> + }
> + if (!hostaddr || !CTYPE(isdigit,*hostaddr))
> + return -1;
> + dev->u.hostdev.hostbus = strtoul(hostbus, NULL, 10);
> + dev->u.hostdev.hostaddr = strtoul(hostaddr, NULL, 10);
Perhaps it would be easier to use the 2nd argument to strtoul ?
Or strchr ?
Also, some style quibbles: spaces should appear after the keyword in
"for (" and "if (" and not inside the parens in "if
(!hostaddr)". And
it seems odd to write an if without braces for the one-line if branch
and with braces for the one-line else branch. You have quite a lot of
these...
> +static int usb_add(uint32_t domid, libxl_device_usb_type type,
> + const char * device)
^
This indent is odd.
> + if ( (rc = libxl_device_usb_add(ctx, domid, &usbdev, NULL)) < 0
)
> + fprintf(stderr, "libxl_device_usb_add failed.\n");
Can''t we have the call and the assignment to rc on its own line ?
Ian.
Ian Jackson
2013-Apr-18 17:51 UTC
Re: [PATCH v5 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
George Dunlap writes ("[PATCH v5 1/2] libxl: Introduce functions to add and
remove USB devices to an HVM guest"):> + /* Helpfully, libxl__xs_rm_checked() returns 0 on success... */
> + rc = libxl__xs_rm_checked(gc, t, libxl_usb_path);
> + if (rc) goto out;
> + /* ...but libxl__xs_mkdir() returns 1 on success, 0 on failure. */
> + rc = libxl__xs_mkdir(gc, t, libxl_usb_path, noperm,
ARRAY_SIZE(noperm));
> + if (!rc) goto out;
Is this really right ? If mkdir fails, you cause libxl__domain_make
to immediately return success. The out label expects the error code
to be in rc. I think it''s a stylistic error to assign the
(deprecated) boolean return value from libxl__xs_mkdir to rc; rc would
normally contain a libxl error code.
> + id = GCSPRINTF(HOST_USB_QDEV_ID,
> + (uint16_t) dev->u.hostdev.hostbus,
> + (uint16_t) dev->u.hostdev.hostaddr);
I think the style in libxl is to cuddle casts.
> + switch(usbdev->type) {
Missing space. (etc.) And some overly long lines.
Most of the rest of this looks plausible although I wish you wouldn''t
write things like this:
> + switch(usbdev->type) {
> + case LIBXL_DEVICE_USB_TYPE_HOSTDEV:
> + if ((rc = read_hostdev_xenstore_entry(gc, path, usbdev)) < 0)
> + goto out;
I guess old habits are hard to break :-).
I haven''t checked the semantics and timing of the xenstore/qmp
protocol in detail. But let me ask a question:
Suppose attempted attach or remove of a usb device fails halfway
through (eg, the user presses ^C and xl just stops since it doesn''t
handle SIGINT). Is the situation now (a) reported in the usb device
listing (b) capable of being cleaned up with a remove operation (c)
tidied up on domain destroy ? (Is this true at all times - ie does it
never go through a state where things won''t be cleaned up?)
Ian.
On 18/04/13 18:41, Ian Jackson wrote:> George Dunlap writes ("[PATCH v5 2/2] xl: Add commands for usb hot-plug"): >> +=item B<usb-add> I<-d domain-id> I<-v hosbus.hostaddr> >> + > Shouldn''t these be more like `xl {disk,network}-{attach,detach}'' ? > > I was imagining something like > xl usb-attach debian.guest.osstest host:046d:c014 > ? (Perhaps with > xl usb-remove --protocol hvm debian.guest.osstest tablet > coming later at some point.)I''m not really a fan of the "use position to specify arguments" when there is more than one or two arguments; but that''s what block/net/pci do, so we should probably follow suit. Hmm, a bit strange that the libxl functions are "libxl_device_[foo]_add()", but the xl commands are "[foo]-attach". Ah well -- I''ll change that around too. One of the reasons I didn''t do this was because I didn''t want to have to implement a parser to figure out which kind of device to implement. But an advantage of doing that is that we could at some point move from the sort of hacky, qemu-specific way of specifying usb devices for HVM guests in the config file (which basically passes them on directly to the qemu command-line via a deprecated interface), and move to a unified way to specify USB devices for either PV or HVM (with, for example, hot-unplug of devices which were added at boot time). Let me take a look and see if I can steal a parsing function from someone else. Any recommendations? :-)> >> +static int parse_usb_hostdev_specifier(libxl_device_usb *dev, const char *s) >> +{ >> + const char * hostbus, *hostaddr, *p; >> + >> + hostbus = s; >> + hostaddr=NULL; >> + >> + /* Match [0-9]+\.[0-9] */ >> + if (!CTYPE(isdigit,*hostbus)) >> + return -1; >> + >> + for(p=s; *p; p++) { >> + if(*p == ''.'') { >> + if ( !hostaddr ) >> + hostaddr = p+1; >> + else { >> + return -1; >> + } >> + } else if (!CTYPE(isdigit,*p)) { >> + return -1; >> + } >> + } >> + if (!hostaddr || !CTYPE(isdigit,*hostaddr)) >> + return -1; >> + dev->u.hostdev.hostbus = strtoul(hostbus, NULL, 10); >> + dev->u.hostdev.hostaddr = strtoul(hostaddr, NULL, 10); > Perhaps it would be easier to use the 2nd argument to strtoul ? > Or strchr ?I''ll see about that when looking at the parsing functions.> > Also, some style quibbles: spaces should appear after the keyword in > "for (" and "if (" and not inside the parens in "if (!hostaddr)". And > it seems odd to write an if without braces for the one-line if branch > and with braces for the one-line else branch. You have quite a lot of > these...Ack. Sorry, for some reason didn''t go through and check for stylistic things in this patch.> >> +static int usb_add(uint32_t domid, libxl_device_usb_type type, >> + const char * device) > ^ > This indent is odd.Ack. Didn''t re-indent after removing "hvm_host_" from the function name.> >> + if ( (rc = libxl_device_usb_add(ctx, domid, &usbdev, NULL)) < 0 ) >> + fprintf(stderr, "libxl_device_usb_add failed.\n"); > Can''t we have the call and the assignment to rc on its own line ?If you like. :-) -George
George Dunlap
2013-Apr-19 10:18 UTC
Re: [PATCH v5 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
On 18/04/13 18:51, Ian Jackson wrote:> George Dunlap writes ("[PATCH v5 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest"): >> + /* Helpfully, libxl__xs_rm_checked() returns 0 on success... */ >> + rc = libxl__xs_rm_checked(gc, t, libxl_usb_path); >> + if (rc) goto out; >> + /* ...but libxl__xs_mkdir() returns 1 on success, 0 on failure. */ >> + rc = libxl__xs_mkdir(gc, t, libxl_usb_path, noperm, ARRAY_SIZE(noperm)); >> + if (!rc) goto out; > Is this really right ? If mkdir fails, you cause libxl__domain_make > to immediately return success. The out label expects the error code > to be in rc. I think it''s a stylistic error to assign the > (deprecated) boolean return value from libxl__xs_mkdir to rc; rc would > normally contain a libxl error code.Yes, you''re right on both points. Good catch.> >> + id = GCSPRINTF(HOST_USB_QDEV_ID, >> + (uint16_t) dev->u.hostdev.hostbus, >> + (uint16_t) dev->u.hostdev.hostaddr); > I think the style in libxl is to cuddle casts.Ack. I''ve never thought of variables as cuddly before...> >> + switch(usbdev->type) { > Missing space. (etc.) And some overly long lines.Ack.> > Most of the rest of this looks plausible although I wish you wouldn''t > write things like this: > >> + switch(usbdev->type) { >> + case LIBXL_DEVICE_USB_TYPE_HOSTDEV: >> + if ((rc = read_hostdev_xenstore_entry(gc, path, usbdev)) < 0) >> + goto out; > I guess old habits are hard to break :-).Well I prefer that way -- having the ''if'' on a separate line actually annoys me (mildly). "Do this and handle the error if any" seems like one operation in my mind, not two. But I don''t mind changing it.> I haven''t checked the semantics and timing of the xenstore/qmp > protocol in detail. But let me ask a question: > > Suppose attempted attach or remove of a usb device fails halfway > through (eg, the user presses ^C and xl just stops since it doesn''t > handle SIGINT). Is the situation now (a) reported in the usb device > listing (b) capable of being cleaned up with a remove operation (c) > tidied up on domain destroy ? (Is this true at all times - ie does it > never go through a state where things won''t be cleaned up?)As far as I could determine, there is absolutely no way to query qemu to find out what devices are actually attached at the moment; that''s the whole reason for storing stuff in xenstore in the first place. So yes -- if you kill the process after the qmp command has been issued but before writing something in xenstore, then usb-list will not show the device, usb-remove will return an error if you attempt to remove the device. usb-add will issue the qmp command, which will then fail. I think this is probably true of most of the add/remove functions. We could add an option that says "attempt to do the remove even if you don''t see the device in xenstore" I suppose. But I''m not really sure how to document that in a useful way without basically describing the internal mechanics of the command. There''s nothing we can really do about usb-list though. On the whole I think I''d rather just say, "Don''t do that." -George
Ian Campbell
2013-Apr-19 10:50 UTC
Re: [PATCH v5 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
On Fri, 2013-04-19 at 11:18 +0100, George Dunlap wrote:> > > We could add an option that says "attempt to do the remove even if > you > don''t see the device in xenstore" I suppose. But I''m not really sure > how to document that in a useful way without basically describing the > internal mechanics of the command. > > There''s nothing we can really do about usb-list though.You could write it to xenstore before attempting the qmp commands. That just relies on the qmp remove failing gracefully for devices which aren''t actually attached. You might also be able to come up with some complicated, prepare, do it, commit in xenstore like approach -- but that''s probably a) difficult (if not impossibly due to races) and b) unnecessary. Ian.
George Dunlap
2013-Apr-19 10:53 UTC
Re: [PATCH v5 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
On 18/04/13 18:51, Ian Jackson wrote:> George Dunlap writes ("[PATCH v5 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest"): >> + /* Helpfully, libxl__xs_rm_checked() returns 0 on success... */ >> + rc = libxl__xs_rm_checked(gc, t, libxl_usb_path); >> + if (rc) goto out; >> + /* ...but libxl__xs_mkdir() returns 1 on success, 0 on failure. */ >> + rc = libxl__xs_mkdir(gc, t, libxl_usb_path, noperm, ARRAY_SIZE(noperm)); >> + if (!rc) goto out; > Is this really right ? If mkdir fails, you cause libxl__domain_make > to immediately return success. The out label expects the error code > to be in rc. I think it''s a stylistic error to assign the > (deprecated) boolean return value from libxl__xs_mkdir to rc; rc would > normally contain a libxl error code. > >> + id = GCSPRINTF(HOST_USB_QDEV_ID, >> + (uint16_t) dev->u.hostdev.hostbus, >> + (uint16_t) dev->u.hostdev.hostaddr); > I think the style in libxl is to cuddle casts. > >> + switch(usbdev->type) { > Missing space. (etc.) And some overly long lines. > > Most of the rest of this looks plausible although I wish you wouldn''t > write things like this: > >> + switch(usbdev->type) { >> + case LIBXL_DEVICE_USB_TYPE_HOSTDEV: >> + if ((rc = read_hostdev_xenstore_entry(gc, path, usbdev)) < 0) >> + goto out; > I guess old habits are hard to break :-). > > I haven''t checked the semantics and timing of the xenstore/qmp > protocol in detail. But let me ask a question: > > Suppose attempted attach or remove of a usb device fails halfway > through (eg, the user presses ^C and xl just stops since it doesn''t > handle SIGINT). Is the situation now (a) reported in the usb device > listing (b) capable of being cleaned up with a remove operation (c) > tidied up on domain destroy ? (Is this true at all times - ie does it > never go through a state where things won''t be cleaned up?)Re domain destroy: AFAIK everything is completely cleaned up on domain destroy. All lf the xenstore directories are removed, and qemu will release the device when it shuts down. This actually has been tested when I messed up something in storing the xenstore stuff; I didn''t have to reboot the host, just shutdown the guest, re-build the library, start the guest again and re-test. -George
George Dunlap
2013-Apr-19 11:00 UTC
Re: [PATCH v5 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
On 19/04/13 11:50, Ian Campbell wrote:> On Fri, 2013-04-19 at 11:18 +0100, George Dunlap wrote: >> >> We could add an option that says "attempt to do the remove even if >> you >> don''t see the device in xenstore" I suppose. But I''m not really sure >> how to document that in a useful way without basically describing the >> internal mechanics of the command. >> >> There''s nothing we can really do about usb-list though. > You could write it to xenstore before attempting the qmp commands. That > just relies on the qmp remove failing gracefully for devices which > aren''t actually attached.qmp remove will just return an error if you attempt to remove a device id it doesn''t know about. I suppose that could work. It''s also a bit more robust against, say, the xenstore add failing for whatever reason.> You might also be able to come up with some complicated, prepare, do it, > commit in xenstore like approach -- but that''s probably a) difficult (if > not impossibly due to races) and b) unnecessary.Well if the failure mode is the process executing the libxl function being killed at a random point, I don''t think we can do a transaction thing -- it would have to coordinate between qmp and xenstore. AFAIK qmp doesn''t have any transaction support, so who is going to do the cleanup? The process that was just killed? If it could, it could just finish whatever it was doing anyway. -George
George Dunlap
2013-Apr-19 14:14 UTC
Re: [PATCH v5 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
On 19/04/13 12:00, George Dunlap wrote:> On 19/04/13 11:50, Ian Campbell wrote: >> On Fri, 2013-04-19 at 11:18 +0100, George Dunlap wrote: >>> We could add an option that says "attempt to do the remove even if >>> you >>> don''t see the device in xenstore" I suppose. But I''m not really sure >>> how to document that in a useful way without basically describing the >>> internal mechanics of the command. >>> >>> There''s nothing we can really do about usb-list though. >> You could write it to xenstore before attempting the qmp commands. That >> just relies on the qmp remove failing gracefully for devices which >> aren''t actually attached. > qmp remove will just return an error if you attempt to remove a device > id it doesn''t know about. I suppose that could work. It''s also a bit > more robust against, say, the xenstore add failing for whatever reason.Hmm, if the qmp_remove fails, we don''t know if that''s because because the device didn''t exist, or because it failed for some other reason. If qmp remove fails, we shouldn''t touch the xenstore entry. So I think you get in to the same basic problem here, except that you get an un-removeable xenstore entry instead of an unremoveable USB device. -George