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

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.