There is a domain id in the virDomain structure as well as in the virDomainObj structure. While the former can become stale the latter is kept up to date. So it is safer to always (virDomainObjPtr)->def->id internally. This will fix issues seen when managing Xen guests through libvirt from virt-manager (not being able to get domain info after define or reboot). This was caused both though libxlDomainGetInfo() only but there were a lot of places that might potentially cause issues, too. Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com&gt; --- src/libxl/libxl_driver.c | 75 +++++++++++++++++++++++----------------------- 1 file changed, 38 insertions(+), 37 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index fc97db4..b5061df 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -770,10 +770,10 @@ libxlDomainSuspend(virDomainPtr dom) priv = vm->privateData; if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_PAUSED) { - if (libxl_domain_pause(priv->ctx, dom->id) != 0) { + if (libxl_domain_pause(priv->ctx, vm->def->id) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to suspend domain '%d' with libxenlight"), - dom->id); + vm->def->id); goto endjob; } @@ -829,10 +829,10 @@ libxlDomainResume(virDomainPtr dom) priv = vm->privateData; if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) { - if (libxl_domain_unpause(priv->ctx, dom->id) != 0) { + if (libxl_domain_unpause(priv->ctx, vm->def->id) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to resume domain '%d' with libxenlight"), - dom->id); + vm->def->id); goto endjob; } @@ -883,10 +883,10 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags) } priv = vm->privateData; - if (libxl_domain_shutdown(priv->ctx, dom->id) != 0) { + if (libxl_domain_shutdown(priv->ctx, vm->def->id) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to shutdown domain '%d' with libxenlight"), - dom->id); + vm->def->id); goto cleanup; } @@ -930,10 +930,10 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags) } priv = vm->privateData; - if (libxl_domain_reboot(priv->ctx, dom->id) != 0) { + if (libxl_domain_reboot(priv->ctx, vm->def->id) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to reboot domain '%d' with libxenlight"), - dom->id); + vm->def->id); goto cleanup; } ret = 0; @@ -974,7 +974,7 @@ libxlDomainDestroyFlags(virDomainPtr dom, priv = vm->privateData; if (libxl_domain_destroy(priv->ctx, vm->def->id, NULL) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to destroy domain '%d'"), dom->id); + _("Failed to destroy domain '%d'"), vm->def->id); goto cleanup; } @@ -1105,10 +1105,10 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, if (flags & VIR_DOMAIN_MEM_LIVE) { priv = vm->privateData; - if (libxl_domain_setmaxmem(priv->ctx, dom->id, newmem) < 0) { + if (libxl_domain_setmaxmem(priv->ctx, vm->def->id, newmem) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to set maximum memory for domain '%d'" - " with libxenlight"), dom->id); + " with libxenlight"), vm->def->id); goto endjob; } } @@ -1138,13 +1138,13 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, priv = vm->privateData; /* Unlock virDomainObj while ballooning memory */ virObjectUnlock(vm); - res = libxl_set_memory_target(priv->ctx, dom->id, newmem, 0, + res = libxl_set_memory_target(priv->ctx, vm->def->id, newmem, 0, /* force */ 1); virObjectLock(vm); if (res < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to set memory for domain '%d'" - " with libxenlight"), dom->id); + " with libxenlight"), vm->def->id); goto endjob; } } @@ -1202,9 +1202,10 @@ libxlDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info) info->memory = vm->def->mem.cur_balloon; info->maxMem = vm->def->mem.max_balloon; } else { - if (libxl_domain_info(priv->ctx, &d_info, dom->id) != 0) { + if (libxl_domain_info(priv->ctx, &d_info, vm->def->id) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("libxl_domain_info failed for domain '%d'"), dom->id); + _("libxl_domain_info failed for domain '%d'"), + vm->def->id); goto cleanup; } info->cpuTime = d_info.cpu_time; @@ -1483,11 +1484,11 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags) if (!(flags & VIR_DUMP_LIVE) && virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { - if (libxl_domain_pause(priv->ctx, dom->id) != 0) { + if (libxl_domain_pause(priv->ctx, vm->def->id) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Before dumping core, failed to suspend domain '%d'" " with libxenlight"), - dom->id); + vm->def->id); goto endjob; } virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_DUMP); @@ -1496,20 +1497,20 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags) /* Unlock virDomainObj while dumping core */ virObjectUnlock(vm); - ret = libxl_domain_core_dump(priv->ctx, dom->id, to, NULL); + ret = libxl_domain_core_dump(priv->ctx, vm->def->id, to, NULL); virObjectLock(vm); if (ret != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to dump core of domain '%d' with libxenlight"), - dom->id); + vm->def->id); ret = -1; goto unpause; } if (flags & VIR_DUMP_CRASH) { - if (libxl_domain_destroy(priv->ctx, dom->id, NULL) < 0) { + if (libxl_domain_destroy(priv->ctx, vm->def->id, NULL) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to destroy domain '%d'"), dom->id); + _("Failed to destroy domain '%d'"), vm->def->id); goto unpause; } @@ -1524,10 +1525,10 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags) unpause: if (virDomainObjIsActive(vm) && paused) { - if (libxl_domain_unpause(priv->ctx, dom->id) != 0) { + if (libxl_domain_unpause(priv->ctx, vm->def->id) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("After dumping core, failed to resume domain '%d' with" - " libxenlight"), dom->id); + " libxenlight"), vm->def->id); } else { virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_UNPAUSED); @@ -1786,19 +1787,19 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, break; case VIR_DOMAIN_VCPU_LIVE: - if (libxl_set_vcpuonline(priv->ctx, dom->id, &map) != 0) { + if (libxl_set_vcpuonline(priv->ctx, vm->def->id, &map) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to set vcpus for domain '%d'" - " with libxenlight"), dom->id); + " with libxenlight"), vm->def->id); goto endjob; } break; case VIR_DOMAIN_VCPU_LIVE | VIR_DOMAIN_VCPU_CONFIG: - if (libxl_set_vcpuonline(priv->ctx, dom->id, &map) != 0) { + if (libxl_set_vcpuonline(priv->ctx, vm->def->id, &map) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to set vcpus for domain '%d'" - " with libxenlight"), dom->id); + " with libxenlight"), vm->def->id); goto endjob; } def->vcpus = nvcpus; @@ -1934,7 +1935,7 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, libxlDomainObjPrivatePtr priv; priv = vm->privateData; - if (libxl_set_vcpuaffinity(priv->ctx, dom->id, vcpu, &map) != 0) { + if (libxl_set_vcpuaffinity(priv->ctx, vm->def->id, vcpu, &map) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to pin vcpu '%d' with libxenlight"), vcpu); @@ -2099,11 +2100,11 @@ libxlDomainGetVcpus(virDomainPtr dom, virVcpuInfoPtr info, int maxinfo, } priv = vm->privateData; - if ((vcpuinfo = libxl_list_vcpu(priv->ctx, dom->id, &maxcpu, + if ((vcpuinfo = libxl_list_vcpu(priv->ctx, vm->def->id, &maxcpu, &hostcpus)) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to list vcpus for domain '%d' with libxenlight"), - dom->id); + vm->def->id); goto cleanup; } @@ -3608,7 +3609,7 @@ libxlDomainGetSchedulerType(virDomainPtr dom, int *nparams) default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to get scheduler id for domain '%d'" - " with libxenlight"), dom->id); + " with libxenlight"), vm->def->id); goto cleanup; } @@ -3659,10 +3660,10 @@ libxlDomainGetSchedulerParametersFlags(virDomainPtr dom, goto cleanup; } - if (libxl_domain_sched_params_get(priv->ctx, dom->id, &sc_info) != 0) { + if (libxl_domain_sched_params_get(priv->ctx, vm->def->id, &sc_info) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to get scheduler parameters for domain '%d'" - " with libxenlight"), dom->id); + " with libxenlight"), vm->def->id); goto cleanup; } @@ -3740,10 +3741,10 @@ libxlDomainSetSchedulerParametersFlags(virDomainPtr dom, goto endjob; } - if (libxl_domain_sched_params_get(priv->ctx, dom->id, &sc_info) != 0) { + if (libxl_domain_sched_params_get(priv->ctx, vm->def->id, &sc_info) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to get scheduler parameters for domain '%d'" - " with libxenlight"), dom->id); + " with libxenlight"), vm->def->id); goto endjob; } @@ -3756,10 +3757,10 @@ libxlDomainSetSchedulerParametersFlags(virDomainPtr dom, sc_info.cap = params[i].value.ui; } - if (libxl_domain_sched_params_set(priv->ctx, dom->id, &sc_info) != 0) { + if (libxl_domain_sched_params_set(priv->ctx, vm->def->id, &sc_info) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to set scheduler parameters for domain '%d'" - " with libxenlight"), dom->id); + " with libxenlight"), vm->def->id); goto endjob; } -- 1.7.9.5

The XML config for a CDROM device can be without a source path, indicating that there is no media present. Without this change the libxl driver fails to start a guest in that case because the libxl library checks for the LIBXL_DISK_FORMAT_EMPTY format type and tries to stat the NULL pointer that gets passed on. Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com&gt; --- src/libxl/libxl_conf.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index de6f7ce..b8de72a 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -827,6 +827,9 @@ libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk *x_disk) x_disk->removable = 1; x_disk->readwrite = !l_disk->readonly; x_disk->is_cdrom = l_disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM ? 1 : 0; + /* An empty CDROM must have the empty format, otherwise libxl fails. */ + if (x_disk->is_cdrom && !x_disk->pdev_path) + x_disk->format = LIBXL_DISK_FORMAT_EMPTY; if (l_disk->transient) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("libxenlight does not support transient disks")); -- 1.7.9.5

This started as an investigation into an issue where libvirt (using the libxl driver) and the Xen host, like an old couple, could not agree on who is responsible for selecting the VNC port to use. Things usually (and a bit surprisingly) did work because, just like that old couple, they had the same idea on what to do by default. However it was possible that this ended up in a big argument. The problem is that display information exists in two different places: in the vfbs list and in the build info. And for launching the device model, only the latter is used. But that never gets initialized from libvirt. So Xen allows the device model to select a default port while libvirt thinks it has told Xen that this is done by libvirt (though the vfbs config). While fixing that, I made a stab at actually evaluating the configuration of the video device. So that it is now possible to at least decide between a Cirrus or standard VGA emulation and to modify the VRAM within certain limits using libvirt. Signed-off-by: Stefan Bader <stefan.bader(a)canonical.com&gt; --- src/libxl/libxl_conf.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index b8de72a..fdd2726 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1298,6 +1298,82 @@ libxlMakeCapabilities(libxl_ctx *ctx) return NULL; } +static void +libxlSetBuildGraphics(virDomainDefPtr def, libxl_domain_config *d_config) +{ + libxl_domain_build_info *b_info = &d_config->b_info; + + /* + * Take the first defined video device (graphics card) to display + * on the first graphics device (display). + * Right now only type and vram info is used and anything beside + * type xen and vga is mapped to cirrus. + */ + if (def->nvideos) { + unsigned int min_vram = 8 * 1024; + + switch (def->videos[0]->type) { + case VIR_DOMAIN_VIDEO_TYPE_VGA: + case VIR_DOMAIN_VIDEO_TYPE_XEN: + b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD; + /* + * Libxl enforces a minimal VRAM size of 8M when using + * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or + * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN. + * Avoid build failures and go with the minimum if less + * is specified. + */ + switch (b_info->device_model_version) { + case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL: + min_vram = 8 * 1024; + break; + case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN: + default: + min_vram = 16 * 1024; + } + break; + default: + /* + * Ignore any other device type and use Cirrus. Again fix + * up the minimal VRAM to what libxl expects. + */ + b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS; + switch (b_info->device_model_version) { + case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL: + min_vram = 4 * 1024; /* Actually the max, too */ + break; + case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN: + default: + min_vram = 8 * 1024; + } + } + b_info->video_memkb = (def->videos[0]->vram > min_vram) ? + def->videos[0]->vram : + LIBXL_MEMKB_DEFAULT; + } else { + libxl_defbool_set(&b_info->u.hvm.nographic, 1); + } + + /* + * When making the list of displays, only VNC and SDL types were + * taken into account. So it seems sensible to connect the default + * video device to the first in the vfb list. + * + * FIXME: Copy the structures and fixing the strings feels a bit dirty. + */ + if (d_config->num_vfbs) { + libxl_device_vfb *vfb0 = &d_config->vfbs[0]; + + b_info->u.hvm.vnc = vfb0->vnc; + VIR_STRDUP(b_info->u.hvm.vnc.listen, vfb0->vnc.listen); + VIR_STRDUP(b_info->u.hvm.vnc.passwd, vfb0->vnc.passwd); + b_info->u.hvm.sdl = vfb0->sdl; + VIR_STRDUP(b_info->u.hvm.sdl.display, vfb0->sdl.display); + VIR_STRDUP(b_info->u.hvm.sdl.xauthority, vfb0->sdl.xauthority); + VIR_STRDUP(b_info->u.hvm.keymap, vfb0->keymap); + } +} + int libxlBuildDomainConfig(libxlDriverPrivatePtr driver, virDomainObjPtr vm, libxl_domain_config *d_config) @@ -1325,6 +1401,15 @@ libxlBuildDomainConfig(libxlDriverPrivatePtr driver, if (libxlMakePciList(def, d_config) < 0) return -1; + /* + * Now that any potential VFBs are defined, it is time to update the + * build info with the data of the primary display. Some day libxl + * might implicitely do so but as it does not right now, better be + * explicit. + */ + if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM) + libxlSetBuildGraphics(def, d_config); + d_config->on_reboot = def->onReboot; d_config->on_poweroff = def->onPoweroff; d_config->on_crash = def->onCrash; -- 1.7.9.5