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@xxxxxxxxxxxxx>
>>> ---
>>> 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.
Right. David mentioned in the cover letter that the resulting
capabilities would looks something like
<arch name='i686'>
<wordsize>32</wordsize>
<machine>xenfv</machine>
<domain type='xen'>
<emulator>/usr/lib64/xen/bin/qemu-dm</emulator>
<loader>/usr/lib/xen/boot/hvmloader</loader>
</domain>
<domain type='xen'>
<emulator>/usr/lib64/xen/bin/qemu-system-i386</emulator>
<loader>/usr/lib/xen/boot/hvmloader</loader>
</domain>
</arch>
> 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.
I gave David this bogus info in response to a patch he posted on
xen-devel while discussing another issue
http://lists.xen.org/archives/html/xen-devel/2013-04/msg02905.html
> Apps are still free to provide emulator binaries
> that are not listed.
>
Yes, I recall this now. I've even used it to call a wrapper around the
installed binary!
David, sorry for wasting your time with the capabilities nonsense :-/.
I think something along the lines of your original patch will be
sufficient, perhaps with checking that the requested emulator actually
exists so an error can be given early.
Regards,
Jim
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list