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.

The capabilities XML is about telling the app what the preferred emulator is for a (arch, domain, os_type) triple, not listing all possible emulators. So you really need to chose one, or the other, but not both.

Apps are still free to provide emulator binaries that are not listed.