George Dunlap
2012-Nov-30 16:24 UTC
[PATCH 0 of 4 v2] Allow xl to build a domain with multiple USB devices
v2 adds two patches. The first consoldates the device model argument builder for qemu-traditional and qemu-upstream into a single function, to make it easier to keep up to date. Along the way, the first patch checks to make sure that the vnc display is not set in both "vnclisten" and "vncdisplay"; if so, it throws an error. The second patch enforces this in libxl, so the user gets a friendly error message rather than an ugly libxl internal error.
George Dunlap
2012-Nov-30 16:24 UTC
[PATCH 1 of 4 v2] libxl: Combine device model arg build functions
# HG changeset patch
# User George Dunlap <george.dunlap@eu.citrix.com>
# Date 1354287957 0
# Node ID 722da032ac90c0e1a78b1154fa588bf295d1f009
# Parent ae6fb202b233af815466055d9f1a635802a50855
libxl: Combine device model arg build functions
qemu-traditional and qemu-upstream have some differences in the way
the arguments need to be passed. At the moment, this is dealt with by
having two entirely separate functions, libxl__build_device_model_args_new
and libxl__build_device_model_args_old.
However, at least 80% of these are the same; this means that fixes or
additions to one may not make it into the other. Furthermore, there
are some unaccounable differences in implementation.
This patch combines the two functions into one, choosing between old
and new implementations as necessary.
It also makes the following changes to the vnc generation code:
* Simplifies and comments it, making it easier to read and grok
* Throws an error if duplicate values of display are set, rather
than the current very un-intuitive behavior.
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -95,181 +95,6 @@ static const char *dm_keymap(const libxl
return NULL;
}
-static char ** libxl__build_device_model_args_old(libxl__gc *gc,
- const char *dm, int domid,
- const libxl_domain_config
*guest_config,
- const libxl__domain_build_state *state)
-{
- const libxl_domain_create_info *c_info = &guest_config->c_info;
- const libxl_domain_build_info *b_info = &guest_config->b_info;
- const libxl_device_nic *nics = guest_config->nics;
- const libxl_vnc_info *vnc = libxl__dm_vnc(guest_config);
- const libxl_sdl_info *sdl = dm_sdl(guest_config);
- const int num_nics = guest_config->num_nics;
- const char *keymap = dm_keymap(guest_config);
- int i;
- flexarray_t *dm_args;
- dm_args = flexarray_make(gc, 16, 1);
-
- flexarray_vappend(dm_args, dm,
- "-d", libxl__sprintf(gc, "%d",
domid), NULL);
-
- if (c_info->name)
- flexarray_vappend(dm_args, "-domain-name", c_info->name,
NULL);
-
- if (vnc) {
- char *vncarg;
- if (vnc->display) {
- if (vnc->listen && strchr(vnc->listen,
'':'') == NULL) {
- vncarg = libxl__sprintf(gc, "%s:%d",
- vnc->listen,
- vnc->display);
- } else {
- vncarg = libxl__sprintf(gc, "127.0.0.1:%d",
vnc->display);
- }
- } else if (vnc->listen) {
- if (strchr(vnc->listen, '':'') != NULL) {
- vncarg = vnc->listen;
- } else {
- vncarg = libxl__sprintf(gc, "%s:0", vnc->listen);
- }
- } else {
- vncarg = "127.0.0.1:0";
- }
- if (vnc->passwd && (vnc->passwd[0] !=
''\0''))
- vncarg = libxl__sprintf(gc, "%s,password", vncarg);
- flexarray_append(dm_args, "-vnc");
- flexarray_append(dm_args, vncarg);
-
- if (libxl_defbool_val(vnc->findunused)) {
- flexarray_append(dm_args, "-vncunused");
- }
- }
- if (sdl) {
- flexarray_append(dm_args, "-sdl");
- if (!libxl_defbool_val(sdl->opengl)) {
- flexarray_append(dm_args, "-disable-opengl");
- }
- /* XXX sdl->{display,xauthority} into $DISPLAY/$XAUTHORITY */
- }
- if (keymap) {
- flexarray_vappend(dm_args, "-k", keymap, NULL);
- }
- if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
- int ioemu_nics = 0;
- int nr_set_cpus = 0;
- char *s;
-
- if (b_info->u.hvm.serial) {
- flexarray_vappend(dm_args, "-serial",
b_info->u.hvm.serial, NULL);
- }
-
- if (libxl_defbool_val(b_info->u.hvm.nographic) && (!sdl
&& !vnc)) {
- flexarray_append(dm_args, "-nographic");
- }
-
- if (b_info->video_memkb) {
- flexarray_vappend(dm_args, "-videoram",
- libxl__sprintf(gc, "%d",
-
libxl__sizekb_to_mb(b_info->video_memkb)),
- NULL);
- }
-
- switch (b_info->u.hvm.vga.kind) {
- case LIBXL_VGA_INTERFACE_TYPE_STD:
- flexarray_append(dm_args, "-std-vga");
- break;
- case LIBXL_VGA_INTERFACE_TYPE_CIRRUS:
- break;
- }
-
- if (b_info->u.hvm.boot) {
- flexarray_vappend(dm_args, "-boot",
b_info->u.hvm.boot, NULL);
- }
- if (libxl_defbool_val(b_info->u.hvm.usb) ||
b_info->u.hvm.usbdevice) {
- flexarray_append(dm_args, "-usb");
- if (b_info->u.hvm.usbdevice) {
- flexarray_vappend(dm_args,
- "-usbdevice",
b_info->u.hvm.usbdevice, NULL);
- }
- }
- if (b_info->u.hvm.soundhw) {
- flexarray_vappend(dm_args, "-soundhw",
b_info->u.hvm.soundhw, NULL);
- }
- if (libxl_defbool_val(b_info->u.hvm.acpi)) {
- flexarray_append(dm_args, "-acpi");
- }
- if (b_info->max_vcpus > 1) {
- flexarray_vappend(dm_args, "-vcpus",
- libxl__sprintf(gc, "%d",
b_info->max_vcpus),
- NULL);
- }
-
- nr_set_cpus = libxl_bitmap_count_set(&b_info->avail_vcpus);
- s = libxl_bitmap_to_hex_string(CTX, &b_info->avail_vcpus);
- flexarray_vappend(dm_args, "-vcpu_avail",
- libxl__sprintf(gc, "%s", s), NULL);
- free(s);
-
- for (i = 0; i < num_nics; i++) {
- if (nics[i].nictype == LIBXL_NIC_TYPE_VIF_IOEMU) {
- char *smac = libxl__sprintf(gc,
- LIBXL_MAC_FMT,
LIBXL_MAC_BYTES(nics[i].mac));
- const char *ifname = libxl__device_nic_devname(gc,
- domid, nics[i].devid,
- LIBXL_NIC_TYPE_VIF_IOEMU);
- flexarray_vappend(dm_args,
- "-net",
- GCSPRINTF(
-
"nic,vlan=%d,macaddr=%s,model=%s",
- nics[i].devid, smac, nics[i].model),
- "-net",
- GCSPRINTF(
-
"tap,vlan=%d,ifname=%s,bridge=%s,"
- "script=%s,downscript=%s",
- nics[i].devid, ifname, nics[i].bridge,
- libxl_tapif_script(gc),
- libxl_tapif_script(gc)),
- NULL);
- ioemu_nics++;
- }
- }
- /* If we have no emulated nics, tell qemu not to create any */
- if ( ioemu_nics == 0 ) {
- flexarray_vappend(dm_args, "-net", "none",
NULL);
- }
- if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) {
- flexarray_append(dm_args, "-gfx_passthru");
- }
- } else {
- if (!sdl && !vnc)
- flexarray_append(dm_args, "-nographic");
- }
-
- if (state->saved_state) {
- flexarray_vappend(dm_args, "-loadvm", state->saved_state,
NULL);
- }
- for (i = 0; b_info->extra && b_info->extra[i] != NULL; i++)
- flexarray_append(dm_args, b_info->extra[i]);
- flexarray_append(dm_args, "-M");
- switch (b_info->type) {
- case LIBXL_DOMAIN_TYPE_PV:
- flexarray_append(dm_args, "xenpv");
- for (i = 0; b_info->extra_pv && b_info->extra_pv[i] !=
NULL; i++)
- flexarray_append(dm_args, b_info->extra_pv[i]);
- break;
- case LIBXL_DOMAIN_TYPE_HVM:
- flexarray_append(dm_args, "xenfv");
- for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] !=
NULL; i++)
- flexarray_append(dm_args, b_info->extra_hvm[i]);
- break;
- default:
- abort();
- }
- flexarray_append(dm_args, NULL);
- return (char **) flexarray_contents(dm_args);
-}
-
static const char *qemu_disk_format_string(libxl_disk_format format)
{
switch (format) {
@@ -318,7 +143,7 @@ static char *dm_spice_options(libxl__gc
return opt;
}
-static char ** libxl__build_device_model_args_new(libxl__gc *gc,
+static char ** libxl__build_device_model_args(libxl__gc *gc,
const char *dm, int guest_domid,
const libxl_domain_config
*guest_config,
const libxl__domain_build_state *state)
@@ -336,62 +161,106 @@ static char ** libxl__build_device_model
flexarray_t *dm_args;
int i;
uint64_t ram_size;
+ int dm_new;
+
+ switch (guest_config->b_info.device_model_version) {
+ case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+ dm_new = 0;
+ break;
+ case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+ dm_new = 1;
+ break;
+ default:
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "unknown device model
version %d",
+ guest_config->b_info.device_model_version);
+ return NULL;
+ }
+
dm_args = flexarray_make(gc, 16, 1);
- flexarray_vappend(dm_args, dm,
- "-xen-domid",
- libxl__sprintf(gc, "%d", guest_domid), NULL);
+ /* Domain ID */
+ if (dm_new) {
+ flexarray_vappend(dm_args, dm,
+ "-xen-domid",
+ libxl__sprintf(gc, "%d", guest_domid),
NULL);
+ } else {
+ flexarray_vappend(dm_args, dm,
+ "-d", libxl__sprintf(gc, "%d",
guest_domid), NULL);
+ }
- flexarray_append(dm_args, "-chardev");
- flexarray_append(dm_args,
- libxl__sprintf(gc, "socket,id=libxl-cmd,"
-
"path=%s/qmp-libxl-%d,server,nowait",
- libxl__run_dir_path(), guest_domid));
+ if (dm_new) {
+ flexarray_append(dm_args, "-chardev");
+ flexarray_append(dm_args,
+ libxl__sprintf(gc, "socket,id=libxl-cmd,"
+
"path=%s/qmp-libxl-%d,server,nowait",
+ libxl__run_dir_path(), guest_domid));
- flexarray_append(dm_args, "-mon");
- flexarray_append(dm_args, "chardev=libxl-cmd,mode=control");
+ flexarray_append(dm_args, "-mon");
+ flexarray_append(dm_args, "chardev=libxl-cmd,mode=control");
- if (b_info->type == LIBXL_DOMAIN_TYPE_PV) {
- flexarray_append(dm_args, "-xen-attach");
+ if (b_info->type == LIBXL_DOMAIN_TYPE_PV) {
+ flexarray_append(dm_args, "-xen-attach");
+ }
}
if (c_info->name) {
- flexarray_vappend(dm_args, "-name", c_info->name, NULL);
+ if ( dm_new )
+ flexarray_vappend(dm_args, "-name", c_info->name,
NULL);
+ else
+ flexarray_vappend(dm_args, "-domain-name",
c_info->name, NULL);
}
+
if (vnc) {
- int display = 0;
- const char *addr = "127.0.0.1";
char *vncarg = NULL;
flexarray_append(dm_args, "-vnc");
- if (vnc->display) {
- display = vnc->display;
- if (vnc->listen && strchr(vnc->listen,
'':'') == NULL) {
- addr = vnc->listen;
+ /*
+ * If vnc->listen is present and contains a :, and
+ * - vnc->display is 0, use vnc->listen
+ * - vnc->display is non-zero, be confused
+ * If vnc->listen is present but doesn''t, use
vnc->listen:vnc->display.
+ * If vnc->listen is not present, use 127.0.0.1:vnc->display
+ * (Remembering that vnc->display already defaults to 0.)
+ */
+ if (vnc->listen) {
+ if (strchr(vnc->listen, '':'') != NULL) {
+ if (vnc->display) {
+ LOG(ERROR, "vncdisplay set, vnclisten contains
display");
+ return NULL;
+ }
+ vncarg = vnc->listen;
+ } else {
+ vncarg = libxl__sprintf(gc, "%s:%d", vnc->listen,
+ vnc->display);
}
- } else if (vnc->listen) {
- addr = vnc->listen;
- }
+ } else
+ vncarg = libxl__sprintf(gc, "127.0.0.1:%d",
vnc->display);
- if (strchr(addr, '':'') != NULL)
- vncarg = libxl__sprintf(gc, "%s", addr);
- else
- vncarg = libxl__sprintf(gc, "%s:%d", addr, display);
if (vnc->passwd && vnc->passwd[0]) {
vncarg = libxl__sprintf(gc, "%s,password", vncarg);
}
- if (libxl_defbool_val(vnc->findunused)) {
+
+ if (dm_new && libxl_defbool_val(vnc->findunused)) {
/* This option asks to QEMU to try this number of port before to
* give up. So QEMU will try ports between $display and $display +
* 99. This option needs to be the last one of the vnc options. */
vncarg = libxl__sprintf(gc, "%s,to=99", vncarg);
}
+
flexarray_append(dm_args, vncarg);
+
+ if (!dm_new && libxl_defbool_val(vnc->findunused)) {
+ flexarray_append(dm_args, "-vncunused");
+ }
}
if (sdl) {
flexarray_append(dm_args, "-sdl");
+
+ if (!dm_new && !libxl_defbool_val(sdl->opengl)) {
+ flexarray_append(dm_args, "-disable-opengl");
+ }
/* XXX sdl->{display,xauthority} into $DISPLAY/$XAUTHORITY */
}
@@ -414,7 +283,7 @@ static char ** libxl__build_device_model
flexarray_append(dm_args, "-nographic");
}
- if (libxl_defbool_val(b_info->u.hvm.spice.enable)) {
+ if (dm_new && libxl_defbool_val(b_info->u.hvm.spice.enable))
{
const libxl_spice_info *spice = &b_info->u.hvm.spice;
char *spiceoptions = dm_spice_options(gc, spice);
if (!spiceoptions)
@@ -424,19 +293,35 @@ static char ** libxl__build_device_model
flexarray_append(dm_args, spiceoptions);
}
- switch (b_info->u.hvm.vga.kind) {
- case LIBXL_VGA_INTERFACE_TYPE_STD:
- flexarray_vappend(dm_args, "-vga", "std",
NULL);
- break;
- case LIBXL_VGA_INTERFACE_TYPE_CIRRUS:
- flexarray_vappend(dm_args, "-vga", "cirrus",
NULL);
- break;
+ if ( dm_new ) {
+ switch (b_info->u.hvm.vga.kind) {
+ case LIBXL_VGA_INTERFACE_TYPE_STD:
+ flexarray_vappend(dm_args, "-vga", "std",
NULL);
+ break;
+ case LIBXL_VGA_INTERFACE_TYPE_CIRRUS:
+ flexarray_vappend(dm_args, "-vga",
"cirrus", NULL);
+ break;
+ }
+ } else {
+ switch (b_info->u.hvm.vga.kind) {
+ case LIBXL_VGA_INTERFACE_TYPE_STD:
+ flexarray_append(dm_args, "-std-vga");
+ break;
+ case LIBXL_VGA_INTERFACE_TYPE_CIRRUS:
+ break;
+ }
}
if (b_info->u.hvm.boot) {
- flexarray_vappend(dm_args, "-boot",
- libxl__sprintf(gc, "order=%s",
b_info->u.hvm.boot), NULL);
+ if (dm_new)
+ flexarray_vappend(dm_args, "-boot",
+ libxl__sprintf(gc, "order=%s",
+ b_info->u.hvm.boot), NULL);
+ else
+ flexarray_vappend(dm_args, "-boot",
b_info->u.hvm.boot, NULL);
+
}
+
if (libxl_defbool_val(b_info->u.hvm.usb) ||
b_info->u.hvm.usbdevice) {
flexarray_append(dm_args, "-usb");
if (b_info->u.hvm.usbdevice) {
@@ -450,19 +335,39 @@ static char ** libxl__build_device_model
if (!libxl_defbool_val(b_info->u.hvm.acpi)) {
flexarray_append(dm_args, "-no-acpi");
}
- if (b_info->max_vcpus > 1) {
- flexarray_append(dm_args, "-smp");
- if (b_info->avail_vcpus.size) {
- int nr_set_cpus = 0;
- nr_set_cpus =
libxl_bitmap_count_set(&b_info->avail_vcpus);
- flexarray_append(dm_args, libxl__sprintf(gc,
"%d,maxcpus=%d",
- b_info->max_vcpus,
- nr_set_cpus));
- } else
- flexarray_append(dm_args, libxl__sprintf(gc, "%d",
-
b_info->max_vcpus));
+ if (dm_new)
+ {
+ if (b_info->max_vcpus > 1) {
+ flexarray_append(dm_args, "-smp");
+ if (b_info->avail_vcpus.size) {
+ int nr_set_cpus = 0;
+ nr_set_cpus =
libxl_bitmap_count_set(&b_info->avail_vcpus);
+
+ flexarray_append(dm_args,
+ libxl__sprintf(gc,
"%d,maxcpus=%d",
+ b_info->max_vcpus,
+ nr_set_cpus));
+ } else
+ flexarray_append(dm_args,
+ libxl__sprintf(gc, "%d",
+ b_info->max_vcpus));
+ }
+ } else {
+ int nr_set_cpus = 0;
+ char *s;
+ if (b_info->max_vcpus > 1) {
+ flexarray_vappend(dm_args, "-vcpus",
+ libxl__sprintf(gc, "%d",
b_info->max_vcpus),
+ NULL);
+ }
+ nr_set_cpus = libxl_bitmap_count_set(&b_info->avail_vcpus);
+ s = libxl_bitmap_to_hex_string(CTX, &b_info->avail_vcpus);
+ flexarray_vappend(dm_args, "-vcpu_avail",
+ libxl__sprintf(gc, "%s", s), NULL);
+ free(s);
}
+
for (i = 0; i < num_nics; i++) {
if (nics[i].nictype == LIBXL_NIC_TYPE_VIF_IOEMU) {
char *smac = libxl__sprintf(gc,
@@ -470,18 +375,35 @@ static char ** libxl__build_device_model
const char *ifname = libxl__device_nic_devname(gc,
guest_domid, nics[i].devid,
LIBXL_NIC_TYPE_VIF_IOEMU);
- flexarray_append(dm_args, "-device");
- flexarray_append(dm_args,
- libxl__sprintf(gc,
"%s,id=nic%d,netdev=net%d,mac=%s",
- nics[i].model, nics[i].devid,
- nics[i].devid, smac));
- flexarray_append(dm_args, "-netdev");
- flexarray_append(dm_args, GCSPRINTF(
-
"type=tap,id=net%d,ifname=%s,"
+ if (dm_new) {
+ flexarray_append(dm_args, "-device");
+ flexarray_append(dm_args,
+ libxl__sprintf(gc,
"%s,id=nic%d,netdev=net%d,mac=%s",
+ nics[i].model, nics[i].devid,
+ nics[i].devid, smac));
+ flexarray_append(dm_args, "-netdev");
+ flexarray_append(dm_args, GCSPRINTF(
+
"type=tap,id=net%d,ifname=%s,"
+ "script=%s,downscript=%s",
+ nics[i].devid, ifname,
+ libxl_tapif_script(gc),
+ libxl_tapif_script(gc)));
+ } else {
+ flexarray_vappend(dm_args,
+ "-net",
+ GCSPRINTF(
+
"nic,vlan=%d,macaddr=%s,model=%s",
+ nics[i].devid, smac, nics[i].model),
+ "-net",
+ GCSPRINTF(
+
"tap,vlan=%d,ifname=%s,bridge=%s,"
"script=%s,downscript=%s",
nics[i].devid, ifname,
+ nics[i].bridge,
libxl_tapif_script(gc),
- libxl_tapif_script(gc)));
+ libxl_tapif_script(gc)),
+ NULL);
+ }
ioemu_nics++;
}
}
@@ -500,10 +422,15 @@ static char ** libxl__build_device_model
}
if (state->saved_state) {
- /* This file descriptor is meant to be used by QEMU */
- int migration_fd = open(state->saved_state, O_RDONLY);
- flexarray_append(dm_args, "-incoming");
- flexarray_append(dm_args, libxl__sprintf(gc, "fd:%d",
migration_fd));
+ if (dm_new) {
+ /* This file descriptor is meant to be used by QEMU */
+ int migration_fd = open(state->saved_state, O_RDONLY);
+ flexarray_append(dm_args, "-incoming");
+ flexarray_append(dm_args,
+ libxl__sprintf(gc, "fd:%d",
migration_fd));
+ } else {
+ flexarray_vappend(dm_args, "-loadvm",
state->saved_state, NULL);
+ }
}
for (i = 0; b_info->extra && b_info->extra[i] != NULL; i++)
flexarray_append(dm_args, b_info->extra[i]);
@@ -523,6 +450,9 @@ static char ** libxl__build_device_model
abort();
}
+ if (!dm_new)
+ goto finish;
+
ram_size = libxl__sizekb_to_mb(b_info->max_memkb -
b_info->video_memkb);
flexarray_append(dm_args, "-m");
flexarray_append(dm_args, libxl__sprintf(gc, "%"PRId64,
ram_size));
@@ -585,33 +515,11 @@ static char ** libxl__build_device_model
flexarray_append(dm_args, drive);
}
}
+finish:
flexarray_append(dm_args, NULL);
return (char **) flexarray_contents(dm_args);
}
-static char ** libxl__build_device_model_args(libxl__gc *gc,
- const char *dm, int guest_domid,
- const libxl_domain_config
*guest_config,
- const libxl__domain_build_state *state)
-{
- libxl_ctx *ctx = libxl__gc_owner(gc);
-
- switch (guest_config->b_info.device_model_version) {
- case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
- return libxl__build_device_model_args_old(gc, dm,
- guest_domid, guest_config,
- state);
- case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
- return libxl__build_device_model_args_new(gc, dm,
- guest_domid, guest_config,
- state);
- default:
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "unknown device model
version %d",
- guest_config->b_info.device_model_version);
- return NULL;
- }
-}
-
static void libxl__dm_vifs_from_hvm_guest_config(libxl__gc *gc,
libxl_domain_config * const guest_config,
libxl_domain_config *dm_config)
George Dunlap
2012-Nov-30 16:24 UTC
[PATCH 2 of 4 v2] xl: Check for duplicate vncdisplay options, and return an error
# HG changeset patch
# User George Dunlap <george.dunlap@eu.citrix.com>
# Date 1354291984 0
# Node ID fa37bd276212a01e3c898b54a7f2385454c406a7
# Parent 722da032ac90c0e1a78b1154fa588bf295d1f009
xl: Check for duplicate vncdisplay options, and return an error
If the user has set a vnc display number both in vnclisten (with
"xxxx:yy"), and with vncdisplay, throw an error.
Update man pages to match.
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -350,11 +350,17 @@ other VNC-related settings. The default
Specifies the IP address, and optionally VNC display number, to use.
+NB that if you specify the display number here, you should not use
+vncdisplay.
+
=item C<vncdisplay=DISPLAYNUM>
Specifies the VNC display number to use. The actual TCP port number
will be DISPLAYNUM+5900.
+NB that you should not use this option if you set the displaynum in the
+vnclisten string.
+
=item C<vncunused=BOOLEAN>
Requests that the VNC display setup search for a free TCP port to use.
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1257,6 +1257,7 @@ skip_nic:
vfb->sdl.xauthority = strdup(p2 + 1);
}
} while ((p = strtok(NULL, ",")) != NULL);
+
skip_vfb:
free(buf2);
d_config->num_vfbs++;
@@ -1490,6 +1491,16 @@ skip_vfb:
xlu_cfg_replace_string (config, "soundhw",
&b_info->u.hvm.soundhw, 0);
xlu_cfg_get_defbool(config, "xen_platform_pci",
&b_info->u.hvm.xen_platform_pci, 0);
+
+ if(b_info->u.hvm.vnc.listen
+ && b_info->u.hvm.vnc.display
+ && strchr(b_info->u.hvm.vnc.listen,
'':'') != NULL) {
+ fprintf(stderr,
+ "ERROR: Display specified both in vnclisten"
+ " and vncdisplay!\n");
+ exit (1);
+
+ }
}
xlu_cfg_destroy(config);
George Dunlap
2012-Nov-30 16:24 UTC
[PATCH 3 of 4 v2] libxl: Allow multiple USB devices on HVM domain creation
# HG changeset patch
# User George Dunlap <george.dunlap@eu.citrix.com>
# Date 1354291988 0
# Node ID af47b01d2dbe0e916e1014a264845c48d6ef8108
# Parent fa37bd276212a01e3c898b54a7f2385454c406a7
libxl: Allow multiple USB devices on HVM domain creation
This patch allows an HVM domain to be created with multiple USB
devices.
Since the previous interface only allowed the passing of a single
device, this requires us to add a new element to the hvm struct of
libxl_domain_build_info -- usbdevice_list. For API compatibility, the
old element, usbdevice, remains.
If hvm.usbdevice_list is set, each device listed will cause an extra
"-usbdevice [foo]" to be appended to the qemu command line.
Callers may set either hvm.usbdevice or hvm.usbdevice_list, but not
both; libxl will throw an error if both are set.
In order to allow users of libxl to write software compatible with
older versions of libxl, also define LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST.
If this is defined, callers may use either hvm.usbdevice or
hvm.usbdevice_list; otherwise, only hvm.usbdevice will be available.
v2:
- Throw an error if both usbdevice and usbdevice_list are set
- Update and clarify definition based on feedback
- Previous patches means this works for both traditional and upstream
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -266,6 +266,22 @@
#endif
#endif
+/*
+ * LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST
+ *
+ * If this is defined, then the libxl_domain_build_info structure will
+ * contain hvm.usbdevice_list, a libxl_string_list type that contains
+ * a list of USB devices to specify on the qemu command-line.
+ *
+ * If it is set, callers may use either hvm.usbdevice or
+ * hvm.usbdevice_list, but not both; if both are set, libxl will
+ * throw an error.
+ *
+ * If this is not defined, callers can only use hvm.usbdevice. Note
+ * that this means only one device can be added at domain build time.
+ */
+#define LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST 1
+
/* Functions annotated with LIBXL_EXTERNAL_CALLERS_ONLY may not be
* called from within libxl itself. Callers outside libxl, who
* do not #include libxl_internal.h, are fine. */
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -322,11 +322,28 @@ static char ** libxl__build_device_model
}
- if (libxl_defbool_val(b_info->u.hvm.usb) ||
b_info->u.hvm.usbdevice) {
+ if (libxl_defbool_val(b_info->u.hvm.usb)
+ || b_info->u.hvm.usbdevice
+ || b_info->u.hvm.usbdevice_list) {
+ if ( b_info->u.hvm.usbdevice &&
b_info->u.hvm.usbdevice_list )
+ {
+ LOG(ERROR, "%s: Both usbdevice and usbdevice_list
set",
+ __func__);
+ return NULL;
+ }
flexarray_append(dm_args, "-usb");
if (b_info->u.hvm.usbdevice) {
flexarray_vappend(dm_args,
"-usbdevice",
b_info->u.hvm.usbdevice, NULL);
+ } else if (b_info->u.hvm.usbdevice_list) {
+ char **p;
+ for (p = b_info->u.hvm.usbdevice_list;
+ *p;
+ p++) {
+ flexarray_vappend(dm_args,
+ "-usbdevice",
+ *p, NULL);
+ }
}
}
if (b_info->u.hvm.soundhw) {
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -326,6 +326,7 @@ libxl_domain_build_info = Struct("domain
("usbdevice", string),
("soundhw", string),
("xen_platform_pci",
libxl_defbool),
+ ("usbdevice_list",
libxl_string_list),
])),
("pv", Struct(None, [("kernel", string),
("slack_memkb", MemKB),
George Dunlap
2012-Nov-30 16:24 UTC
[PATCH 4 of 4 v2] xl: Accept a list for usbdevice in config file
# HG changeset patch
# User George Dunlap <george.dunlap@eu.citrix.com>
# Date 1354291988 0
# Node ID 043d69be04c91eaead159e8e74999c59eb68f560
# Parent af47b01d2dbe0e916e1014a264845c48d6ef8108
xl: Accept a list for usbdevice in config file
Allow the "usbdevice" key to accept a list of USB devices, and pass
them in using the new usbdevice_list domain build element.
For backwards compatibility, still accept singleton values.
Also update the xl.cfg manpage, adding information about how to pass
through host devices.
v2:
- Add some verbiage to make it clear that "usb" is for emulated
devices
- Reference qemu manual for more usbdevice options
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1107,17 +1107,27 @@ device.
=item B<usb=BOOLEAN>
-Enables or disables a USB bus in the guest.
+Enables or disables an emulated USB bus in the guest.
-=item B<usbdevice=DEVICE>
+=item B<usbdevice=[ "DEVICE", "DEVICE", ...]>
-Adds B<DEVICE> to the USB bus. The USB bus must also be enabled using
-B<usb=1>. The most common use for this option is
B<usbdevice=tablet>
-which adds pointer device using absolute coordinates. Such devices
-function better than relative coordinate devices (such as a standard
-mouse) since many methods of exporting guest graphics (such as VNC)
-work better in this mode. Note that this is independent of the actual
-pointer device you are using on the host/client side.
+Adds B<DEVICE>s to the emulated USB bus. The USB bus must also be
+enabled using B<usb=1>. The most common use for this option is
+B<usbdevice=[''tablet'']> which adds pointer device using
absolute
+coordinates. Such devices function better than relative coordinate
+devices (such as a standard mouse) since many methods of exporting
+guest graphics (such as VNC) work better in this mode. Note that this
+is independent of the actual pointer device you are using on the
+host/client side.
+
+Host devices can also be passed through in this way, by specifying
+host:USBID, where USBID is of the form xxxx:yyyy. The USBID can
+typically be found by using lsusb or usb-devices.
+
+The form usbdevice=DEVICE is also accepted for backwards compatibility.
+
+More valid options can be found in the "usbdevice" section of the
qemu
+documentation.
=back
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1486,8 +1486,23 @@ skip_vfb:
xlu_cfg_replace_string (config, "serial",
&b_info->u.hvm.serial, 0);
xlu_cfg_replace_string (config, "boot",
&b_info->u.hvm.boot, 0);
xlu_cfg_get_defbool(config, "usb", &b_info->u.hvm.usb,
0);
- xlu_cfg_replace_string (config, "usbdevice",
- &b_info->u.hvm.usbdevice, 0);
+ switch (xlu_cfg_get_list_as_string_list(config, "usbdevice",
+
&b_info->u.hvm.usbdevice_list,
+ 1))
+ {
+
+ case 0: break; /* Success */
+ case ESRCH: break; /* Option not present */
+ case EINVAL:
+ /* If it''s not a valid list, try reading it as an atom,
falling through to
+ * an error if it fails */
+ if (!xlu_cfg_replace_string(config, "usbdevice",
&b_info->u.hvm.usbdevice, 0))
+ break;
+ /* FALLTHRU */
+ default:
+ fprintf(stderr,"xl: Unable to parse usbdevice.\n");
+ exit(-ERROR_FAIL);
+ }
xlu_cfg_replace_string (config, "soundhw",
&b_info->u.hvm.soundhw, 0);
xlu_cfg_get_defbool(config, "xen_platform_pci",
&b_info->u.hvm.xen_platform_pci, 0);
George Dunlap
2012-Nov-30 16:53 UTC
Re: [PATCH 1 of 4 v2] libxl: Combine device model arg build functions
This patch also provides an easy way to trigger an interesting "failure to handle failure" bug in libxl. To reproduce: * Apply *only* this patch, and not patch 2 in this series (which filters it out at the xl level) * Create a config file where vnclisten has a display number, and vncdisplay is non-zero; e.g.: vnclisten="0.0.0.0:1" vncdisplay=1 (vncdisplay=0 is indistinguishable, to xl, from not having it set at all.) The result is a set of error messages like this: libxl: error: libxl_dm.c:231:libxl__build_device_model_args: vncdisplay set, but vnclisten also contains adisplay number Too confused to continue. libxl: error: libxl_dm.c:1113:device_model_spawn_outcome: (null): spawn failed (rc=-3) libxl: error: libxl_exec.c:118:libxl_report_child_exitstatus: /etc/xen/scripts/vif-bridge add [21168] exited with error status 1 libxl: error: libxl_create.c:1117:domcreate_attach_vtpms: unable to add nic devices libxl: error: libxl_dm.c:1140:libxl__destroy_device_model: could not find device-model''s pid for dom 25 libxl: error: libxl.c:1414:libxl__destroy_domid: libxl__destroy_device_model failed for 25 Obviously the failure in libxl__build_device_model_args() failed to abort the domain-building process, and it continued to try to run vif and vtpm setups. -George On Fri, Nov 30, 2012 at 4:24 PM, George Dunlap <george.dunlap@eu.citrix.com>wrote:> # HG changeset patch > # User George Dunlap <george.dunlap@eu.citrix.com> > # Date 1354287957 0 > # Node ID 722da032ac90c0e1a78b1154fa588bf295d1f009 > # Parent ae6fb202b233af815466055d9f1a635802a50855 > libxl: Combine device model arg build functions > > qemu-traditional and qemu-upstream have some differences in the way > the arguments need to be passed. At the moment, this is dealt with by > having two entirely separate functions, libxl__build_device_model_args_new > and libxl__build_device_model_args_old. > > However, at least 80% of these are the same; this means that fixes or > additions to one may not make it into the other. Furthermore, there > are some unaccounable differences in implementation. > > This patch combines the two functions into one, choosing between old > and new implementations as necessary. > > It also makes the following changes to the vnc generation code: > * Simplifies and comments it, making it easier to read and grok > * Throws an error if duplicate values of display are set, rather > than the current very un-intuitive behavior. > > Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com> > > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > --- a/tools/libxl/libxl_dm.c > +++ b/tools/libxl/libxl_dm.c > @@ -95,181 +95,6 @@ static const char *dm_keymap(const libxl > return NULL; > } > > -static char ** libxl__build_device_model_args_old(libxl__gc *gc, > - const char *dm, int domid, > - const libxl_domain_config > *guest_config, > - const libxl__domain_build_state > *state) > -{ > - const libxl_domain_create_info *c_info = &guest_config->c_info; > - const libxl_domain_build_info *b_info = &guest_config->b_info; > - const libxl_device_nic *nics = guest_config->nics; > - const libxl_vnc_info *vnc = libxl__dm_vnc(guest_config); > - const libxl_sdl_info *sdl = dm_sdl(guest_config); > - const int num_nics = guest_config->num_nics; > - const char *keymap = dm_keymap(guest_config); > - int i; > - flexarray_t *dm_args; > - dm_args = flexarray_make(gc, 16, 1); > - > - flexarray_vappend(dm_args, dm, > - "-d", libxl__sprintf(gc, "%d", domid), NULL); > - > - if (c_info->name) > - flexarray_vappend(dm_args, "-domain-name", c_info->name, NULL); > - > - if (vnc) { > - char *vncarg; > - if (vnc->display) { > - if (vnc->listen && strchr(vnc->listen, '':'') == NULL) { > - vncarg = libxl__sprintf(gc, "%s:%d", > - vnc->listen, > - vnc->display); > - } else { > - vncarg = libxl__sprintf(gc, "127.0.0.1:%d", > vnc->display); > - } > - } else if (vnc->listen) { > - if (strchr(vnc->listen, '':'') != NULL) { > - vncarg = vnc->listen; > - } else { > - vncarg = libxl__sprintf(gc, "%s:0", vnc->listen); > - } > - } else { > - vncarg = "127.0.0.1:0"; > - } > - if (vnc->passwd && (vnc->passwd[0] != ''\0'')) > - vncarg = libxl__sprintf(gc, "%s,password", vncarg); > - flexarray_append(dm_args, "-vnc"); > - flexarray_append(dm_args, vncarg); > - > - if (libxl_defbool_val(vnc->findunused)) { > - flexarray_append(dm_args, "-vncunused"); > - } > - } > - if (sdl) { > - flexarray_append(dm_args, "-sdl"); > - if (!libxl_defbool_val(sdl->opengl)) { > - flexarray_append(dm_args, "-disable-opengl"); > - } > - /* XXX sdl->{display,xauthority} into $DISPLAY/$XAUTHORITY */ > - } > - if (keymap) { > - flexarray_vappend(dm_args, "-k", keymap, NULL); > - } > - if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) { > - int ioemu_nics = 0; > - int nr_set_cpus = 0; > - char *s; > - > - if (b_info->u.hvm.serial) { > - flexarray_vappend(dm_args, "-serial", b_info->u.hvm.serial, > NULL); > - } > - > - if (libxl_defbool_val(b_info->u.hvm.nographic) && (!sdl && !vnc)) > { > - flexarray_append(dm_args, "-nographic"); > - } > - > - if (b_info->video_memkb) { > - flexarray_vappend(dm_args, "-videoram", > - libxl__sprintf(gc, "%d", > - > libxl__sizekb_to_mb(b_info->video_memkb)), > - NULL); > - } > - > - switch (b_info->u.hvm.vga.kind) { > - case LIBXL_VGA_INTERFACE_TYPE_STD: > - flexarray_append(dm_args, "-std-vga"); > - break; > - case LIBXL_VGA_INTERFACE_TYPE_CIRRUS: > - break; > - } > - > - if (b_info->u.hvm.boot) { > - flexarray_vappend(dm_args, "-boot", b_info->u.hvm.boot, NULL); > - } > - if (libxl_defbool_val(b_info->u.hvm.usb) || > b_info->u.hvm.usbdevice) { > - flexarray_append(dm_args, "-usb"); > - if (b_info->u.hvm.usbdevice) { > - flexarray_vappend(dm_args, > - "-usbdevice", b_info->u.hvm.usbdevice, > NULL); > - } > - } > - if (b_info->u.hvm.soundhw) { > - flexarray_vappend(dm_args, "-soundhw", b_info->u.hvm.soundhw, > NULL); > - } > - if (libxl_defbool_val(b_info->u.hvm.acpi)) { > - flexarray_append(dm_args, "-acpi"); > - } > - if (b_info->max_vcpus > 1) { > - flexarray_vappend(dm_args, "-vcpus", > - libxl__sprintf(gc, "%d", b_info->max_vcpus), > - NULL); > - } > - > - nr_set_cpus = libxl_bitmap_count_set(&b_info->avail_vcpus); > - s = libxl_bitmap_to_hex_string(CTX, &b_info->avail_vcpus); > - flexarray_vappend(dm_args, "-vcpu_avail", > - libxl__sprintf(gc, "%s", s), NULL); > - free(s); > - > - for (i = 0; i < num_nics; i++) { > - if (nics[i].nictype == LIBXL_NIC_TYPE_VIF_IOEMU) { > - char *smac = libxl__sprintf(gc, > - LIBXL_MAC_FMT, > LIBXL_MAC_BYTES(nics[i].mac)); > - const char *ifname = libxl__device_nic_devname(gc, > - domid, nics[i].devid, > - LIBXL_NIC_TYPE_VIF_IOEMU); > - flexarray_vappend(dm_args, > - "-net", > - GCSPRINTF( > - "nic,vlan=%d,macaddr=%s,model=%s", > - nics[i].devid, smac, nics[i].model), > - "-net", > - GCSPRINTF( > - "tap,vlan=%d,ifname=%s,bridge=%s," > - "script=%s,downscript=%s", > - nics[i].devid, ifname, > nics[i].bridge, > - libxl_tapif_script(gc), > - libxl_tapif_script(gc)), > - NULL); > - ioemu_nics++; > - } > - } > - /* If we have no emulated nics, tell qemu not to create any */ > - if ( ioemu_nics == 0 ) { > - flexarray_vappend(dm_args, "-net", "none", NULL); > - } > - if (libxl_defbool_val(b_info->u.hvm.gfx_passthru)) { > - flexarray_append(dm_args, "-gfx_passthru"); > - } > - } else { > - if (!sdl && !vnc) > - flexarray_append(dm_args, "-nographic"); > - } > - > - if (state->saved_state) { > - flexarray_vappend(dm_args, "-loadvm", state->saved_state, NULL); > - } > - for (i = 0; b_info->extra && b_info->extra[i] != NULL; i++) > - flexarray_append(dm_args, b_info->extra[i]); > - flexarray_append(dm_args, "-M"); > - switch (b_info->type) { > - case LIBXL_DOMAIN_TYPE_PV: > - flexarray_append(dm_args, "xenpv"); > - for (i = 0; b_info->extra_pv && b_info->extra_pv[i] != NULL; i++) > - flexarray_append(dm_args, b_info->extra_pv[i]); > - break; > - case LIBXL_DOMAIN_TYPE_HVM: > - flexarray_append(dm_args, "xenfv"); > - for (i = 0; b_info->extra_hvm && b_info->extra_hvm[i] != NULL; > i++) > - flexarray_append(dm_args, b_info->extra_hvm[i]); > - break; > - default: > - abort(); > - } > - flexarray_append(dm_args, NULL); > - return (char **) flexarray_contents(dm_args); > -} > - > static const char *qemu_disk_format_string(libxl_disk_format format) > { > switch (format) { > @@ -318,7 +143,7 @@ static char *dm_spice_options(libxl__gc > return opt; > } > > -static char ** libxl__build_device_model_args_new(libxl__gc *gc, > +static char ** libxl__build_device_model_args(libxl__gc *gc, > const char *dm, int guest_domid, > const libxl_domain_config > *guest_config, > const libxl__domain_build_state > *state) > @@ -336,62 +161,106 @@ static char ** libxl__build_device_model > flexarray_t *dm_args; > int i; > uint64_t ram_size; > + int dm_new; > + > + switch (guest_config->b_info.device_model_version) { > + case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL: > + dm_new = 0; > + break; > + case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN: > + dm_new = 1; > + break; > + default: > + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "unknown device model > version %d", > + guest_config->b_info.device_model_version); > + return NULL; > + } > + > > dm_args = flexarray_make(gc, 16, 1); > > - flexarray_vappend(dm_args, dm, > - "-xen-domid", > - libxl__sprintf(gc, "%d", guest_domid), NULL); > + /* Domain ID */ > + if (dm_new) { > + flexarray_vappend(dm_args, dm, > + "-xen-domid", > + libxl__sprintf(gc, "%d", guest_domid), NULL); > + } else { > + flexarray_vappend(dm_args, dm, > + "-d", libxl__sprintf(gc, "%d", guest_domid), > NULL); > + } > > - flexarray_append(dm_args, "-chardev"); > - flexarray_append(dm_args, > - libxl__sprintf(gc, "socket,id=libxl-cmd," > - "path=%s/qmp-libxl-%d,server,nowait", > - libxl__run_dir_path(), guest_domid)); > + if (dm_new) { > + flexarray_append(dm_args, "-chardev"); > + flexarray_append(dm_args, > + libxl__sprintf(gc, "socket,id=libxl-cmd," > + > "path=%s/qmp-libxl-%d,server,nowait", > + libxl__run_dir_path(), > guest_domid)); > > - flexarray_append(dm_args, "-mon"); > - flexarray_append(dm_args, "chardev=libxl-cmd,mode=control"); > + flexarray_append(dm_args, "-mon"); > + flexarray_append(dm_args, "chardev=libxl-cmd,mode=control"); > > - if (b_info->type == LIBXL_DOMAIN_TYPE_PV) { > - flexarray_append(dm_args, "-xen-attach"); > + if (b_info->type == LIBXL_DOMAIN_TYPE_PV) { > + flexarray_append(dm_args, "-xen-attach"); > + } > } > > if (c_info->name) { > - flexarray_vappend(dm_args, "-name", c_info->name, NULL); > + if ( dm_new ) > + flexarray_vappend(dm_args, "-name", c_info->name, NULL); > + else > + flexarray_vappend(dm_args, "-domain-name", c_info->name, > NULL); > } > + > if (vnc) { > - int display = 0; > - const char *addr = "127.0.0.1"; > char *vncarg = NULL; > > flexarray_append(dm_args, "-vnc"); > > - if (vnc->display) { > - display = vnc->display; > - if (vnc->listen && strchr(vnc->listen, '':'') == NULL) { > - addr = vnc->listen; > + /* > + * If vnc->listen is present and contains a :, and > + * - vnc->display is 0, use vnc->listen > + * - vnc->display is non-zero, be confused > + * If vnc->listen is present but doesn''t, use > vnc->listen:vnc->display. > + * If vnc->listen is not present, use 127.0.0.1:vnc->display > + * (Remembering that vnc->display already defaults to 0.) > + */ > + if (vnc->listen) { > + if (strchr(vnc->listen, '':'') != NULL) { > + if (vnc->display) { > + LOG(ERROR, "vncdisplay set, vnclisten contains > display"); > + return NULL; > + } > + vncarg = vnc->listen; > + } else { > + vncarg = libxl__sprintf(gc, "%s:%d", vnc->listen, > + vnc->display); > } > - } else if (vnc->listen) { > - addr = vnc->listen; > - } > + } else > + vncarg = libxl__sprintf(gc, "127.0.0.1:%d", vnc->display); > > - if (strchr(addr, '':'') != NULL) > - vncarg = libxl__sprintf(gc, "%s", addr); > - else > - vncarg = libxl__sprintf(gc, "%s:%d", addr, display); > if (vnc->passwd && vnc->passwd[0]) { > vncarg = libxl__sprintf(gc, "%s,password", vncarg); > } > - if (libxl_defbool_val(vnc->findunused)) { > + > + if (dm_new && libxl_defbool_val(vnc->findunused)) { > /* This option asks to QEMU to try this number of port before > to > * give up. So QEMU will try ports between $display and > $display + > * 99. This option needs to be the last one of the vnc > options. */ > vncarg = libxl__sprintf(gc, "%s,to=99", vncarg); > } > + > flexarray_append(dm_args, vncarg); > + > + if (!dm_new && libxl_defbool_val(vnc->findunused)) { > + flexarray_append(dm_args, "-vncunused"); > + } > } > if (sdl) { > flexarray_append(dm_args, "-sdl"); > + > + if (!dm_new && !libxl_defbool_val(sdl->opengl)) { > + flexarray_append(dm_args, "-disable-opengl"); > + } > /* XXX sdl->{display,xauthority} into $DISPLAY/$XAUTHORITY */ > } > > @@ -414,7 +283,7 @@ static char ** libxl__build_device_model > flexarray_append(dm_args, "-nographic"); > } > > - if (libxl_defbool_val(b_info->u.hvm.spice.enable)) { > + if (dm_new && libxl_defbool_val(b_info->u.hvm.spice.enable)) { > const libxl_spice_info *spice = &b_info->u.hvm.spice; > char *spiceoptions = dm_spice_options(gc, spice); > if (!spiceoptions) > @@ -424,19 +293,35 @@ static char ** libxl__build_device_model > flexarray_append(dm_args, spiceoptions); > } > > - switch (b_info->u.hvm.vga.kind) { > - case LIBXL_VGA_INTERFACE_TYPE_STD: > - flexarray_vappend(dm_args, "-vga", "std", NULL); > - break; > - case LIBXL_VGA_INTERFACE_TYPE_CIRRUS: > - flexarray_vappend(dm_args, "-vga", "cirrus", NULL); > - break; > + if ( dm_new ) { > + switch (b_info->u.hvm.vga.kind) { > + case LIBXL_VGA_INTERFACE_TYPE_STD: > + flexarray_vappend(dm_args, "-vga", "std", NULL); > + break; > + case LIBXL_VGA_INTERFACE_TYPE_CIRRUS: > + flexarray_vappend(dm_args, "-vga", "cirrus", NULL); > + break; > + } > + } else { > + switch (b_info->u.hvm.vga.kind) { > + case LIBXL_VGA_INTERFACE_TYPE_STD: > + flexarray_append(dm_args, "-std-vga"); > + break; > + case LIBXL_VGA_INTERFACE_TYPE_CIRRUS: > + break; > + } > } > > if (b_info->u.hvm.boot) { > - flexarray_vappend(dm_args, "-boot", > - libxl__sprintf(gc, "order=%s", b_info->u.hvm.boot), > NULL); > + if (dm_new) > + flexarray_vappend(dm_args, "-boot", > + libxl__sprintf(gc, "order=%s", > + b_info->u.hvm.boot), > NULL); > + else > + flexarray_vappend(dm_args, "-boot", b_info->u.hvm.boot, > NULL); > + > } > + > if (libxl_defbool_val(b_info->u.hvm.usb) || > b_info->u.hvm.usbdevice) { > flexarray_append(dm_args, "-usb"); > if (b_info->u.hvm.usbdevice) { > @@ -450,19 +335,39 @@ static char ** libxl__build_device_model > if (!libxl_defbool_val(b_info->u.hvm.acpi)) { > flexarray_append(dm_args, "-no-acpi"); > } > - if (b_info->max_vcpus > 1) { > - flexarray_append(dm_args, "-smp"); > - if (b_info->avail_vcpus.size) { > - int nr_set_cpus = 0; > - nr_set_cpus > libxl_bitmap_count_set(&b_info->avail_vcpus); > > - flexarray_append(dm_args, libxl__sprintf(gc, > "%d,maxcpus=%d", > - > b_info->max_vcpus, > - nr_set_cpus)); > - } else > - flexarray_append(dm_args, libxl__sprintf(gc, "%d", > - > b_info->max_vcpus)); > + if (dm_new) > + { > + if (b_info->max_vcpus > 1) { > + flexarray_append(dm_args, "-smp"); > + if (b_info->avail_vcpus.size) { > + int nr_set_cpus = 0; > + nr_set_cpus > libxl_bitmap_count_set(&b_info->avail_vcpus); > + > + flexarray_append(dm_args, > + libxl__sprintf(gc, "%d,maxcpus=%d", > + b_info->max_vcpus, > + nr_set_cpus)); > + } else > + flexarray_append(dm_args, > + libxl__sprintf(gc, "%d", > + b_info->max_vcpus)); > + } > + } else { > + int nr_set_cpus = 0; > + char *s; > + if (b_info->max_vcpus > 1) { > + flexarray_vappend(dm_args, "-vcpus", > + libxl__sprintf(gc, "%d", > b_info->max_vcpus), > + NULL); > + } > + nr_set_cpus = libxl_bitmap_count_set(&b_info->avail_vcpus); > + s = libxl_bitmap_to_hex_string(CTX, &b_info->avail_vcpus); > + flexarray_vappend(dm_args, "-vcpu_avail", > + libxl__sprintf(gc, "%s", s), NULL); > + free(s); > } > + > for (i = 0; i < num_nics; i++) { > if (nics[i].nictype == LIBXL_NIC_TYPE_VIF_IOEMU) { > char *smac = libxl__sprintf(gc, > @@ -470,18 +375,35 @@ static char ** libxl__build_device_model > const char *ifname = libxl__device_nic_devname(gc, > guest_domid, > nics[i].devid, > LIBXL_NIC_TYPE_VIF_IOEMU); > - flexarray_append(dm_args, "-device"); > - flexarray_append(dm_args, > - libxl__sprintf(gc, "%s,id=nic%d,netdev=net%d,mac=%s", > - nics[i].model, > nics[i].devid, > - nics[i].devid, smac)); > - flexarray_append(dm_args, "-netdev"); > - flexarray_append(dm_args, GCSPRINTF( > - "type=tap,id=net%d,ifname=%s," > + if (dm_new) { > + flexarray_append(dm_args, "-device"); > + flexarray_append(dm_args, > + libxl__sprintf(gc, > "%s,id=nic%d,netdev=net%d,mac=%s", > + nics[i].model, nics[i].devid, > + nics[i].devid, smac)); > + flexarray_append(dm_args, "-netdev"); > + flexarray_append(dm_args, GCSPRINTF( > + "type=tap,id=net%d,ifname=%s," > + "script=%s,downscript=%s", > + nics[i].devid, ifname, > + libxl_tapif_script(gc), > + libxl_tapif_script(gc))); > + } else { > + flexarray_vappend(dm_args, > + "-net", > + GCSPRINTF( > + > "nic,vlan=%d,macaddr=%s,model=%s", > + nics[i].devid, smac, > nics[i].model), > + "-net", > + GCSPRINTF( > + > "tap,vlan=%d,ifname=%s,bridge=%s," > "script=%s,downscript=%s", > nics[i].devid, ifname, > + nics[i].bridge, > libxl_tapif_script(gc), > - libxl_tapif_script(gc))); > + libxl_tapif_script(gc)), > + NULL); > + } > ioemu_nics++; > } > } > @@ -500,10 +422,15 @@ static char ** libxl__build_device_model > } > > if (state->saved_state) { > - /* This file descriptor is meant to be used by QEMU */ > - int migration_fd = open(state->saved_state, O_RDONLY); > - flexarray_append(dm_args, "-incoming"); > - flexarray_append(dm_args, libxl__sprintf(gc, "fd:%d", > migration_fd)); > + if (dm_new) { > + /* This file descriptor is meant to be used by QEMU */ > + int migration_fd = open(state->saved_state, O_RDONLY); > + flexarray_append(dm_args, "-incoming"); > + flexarray_append(dm_args, > + libxl__sprintf(gc, "fd:%d", migration_fd)); > + } else { > + flexarray_vappend(dm_args, "-loadvm", state->saved_state, > NULL); > + } > } > for (i = 0; b_info->extra && b_info->extra[i] != NULL; i++) > flexarray_append(dm_args, b_info->extra[i]); > @@ -523,6 +450,9 @@ static char ** libxl__build_device_model > abort(); > } > > + if (!dm_new) > + goto finish; > + > ram_size = libxl__sizekb_to_mb(b_info->max_memkb - > b_info->video_memkb); > flexarray_append(dm_args, "-m"); > flexarray_append(dm_args, libxl__sprintf(gc, "%"PRId64, ram_size)); > @@ -585,33 +515,11 @@ static char ** libxl__build_device_model > flexarray_append(dm_args, drive); > } > } > +finish: > flexarray_append(dm_args, NULL); > return (char **) flexarray_contents(dm_args); > } > > -static char ** libxl__build_device_model_args(libxl__gc *gc, > - const char *dm, int guest_domid, > - const libxl_domain_config > *guest_config, > - const libxl__domain_build_state > *state) > -{ > - libxl_ctx *ctx = libxl__gc_owner(gc); > - > - switch (guest_config->b_info.device_model_version) { > - case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL: > - return libxl__build_device_model_args_old(gc, dm, > - guest_domid, > guest_config, > - state); > - case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN: > - return libxl__build_device_model_args_new(gc, dm, > - guest_domid, > guest_config, > - state); > - default: > - LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "unknown device model > version %d", > - guest_config->b_info.device_model_version); > - return NULL; > - } > -} > - > static void libxl__dm_vifs_from_hvm_guest_config(libxl__gc *gc, > libxl_domain_config * const > guest_config, > libxl_domain_config *dm_config) > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Ian Campbell
2012-Dec-04 15:21 UTC
Re: [PATCH 1 of 4 v2] libxl: Combine device model arg build functions
On Fri, 2012-11-30 at 16:24 +0000, George Dunlap wrote:> # HG changeset patch > # User George Dunlap <george.dunlap@eu.citrix.com> > # Date 1354287957 0 > # Node ID 722da032ac90c0e1a78b1154fa588bf295d1f009 > # Parent ae6fb202b233af815466055d9f1a635802a50855 > libxl: Combine device model arg build functions > > qemu-traditional and qemu-upstream have some differences in the way > the arguments need to be passed. At the moment, this is dealt with by > having two entirely separate functions, libxl__build_device_model_args_new > and libxl__build_device_model_args_old. > > However, at least 80% of these are the same; this means that fixes or > additions to one may not make it into the other. Furthermore, there > are some unaccounable differences in implementation.FWIW: 1 file changed, 168 insertions(+), 260 deletions(-) But that doesn''t really show how much of the code in the new function is shared and how much is per-qemu-version. Casting my eye over the code with the patch applied (i.e. an unscientific estimate) it seems like most of it is under an if (dm_new) of some sort. My main concern is that qemu-xen-traditional is frozen and so this code shouldn''t be changing, but by bundling it with the qemu-xen code it may be subject to churn as new stuff gets added to the new qemu and plumbed through. A related concern is that some of the interfaces we use (e.g. "-hda") are deprecated in favour of more expressive forms (-drive -device et al), depending on how things go upstream we may want to switch to using them. Perhaps a useful compromise would be to pull the common stuff into a common function but call out to version specific function for the bits which differ too. Perhaps factoring into functional areas might help too? e.g. make_{disk,net,foo}_args, then the decision to combine or not can be made on a per functional area basis?> @@ -523,6 +450,9 @@ static char ** libxl__build_device_model > abort(); > } > > + if (!dm_new) > + goto finish; > + > ram_size = libxl__sizekb_to_mb(b_info->max_memkb - b_info->video_memkb); > flexarray_append(dm_args, "-m"); > flexarray_append(dm_args, libxl__sprintf(gc, "%"PRId64, ram_size)); > @@ -585,33 +515,11 @@ static char ** libxl__build_device_model > flexarray_append(dm_args, drive); > } > } > +finish:This feels especially unsatisfying...
Ian Campbell
2012-Dec-04 15:30 UTC
Re: [PATCH 3 of 4 v2] libxl: Allow multiple USB devices on HVM domain creation
On Fri, 2012-11-30 at 16:24 +0000, George Dunlap wrote:> # HG changeset patch > # User George Dunlap <george.dunlap@eu.citrix.com> > # Date 1354291988 0 > # Node ID af47b01d2dbe0e916e1014a264845c48d6ef8108 > # Parent fa37bd276212a01e3c898b54a7f2385454c406a7 > libxl: Allow multiple USB devices on HVM domain creation > > This patch allows an HVM domain to be created with multiple USB > devices. > > Since the previous interface only allowed the passing of a single > device, this requires us to add a new element to the hvm struct of > libxl_domain_build_info -- usbdevice_list. For API compatibility, the > old element, usbdevice, remains. > > If hvm.usbdevice_list is set, each device listed will cause an extra > "-usbdevice [foo]" to be appended to the qemu command line. > > Callers may set either hvm.usbdevice or hvm.usbdevice_list, but not > both; libxl will throw an error if both are set. > > In order to allow users of libxl to write software compatible with > older versions of libxl, also define LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST. > If this is defined, callers may use either hvm.usbdevice or > hvm.usbdevice_list; otherwise, only hvm.usbdevice will be available. > > v2: > - Throw an error if both usbdevice and usbdevice_list are set > - Update and clarify definition based on feedback > - Previous patches means this works for both traditional and upstreamIs this a requirement? Did this work with qemu-xen-trad with xend? Unless it did I''m not especially in favour of adding new features to qemu-xen-trad just because it looks easy to do so.> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com> > > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h > --- a/tools/libxl/libxl.h > +++ b/tools/libxl/libxl.h > @@ -266,6 +266,22 @@ > #endif > #endif > > +/* > + * LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST > + * > + * If this is defined, then the libxl_domain_build_info structure will > + * contain hvm.usbdevice_list, a libxl_string_list type that contains > + * a list of USB devices to specify on the qemu command-line. > + * > + * If it is set, callers may use either hvm.usbdevice or > + * hvm.usbdevice_list, but not both; if both are set, libxl will > + * throw an error. > + * > + * If this is not defined, callers can only use hvm.usbdevice. Note > + * that this means only one device can be added at domain build time. > + */ > +#define LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST 1 > + > /* Functions annotated with LIBXL_EXTERNAL_CALLERS_ONLY may not be > * called from within libxl itself. Callers outside libxl, who > * do not #include libxl_internal.h, are fine. */ > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > --- a/tools/libxl/libxl_dm.c > +++ b/tools/libxl/libxl_dm.c > @@ -322,11 +322,28 @@ static char ** libxl__build_device_model > > } > > - if (libxl_defbool_val(b_info->u.hvm.usb) || b_info->u.hvm.usbdevice) { > + if (libxl_defbool_val(b_info->u.hvm.usb) > + || b_info->u.hvm.usbdevice > + || b_info->u.hvm.usbdevice_list) { > + if ( b_info->u.hvm.usbdevice && b_info->u.hvm.usbdevice_list ) > + { > + LOG(ERROR, "%s: Both usbdevice and usbdevice_list set", > + __func__); > + return NULL; > + } > flexarray_append(dm_args, "-usb"); > if (b_info->u.hvm.usbdevice) { > flexarray_vappend(dm_args, > "-usbdevice", b_info->u.hvm.usbdevice, NULL); > + } else if (b_info->u.hvm.usbdevice_list) { > + char **p; > + for (p = b_info->u.hvm.usbdevice_list; > + *p; > + p++) {Is the line too long if you unfold this? It doesn''t look to me like it should be.> + flexarray_vappend(dm_args, > + "-usbdevice", > + *p, NULL); > + } > } > } > if (b_info->u.hvm.soundhw) { > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -326,6 +326,7 @@ libxl_domain_build_info = Struct("domain > ("usbdevice", string), > ("soundhw", string), > ("xen_platform_pci", libxl_defbool), > + ("usbdevice_list", libxl_string_list),Is there any chance we might want something more structured that a string in the interface in the future? e.g. libxl_device_usb?> ])), > ("pv", Struct(None, [("kernel", string), > ("slack_memkb", MemKB), > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Ian Campbell
2012-Dec-04 15:50 UTC
Re: [PATCH 4 of 4 v2] xl: Accept a list for usbdevice in config file
On Fri, 2012-11-30 at 16:24 +0000, George Dunlap wrote:> # HG changeset patch > # User George Dunlap <george.dunlap@eu.citrix.com> > # Date 1354291988 0 > # Node ID 043d69be04c91eaead159e8e74999c59eb68f560 > # Parent af47b01d2dbe0e916e1014a264845c48d6ef8108 > xl: Accept a list for usbdevice in config file > > Allow the "usbdevice" key to accept a list of USB devices, and pass > them in using the new usbdevice_list domain build element. > > For backwards compatibility, still accept singleton values. > > Also update the xl.cfg manpage, adding information about how to pass > through host devices. > > v2: > - Add some verbiage to make it clear that "usb" is for emulated devices > - Reference qemu manual for more usbdevice options > > Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>Acked-by: Ian Campbell <ian.campbell@citrix.com>> > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5 > --- a/docs/man/xl.cfg.pod.5 > +++ b/docs/man/xl.cfg.pod.5 > @@ -1107,17 +1107,27 @@ device. > > =item B<usb=BOOLEAN> > > -Enables or disables a USB bus in the guest. > +Enables or disables an emulated USB bus in the guest. > > -=item B<usbdevice=DEVICE> > +=item B<usbdevice=[ "DEVICE", "DEVICE", ...]> > > -Adds B<DEVICE> to the USB bus. The USB bus must also be enabled using > -B<usb=1>. The most common use for this option is B<usbdevice=tablet> > -which adds pointer device using absolute coordinates. Such devices > -function better than relative coordinate devices (such as a standard > -mouse) since many methods of exporting guest graphics (such as VNC) > -work better in this mode. Note that this is independent of the actual > -pointer device you are using on the host/client side. > +Adds B<DEVICE>s to the emulated USB bus. The USB bus must also be > +enabled using B<usb=1>. The most common use for this option is > +B<usbdevice=[''tablet'']> which adds pointer device using absolute > +coordinates. Such devices function better than relative coordinate > +devices (such as a standard mouse) since many methods of exporting > +guest graphics (such as VNC) work better in this mode. Note that this > +is independent of the actual pointer device you are using on the > +host/client side. > + > +Host devices can also be passed through in this way, by specifying > +host:USBID, where USBID is of the form xxxx:yyyy. The USBID can > +typically be found by using lsusb or usb-devices. > + > +The form usbdevice=DEVICE is also accepted for backwards compatibility. > + > +More valid options can be found in the "usbdevice" section of the qemu > +documentation. > > =back > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -1486,8 +1486,23 @@ skip_vfb: > xlu_cfg_replace_string (config, "serial", &b_info->u.hvm.serial, 0); > xlu_cfg_replace_string (config, "boot", &b_info->u.hvm.boot, 0); > xlu_cfg_get_defbool(config, "usb", &b_info->u.hvm.usb, 0); > - xlu_cfg_replace_string (config, "usbdevice", > - &b_info->u.hvm.usbdevice, 0); > + switch (xlu_cfg_get_list_as_string_list(config, "usbdevice", > + &b_info->u.hvm.usbdevice_list, > + 1)) > + { > + > + case 0: break; /* Success */ > + case ESRCH: break; /* Option not present */ > + case EINVAL: > + /* If it''s not a valid list, try reading it as an atom, falling through to > + * an error if it fails */ > + if (!xlu_cfg_replace_string(config, "usbdevice", &b_info->u.hvm.usbdevice, 0)) > + break; > + /* FALLTHRU */ > + default: > + fprintf(stderr,"xl: Unable to parse usbdevice.\n"); > + exit(-ERROR_FAIL); > + } > xlu_cfg_replace_string (config, "soundhw", &b_info->u.hvm.soundhw, 0); > xlu_cfg_get_defbool(config, "xen_platform_pci", > &b_info->u.hvm.xen_platform_pci, 0); > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Ian Campbell
2012-Dec-05 11:22 UTC
Re: [PATCH 2 of 4 v2] xl: Check for duplicate vncdisplay options, and return an error
(seems that I forgot to hit send on this yesterday) On Fri, 2012-11-30 at 16:24 +0000, George Dunlap wrote:> # HG changeset patch > # User George Dunlap <george.dunlap@eu.citrix.com> > # Date 1354291984 0 > # Node ID fa37bd276212a01e3c898b54a7f2385454c406a7 > # Parent 722da032ac90c0e1a78b1154fa588bf295d1f009 > xl: Check for duplicate vncdisplay options, and return an error > > If the user has set a vnc display number both in vnclisten (with > "xxxx:yy"), and with vncdisplay, throw an error. > > Update man pages to match. > > Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>Kind of nasty that we are stuck with both at the libxl interface, but oh well, to late now... Acked + applied, thanks.> > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5 > --- a/docs/man/xl.cfg.pod.5 > +++ b/docs/man/xl.cfg.pod.5 > @@ -350,11 +350,17 @@ other VNC-related settings. The default > > Specifies the IP address, and optionally VNC display number, to use. > > +NB that if you specify the display number here, you should not use > +vncdisplay. > + > =item C<vncdisplay=DISPLAYNUM> > > Specifies the VNC display number to use. The actual TCP port number > will be DISPLAYNUM+5900. > > +NB that you should not use this option if you set the displaynum in the > +vnclisten string. > + > =item C<vncunused=BOOLEAN> > > Requests that the VNC display setup search for a free TCP port to use. > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -1257,6 +1257,7 @@ skip_nic: > vfb->sdl.xauthority = strdup(p2 + 1); > } > } while ((p = strtok(NULL, ",")) != NULL); > + > skip_vfb: > free(buf2); > d_config->num_vfbs++; > @@ -1490,6 +1491,16 @@ skip_vfb: > xlu_cfg_replace_string (config, "soundhw", &b_info->u.hvm.soundhw, 0); > xlu_cfg_get_defbool(config, "xen_platform_pci", > &b_info->u.hvm.xen_platform_pci, 0); > + > + if(b_info->u.hvm.vnc.listen > + && b_info->u.hvm.vnc.display > + && strchr(b_info->u.hvm.vnc.listen, '':'') != NULL) { > + fprintf(stderr, > + "ERROR: Display specified both in vnclisten" > + " and vncdisplay!\n"); > + exit (1); > + > + } > } > > xlu_cfg_destroy(config); > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel