2010/1/20 Daniel Veillard <veillard@xxxxxxxxxx>:
> On Wed, Jan 20, 2010 at 02:46:10AM +0100, Matthias Bolte wrote:
>> ---
>> src/qemu/qemu_conf.c | 137 ++++++++++++++++++++++++++++++--------------------
>> 1 files changed, 82 insertions(+), 55 deletions(-)
>>
>> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
>> index 92a9348..6141ec6 100644
>> --- a/src/qemu/qemu_conf.c
>> +++ b/src/qemu/qemu_conf.c
>> @@ -379,20 +379,20 @@ static const struct qemu_feature_flags const arch_info_x86_64_flags [] = {
>>
>> /* The archicture tables for supported QEMU archs */
>> static const struct qemu_arch_info const arch_info_hvm[] = {
>> - { "i686", 32, NULL, "/usr/bin/qemu",
>> - "/usr/bin/qemu-system-x86_64", arch_info_i686_flags, 4 },
>> - { "x86_64", 64, NULL, "/usr/bin/qemu-system-x86_64",
>> + { "i686", 32, NULL, "qemu",
>> + "qemu-system-x86_64", arch_info_i686_flags, 4 },
>> + { "x86_64", 64, NULL, "qemu-system-x86_64",
>> NULL, arch_info_x86_64_flags, 2 },
>> - { "arm", 32, NULL, "/usr/bin/qemu-system-arm", NULL, NULL, 0 },
>> - { "mips", 32, NULL, "/usr/bin/qemu-system-mips", NULL, NULL, 0 },
>> - { "mipsel", 32, NULL, "/usr/bin/qemu-system-mipsel", NULL, NULL, 0 },
>> - { "sparc", 32, NULL, "/usr/bin/qemu-system-sparc", NULL, NULL, 0 },
>> - { "ppc", 32, NULL, "/usr/bin/qemu-system-ppc", NULL, NULL, 0 },
>> + { "arm", 32, NULL, "qemu-system-arm", NULL, NULL, 0 },
>> + { "mips", 32, NULL, "qemu-system-mips", NULL, NULL, 0 },
>> + { "mipsel", 32, NULL, "qemu-system-mipsel", NULL, NULL, 0 },
>> + { "sparc", 32, NULL, "qemu-system-sparc", NULL, NULL, 0 },
>> + { "ppc", 32, NULL, "qemu-system-ppc", NULL, NULL, 0 },
>> };
>>
>> static const struct qemu_arch_info const arch_info_xen[] = {
>> - { "i686", 32, "xenner", "/usr/bin/xenner", NULL, arch_info_i686_flags, 4 },
>> - { "x86_64", 64, "xenner", "/usr/bin/xenner", NULL, arch_info_x86_64_flags, 2 },
>> + { "i686", 32, "xenner", "xenner", NULL, arch_info_i686_flags, 4 },
>> + { "x86_64", 64, "xenner", "xenner", NULL, arch_info_x86_64_flags, 2 },
>> };
>>
>>
>> @@ -768,8 +768,8 @@ qemudCapsInitGuest(virCapsPtr caps,
>> int i;
>> int haskvm = 0;
>> int haskqemu = 0;
>> - const char *kvmbin = NULL;
>> - const char *binary = NULL;
>> + char *kvmbin = NULL;
>> + char *binary = NULL;
>> time_t binary_mtime;
>> virCapsGuestMachinePtr *machines = NULL;
>> int nmachines = 0;
>> @@ -779,10 +779,15 @@ qemudCapsInitGuest(virCapsPtr caps,
>> /* Check for existance of base emulator, or alternate base
>> * which can be used with magic cpu choice
>> */
>> - if (access(info->binary, X_OK) == 0)
>> - binary = info->binary;
>> - else if (info->altbinary && access(info->altbinary, X_OK) == 0)
>> - binary = info->altbinary;
>> + binary = virFindFileInPath(info->binary);
>> +
>> + if (binary == NULL || access(binary, X_OK) != 0) {
>> + VIR_FREE(binary);
>> + binary = virFindFileInPath(info->altbinary);
>> +
>> + if (binary != NULL && access(binary, X_OK) != 0)
>> + VIR_FREE(binary);
>> + }
>>
>> /* Can use acceleration for KVM/KQEMU if
>> * - host & guest arches match
>> @@ -792,16 +797,30 @@ qemudCapsInitGuest(virCapsPtr caps,
>> */
>> if (STREQ(info->arch, hostmachine) ||
>> (STREQ(hostmachine, "x86_64") && STREQ(info->arch, "i686"))) {
>> - const char *const kvmbins[] = { "/usr/bin/qemu-kvm", /* Fedora */
>> - "/usr/bin/kvm" }; /* Upstream .spec */
>> + if (access("/dev/kvm", F_OK) == 0) {
>> + const char *const kvmbins[] = { "qemu-kvm", /* Fedora */
>> + "kvm" }; /* Upstream .spec */
>> +
>> + for (i = 0; i < ARRAY_CARDINALITY(kvmbins); ++i) {
>> + kvmbin = virFindFileInPath(kvmbins[i]);
>> +
>> + if (kvmbin == NULL || access(kvmbin, X_OK) != 0) {
>> + VIR_FREE(kvmbin);
>> + continue;
>> + }
>>
>> - for (i = 0; i < ARRAY_CARDINALITY(kvmbins); ++i) {
>> - if (access(kvmbins[i], X_OK) == 0 &&
>> - access("/dev/kvm", F_OK) == 0) {
>> haskvm = 1;
>> - kvmbin = kvmbins[i];
>> - if (!binary)
>> - binary = kvmbin;
>> +
>> + if (binary == NULL) {
>> + binary = strdup(kvmbin);
>> +
>> + if (binary == NULL) {
>> + virReportOOMError(NULL);
>> + VIR_FREE(kvmbin);
>> + return -1;
>> + }
>> + }
>> +
>> break;
>> }
>> }
>> @@ -826,21 +845,18 @@ qemudCapsInitGuest(virCapsPtr caps,
>> virCapsGuestMachinePtr machine;
>>
>> if (VIR_ALLOC(machine) < 0) {
>> - virReportOOMError(NULL);
>> - return -1;
>> + goto no_memory;
>> }
>>
>> if (!(machine->name = strdup(info->machine))) {
>> - virReportOOMError(NULL);
>> VIR_FREE(machine);
>> - return -1;
>> + goto no_memory;
>> }
>>
>> if (VIR_ALLOC_N(machines, 1) < 0) {
>> - virReportOOMError(NULL);
>> VIR_FREE(machine->name);
>> VIR_FREE(machine);
>> - return -1;
>> + goto no_memory;
>> }
>>
>> machines[0] = machine;
>> @@ -853,7 +869,7 @@ qemudCapsInitGuest(virCapsPtr caps,
>> old_caps, &machines, &nmachines);
>> if (probe &&
>> qemudProbeMachineTypes(binary, &machines, &nmachines) < 0)
>> - return -1;
>> + goto error;
>> }
>>
>> /* We register kvm as the base emulator too, since we can
>> @@ -865,21 +881,18 @@ qemudCapsInitGuest(virCapsPtr caps,
>> binary,
>> NULL,
>> nmachines,
>> - machines)) == NULL) {
>> - for (i = 0; i < nmachines; i++) {
>> - VIR_FREE(machines[i]->name);
>> - VIR_FREE(machines[i]);
>> - }
>> - VIR_FREE(machines);
>> - return -1;
>> - }
>> + machines)) == NULL)
>> + goto error;
>> +
>> + machines = NULL;
>> + nmachines = 0;
>>
>> guest->arch.defaultInfo.emulator_mtime = binary_mtime;
>>
>> if (qemudProbeCPUModels(binary, info->arch, &ncpus, NULL) == 0
>> && ncpus > 0
>> && !virCapabilitiesAddGuestFeature(guest, "cpuselection", 1, 0))
>> - return -1;
>> + goto error;
>>
>> if (hvm) {
>> if (virCapabilitiesAddGuestDomain(guest,
>> @@ -888,7 +901,7 @@ qemudCapsInitGuest(virCapsPtr caps,
>> NULL,
>> 0,
>> NULL) == NULL)
>> - return -1;
>> + goto error;
>>
>> if (haskqemu &&
>> virCapabilitiesAddGuestDomain(guest,
>> @@ -897,7 +910,7 @@ qemudCapsInitGuest(virCapsPtr caps,
>> NULL,
>> 0,
>> NULL) == NULL)
>> - return -1;
>> + goto error;
>>
>> if (haskvm) {
>> virCapsGuestDomainPtr dom;
>> @@ -911,9 +924,6 @@ qemudCapsInitGuest(virCapsPtr caps,
>> binary_mtime = 0;
>> }
>>
>> - machines = NULL;
>> - nmachines = 0;
>> -
>> if (!STREQ(binary, kvmbin)) {
>> int probe = 1;
>> if (old_caps && binary_mtime)
>> @@ -922,7 +932,7 @@ qemudCapsInitGuest(virCapsPtr caps,
>> old_caps, &machines, &nmachines);
>> if (probe &&
>> qemudProbeMachineTypes(kvmbin, &machines, &nmachines) < 0)
>> - return -1;
>> + goto error;
>> }
>>
>> if ((dom = virCapabilitiesAddGuestDomain(guest,
>> @@ -931,14 +941,12 @@ qemudCapsInitGuest(virCapsPtr caps,
>> NULL,
>> nmachines,
>> machines)) == NULL) {
>> - for (i = 0; i < nmachines; i++) {
>> - VIR_FREE(machines[i]->name);
>> - VIR_FREE(machines[i]);
>> - }
>> - VIR_FREE(machines);
>> - return -1;
>> + goto error;
>> }
>>
>> + machines = NULL;
>> + nmachines = 0;
>> +
>> dom->info.emulator_mtime = binary_mtime;
>> }
>> } else {
>> @@ -948,7 +956,7 @@ qemudCapsInitGuest(virCapsPtr caps,
>> NULL,
>> 0,
>> NULL) == NULL)
>> - return -1;
>> + goto error;
>> }
>>
>> if (info->nflags) {
>> @@ -957,11 +965,24 @@ qemudCapsInitGuest(virCapsPtr caps,
>> info->flags[i].name,
>> info->flags[i].default_on,
>> info->flags[i].toggle) == NULL)
>> - return -1;
>> + goto error;
>> }
>> }
>>
>> + VIR_FREE(binary);
>> + VIR_FREE(kvmbin);
>> +
>> return 0;
>> +
>> +no_memory:
>> + virReportOOMError(NULL);
>> +
>> +error:
>> + VIR_FREE(binary);
>> + VIR_FREE(kvmbin);
>> + virCapabilitiesFreeMachines(machines, nmachines);
>> +
>> + return -1;
>> }
>>
>>
>> @@ -1011,6 +1032,7 @@ virCapsPtr qemudCapsInit(virCapsPtr old_caps) {
>> struct utsname utsname;
>> virCapsPtr caps;
>> int i;
>> + char *xenner = NULL;
>>
>> /* Really, this never fails - look at the man-page. */
>> uname (&utsname);
>> @@ -1051,7 +1073,9 @@ virCapsPtr qemudCapsInit(virCapsPtr old_caps) {
>> goto no_memory;
>>
>> /* Then possibly the Xen paravirt guests (ie Xenner */
>> - if (access("/usr/bin/xenner", X_OK) == 0 &&
>> + xenner = virFindFileInPath("xenner");
>> +
>> + if (xenner != NULL && access(xenner, X_OK) == 0 &&
>> access("/dev/kvm", F_OK) == 0) {
>> for (i = 0 ; i < ARRAY_CARDINALITY(arch_info_xen) ; i++)
>> /* Allow Xen 32-on-32, 32-on-64 and 64-on-64 */
>> @@ -1065,12 +1089,15 @@ virCapsPtr qemudCapsInit(virCapsPtr old_caps) {
>> }
>> }
>>
>> + VIR_FREE(xenner);
>> +
>> /* QEMU Requires an emulator in the XML */
>> virCapabilitiesSetEmulatorRequired(caps);
>>
>> return caps;
>>
>> no_memory:
>> + VIR_FREE(xenner);
>> virCapabilitiesFree(caps);
>> return NULL;
>> }
>
> Pro: it's more flexible
> Cons: it's less rigid
>
> :-)
>
> I think it's fine since in the majority of cases that code will be run
> by libvirtd, which as a daemon will have a relatively well defined and
> contained PATH . Like when a rogue shared library si available in some
> common place that lead to very painful debugging when an user has a
> problem, rather than the rather straighforward error about a missing
> binary the current version. It's a bit of a double edged sword, but in
> that case I think it's fine. But I could see how others could disagree
>
> ACK from my POV,
>
> Daniel
>
Actually this code will never be executed outside of libvirtd, because
it's executed as part of the QEMU state driver startup only.
Well, I wouldn't compare this to a "rogue shared library" or
LD_PRELOAD stuff, because you can use virsh capabilities for example
to see if it picks up unexpected QEMU binaries. So this shouldn't make
debugging harder. You need to know where to look for information when
debugging, that's right.
Also this make libvirt behave more like the user expects it to. There
were some people on the mailing list and on IRC lately that had
problems with the hardcoding of /usr/bin. They expected libvirt to
pick up their QEMU binaries in /usr/local/bin for example.
Matthias
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list