Re: [libvirt] [PATCH 1/2] libxl: expose multiple emulators per guest in the capabilities XML

Daniel P. Berrange wrote: > On Mon, Apr 29, 2013 at 12:18:56PM -0600, Jim Fehlig wrote: > >> David Scott wrote: >> >>> libxl allows users to choose between two standard emulators: >>> 1. (default in xen-4.2): qemu "traditional" (aka "qemu-dm") >>> 2. (default in xen-4.3): qemu "upstream" (aka "qemu-system-i386") >>> >>> The person who builds and packages xen gets to choose which >>> emulators are built. We examine the filesystem for the emulators >>> at runtime and expose them as separate "domains" within the same >>> "guest" architecture. >>> >>> Signed-off-by: David Scott <dave.scott(a)eu.citrix.com&gt; >>> --- >>> src/libxl/libxl_conf.c | 87 ++++++++++++++++++++++++++++++++++++----------- >>> 1 files changed, 66 insertions(+), 21 deletions(-) >>> >>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c >>> index 7e0753a..472d116 100644 >>> --- a/src/libxl/libxl_conf.c >>> +++ b/src/libxl/libxl_conf.c >>> @@ -29,6 +29,8 @@ >>> #include <libxl.h> >>> #include <sys/types.h> >>> #include <sys/socket.h> >>> +#include <sys/stat.h> >>> +#include <unistd.h> >>> >>> #include "internal.h" >>> #include "virlog.h" >>> @@ -50,6 +52,28 @@ >>> /* see xen-unstable.hg/xen/include/asm-x86/cpufeature.h */ >>> #define LIBXL_X86_FEATURE_PAE_MASK 0x40 >>> >>> +enum emulator_type { >>> + emulator_traditional = 0, >>> + emulator_upstream = 1, >>> + emulator_last = 2, >>> + /* extend with specific qemu versions later */ >>> +}; >>> >>> >> Do you think this will need to be extended in the future? As >> 'qemu-traditional' goes by way of the Dodo, this won't be needed right? >> >> >>> + >>> +#define EMULATOR_LIB64 "/usr/lib64/xen/bin/" >>> +#define EMULATOR_LIB32 "/usr/lib/xen/bin/" >>> + >>> +#define EMULATOR_TRADITIONAL "qemu-dm" >>> +#define EMULATOR_UPSTREAM "qemu-system-i386" >>> >>> >> I think this could be made quite a bit simpler with something like >> >> #define LIBXL_EMULATOR_TRADITIONAL_PATH LIBDIR "/xen/bin/qemu-dm" >> #define LIBXL_EMULATOR_UPSTREAM_PATH LIBDIR "/xen/bin/qemu-sytstem-i386" >> >> >>> + >>> +static const char* emulator_lib64_path [] = { >>> + EMULATOR_LIB64 EMULATOR_TRADITIONAL, >>> + EMULATOR_LIB64 EMULATOR_UPSTREAM, >>> +}; >>> + >>> +static const char* emulator_lib32_path [] = { >>> + EMULATOR_LIB32 EMULATOR_TRADITIONAL, >>> + EMULATOR_LIB32 EMULATOR_UPSTREAM, >>> +}; >>> >>> struct guest_arch { >>> virArch arch; >>> @@ -68,10 +92,11 @@ static virCapsPtr >>> libxlBuildCapabilities(virArch hostarch, >>> int host_pae, >>> struct guest_arch *guest_archs, >>> - int nr_guest_archs) >>> + int nr_guest_archs, >>> + int emulators_found[]) >>> { >>> virCapsPtr caps; >>> - int i; >>> + int i, j; >>> >>> if ((caps = virCapabilitiesNew(hostarch, 1, 1)) == NULL) >>> goto no_memory; >>> @@ -91,12 +116,8 @@ libxlBuildCapabilities(virArch hostarch, >>> if ((guest = virCapabilitiesAddGuest(caps, >>> guest_archs[i].hvm ? "hvm" : "xen", >>> guest_archs[i].arch, >>> - ((hostarch == VIR_ARCH_X86_64) ? >>> - "/usr/lib64/xen/bin/qemu-dm" : >>> - "/usr/lib/xen/bin/qemu-dm"), >>> - (guest_archs[i].hvm ? >>> - "/usr/lib/xen/boot/hvmloader" : >>> - NULL), >>> + NULL, >>> + NULL, >>> 1, >>> machines)) == NULL) { >>> virCapabilitiesFreeMachines(machines, 1); >>> @@ -104,13 +125,21 @@ libxlBuildCapabilities(virArch hostarch, >>> } >>> machines = NULL; >>> >>> - if (virCapabilitiesAddGuestDomain(guest, >>> - "xen", >>> - NULL, >>> - NULL, >>> - 0, >>> - NULL) == NULL) >>> - goto no_memory; >>> + for (j = 0; j < emulator_last; ++j) { >>> + if (emulators_found[j] == -1) /* failure from stat(2) */ >>> + continue; >>> + if (virCapabilitiesAddGuestDomain(guest, >>> + "xen", >>> + ((hostarch == VIR_ARCH_X86_64) ? >>> + emulator_lib64_path[j] : >>> + emulator_lib32_path[j]), >>> + (guest_archs[i].hvm ? >>> + "/usr/lib/xen/boot/hvmloader" : >>> + NULL), >>> + 0, >>> + NULL) == NULL) >>> + goto no_memory; >>> + } >>> >>> >> and then just add the emulators here. E.g. >> >> if (virFileExists(LIBXL_EMULATOR_TRADITIONAL_PATH) { >> if (virCapabilitiesAddGuestDomain(guest, >> "xen", >> LIBXL_EMULATOR_TRADITIONAL_PATH >> (guest_archs[i].hvm ? >> "/usr/lib/xen/boot/hvmloader" : >> NULL), >> 0, >> NULL) == NULL) >> goto no_memory; >> } >> if (virFileExists(LIBXL_EMULATOR_UPSTREAM_PATH) { >> if (virCapabilitiesAddGuestDomain(guest, >> "xen", >> LIBXL_EMULATOR_UPSTREAM_PATH >> (guest_archs[i].hvm ? >> "/usr/lib/xen/boot/hvmloader" : >> NULL), >> 0, >> NULL) == NULL) >> goto no_memory; >> } >> > > > NB, for any single (arch, domain, os_type) triple, we should only > report one <guest> in the capabilities XML. IIUC, your code above > report cause us to have two entries for the same triple. FYI, I advised David on this approach after observing the qemu driver behavior with both qemu and kvm packages installed. On this system, the capabilities for a x86-86, hvm guest contain <guest> <os_type>hvm</os_type> <arch name='x86_64'> <wordsize>64</wordsize> <emulator>/usr/bin/qemu-system-x86_64</emulator> <machine>pc-1.1</machine> <machine canonical='pc-1.1'>pc</machine> <machine>pc-1.0</machine> <machine>pc-0.15</machine> <machine>pc-0.14</machine> <machine>pc-0.13</machine> <machine>pc-0.12</machine> <machine>pc-0.11</machine> <machine>pc-0.10</machine> <machine>isapc</machine> <domain type='qemu'> </domain> <domain type='kvm'> <emulator>/usr/bin/qemu-kvm</emulator> <machine>pc-1.1</machine> <machine canonical='pc-1.1'>pc</machine> <machine>pc-1.0</machine> <machine>pc-0.15</machine> <machine>pc-0.14</machine> <machine>pc-0.13</machine> <machine>pc-0.12</machine> <machine>pc-0.11</machine> <machine>pc-0.10</machine> <machine>isapc</machine> </domain> </arch> <features> <cpuselection/> <deviceboot/> <acpi default='on' toggle='yes'/> <apic default='on' toggle='no'/> </features> </guest> After a second look, the <domain type='kvm'> and <domain type='qemu'> elements and their subelements aren't symmetrical. Seems <domain type='qemu'> should include the emulator and supported machines, e.g.