[libvirt] [PATCH] Canonicalize qemu machine types
In qemu-0.11 there is a 'pc-0.10' machine type which allows you to run guests with a machine which is compatible with the pc machine in qemu-0.10 - e.g. using the original PCI class for virtio-blk and virtio-console and disabling MSI support in virtio-net. The idea here is that we don't want to surprise guests by changing the hardware when qemu is updated. I've just posted some patches for qemu-0.11 which allows libvirt to canonicalize the 'pc' machine alias to the latest machine version. This patches makes us use that so that when a guest is configured to use the 'pc' machine type, we resolve that to 'pc-0.11' machine and save that in the guest XML. See also: https://fedoraproject.org/wiki/Features/KVM_Stable_Guest_ABI * src/qemu_conf.c: add qemudCanonicalizeMachine() to parse the output of 'qemu -M ?' * src/qemu_driver.c: canonicalize the machine type in qemudDomainDefine() --- src/qemu_conf.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu_conf.h | 3 + src/qemu_driver.c | 3 + 3 files changed, 120 insertions(+), 0 deletions(-) diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 4043d70..3f4edfa 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -470,6 +470,120 @@ virCapsPtr qemudCapsInit(void) { return NULL; } +/* Format is: + * <machine> <desc> [(alias of <machine>)] + */ +static int +qemudParseMachineTypesStr(const char *machines ATTRIBUTE_UNUSED, + const char *machine ATTRIBUTE_UNUSED, + char **canonical) +{ + const char *p = machines; + + *canonical = NULL; + + do { + const char *eol; + char *s; + + if (!(eol = strchr(p, '\n'))) + return -1; /* eof file without finding @machine */ + + if (!STRPREFIX(p, machine)) { + p = eol + 1; + continue; /* doesn't match @machine */ + } + + p += strlen(machine); + + if (*p != ' ') { + p = eol + 1; + continue; /* not a complete match of @machine */ + } + + do { + p++; + } while (*p == ' '); + + p = strstr(p, "(alias of "); + if (!p || p > eol) + return 0; /* not an alias, name is canonical */ + + *canonical = strndup(p + strlen("(alias of "), eol - p); + + s = strchr(*canonical, ')'); + if (!s) { + VIR_FREE(*canonical); + *canonical = NULL; + return -1; /* output is screwed up */ + } + + *s = '\0'; + break; + } while (1); + + return 0; +} + +int +qemudCanonicalizeMachine(virConnectPtr conn ATTRIBUTE_UNUSED, + virDomainDefPtr def) +{ + const char *const qemuarg[] = { def->emulator, "-M", "?", NULL }; + const char *const qemuenv[] = { "LC_ALL=C", NULL }; + char *machines, *canonical; + enum { MAX_MACHINES_OUTPUT_SIZE = 1024*4 }; + pid_t child; + int newstdout = -1, len; + int ret = -1, status; + + if (virExec(NULL, qemuarg, qemuenv, NULL, + &child, -1, &newstdout, NULL, VIR_EXEC_CLEAR_CAPS) < 0) + return -1; + + len = virFileReadLimFD(newstdout, MAX_MACHINES_OUTPUT_SIZE, &machines); + if (len < 0) { + virReportSystemError(NULL, errno, "%s", + _("Unable to read 'qemu -M ?' output")); + goto cleanup; + } + + if (qemudParseMachineTypesStr(machines, def->os.machine, &canonical) < 0) + goto cleanup2; + + if (canonical) { + VIR_FREE(def->os.machine); + def->os.machine = canonical; + } + + ret = 0; + +cleanup2: + VIR_FREE(machines); +cleanup: + if (close(newstdout) < 0) + ret = -1; + +rewait: + if (waitpid(child, &status, 0) != child) { + if (errno == EINTR) + goto rewait; + + VIR_ERROR(_("Unexpected exit status from qemu %d pid %lu"), + WEXITSTATUS(status), (unsigned long)child); + ret = -1; + } + /* Check & log unexpected exit status, but don't fail, + * as there's really no need to throw an error if we did + * actually read a valid version number above */ + if (WEXITSTATUS(status) != 0) { + VIR_WARN(_("Unexpected exit status '%d', qemu probably failed"), + WEXITSTATUS(status)); + } + + return ret; +} + static unsigned int qemudComputeCmdFlags(const char *help, unsigned int version, unsigned int is_kvm, diff --git a/src/qemu_conf.h b/src/qemu_conf.h index fbf2ab9..b668669 100644 --- a/src/qemu_conf.h +++ b/src/qemu_conf.h @@ -123,6 +123,9 @@ int qemudExtractVersionInfo (const char *qemu, unsigned int *version, unsigned int *flags); +int qemudCanonicalizeMachine (virConnectPtr conn, + virDomainDefPtr def); + int qemudParseHelpStr (const char *str, unsigned int *flags, unsigned int *version, diff --git a/src/qemu_driver.c b/src/qemu_driver.c index d2db1a2..98f8e95 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -4102,6 +4102,9 @@ static virDomainPtr qemudDomainDefine(virConnectPtr conn, const char *xml) { } } + if (qemudCanonicalizeMachine(conn, def) < 0) + goto cleanup; + if (!(vm = virDomainAssignDef(conn, &driver->domains, def))) { -- 1.6.2.5
On Tue, 2009-07-21 at 16:01 +0100, Mark McLoughlin wrote:
In qemu-0.11 there is a 'pc-0.10' machine type which allows you to run guests with a machine which is compatible with the pc machine in qemu-0.10 - e.g. using the original PCI class for virtio-blk and virtio-console and disabling MSI support in virtio-net. The idea here is that we don't want to surprise guests by changing the hardware when qemu is updated.
I've just posted some patches for qemu-0.11 which allows libvirt to canonicalize the 'pc' machine alias to the latest machine version.
qemu-devel archives take a while to refresh, here's the link to the patches: http://lists.gnu.org/archive/html/qemu-devel/2009-07/msg01652.html Cheers, Mark.
On Tue, Jul 21, 2009 at 04:01:37PM +0100, Mark McLoughlin wrote:
In qemu-0.11 there is a 'pc-0.10' machine type which allows you to run guests with a machine which is compatible with the pc machine in qemu-0.10 - e.g. using the original PCI class for virtio-blk and virtio-console and disabling MSI support in virtio-net. The idea here is that we don't want to surprise guests by changing the hardware when qemu is updated.
I've just posted some patches for qemu-0.11 which allows libvirt to canonicalize the 'pc' machine alias to the latest machine version.
This patches makes us use that so that when a guest is configured to use the 'pc' machine type, we resolve that to 'pc-0.11' machine and save that in the guest XML.
We currently also list "valid" machine types in the capabilities XML for each driver. I use quotes, because this list is currently dubiously hardcoded. I'm thinking it might be better to do the parsing of -M ? output as part of the capaiblities intiialization code. The canonicalize method would then just need to query the existing capabilities data & could even be done in the domain XML parser code instead of qemu driver.
--- src/qemu_conf.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu_conf.h | 3 + src/qemu_driver.c | 3 + 3 files changed, 120 insertions(+), 0 deletions(-)
diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 4043d70..3f4edfa 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -470,6 +470,120 @@ virCapsPtr qemudCapsInit(void) { return NULL; }
+/* Format is: + * <machine> <desc> [(alias of <machine>)] + */ +static int +qemudParseMachineTypesStr(const char *machines ATTRIBUTE_UNUSED, + const char *machine ATTRIBUTE_UNUSED, + char **canonical) +{ + const char *p = machines; + + *canonical = NULL; + + do { + const char *eol; + char *s; + + if (!(eol = strchr(p, '\n'))) + return -1; /* eof file without finding @machine */ + + if (!STRPREFIX(p, machine)) { + p = eol + 1; + continue; /* doesn't match @machine */ + } + + p += strlen(machine); + + if (*p != ' ') { + p = eol + 1; + continue; /* not a complete match of @machine */ + } + + do { + p++; + } while (*p == ' '); + + p = strstr(p, "(alias of "); + if (!p || p > eol) + return 0; /* not an alias, name is canonical */ + + *canonical = strndup(p + strlen("(alias of "), eol - p);
Need to check for NULL here.
+ + s = strchr(*canonical, ')'); + if (!s) { + VIR_FREE(*canonical); + *canonical = NULL; + return -1; /* output is screwed up */ + } + + *s = '\0'; + break; + } while (1); + + return 0; +} + +int +qemudCanonicalizeMachine(virConnectPtr conn ATTRIBUTE_UNUSED, + virDomainDefPtr def) +{ + const char *const qemuarg[] = { def->emulator, "-M", "?", NULL }; + const char *const qemuenv[] = { "LC_ALL=C", NULL }; + char *machines, *canonical; + enum { MAX_MACHINES_OUTPUT_SIZE = 1024*4 }; + pid_t child; + int newstdout = -1, len; + int ret = -1, status; + + if (virExec(NULL, qemuarg, qemuenv, NULL, + &child, -1, &newstdout, NULL, VIR_EXEC_CLEAR_CAPS) < 0) + return -1; + + len = virFileReadLimFD(newstdout, MAX_MACHINES_OUTPUT_SIZE, &machines); + if (len < 0) { + virReportSystemError(NULL, errno, "%s", + _("Unable to read 'qemu -M ?' output")); + goto cleanup; + } + + if (qemudParseMachineTypesStr(machines, def->os.machine, &canonical) < 0) + goto cleanup2; + + if (canonical) { + VIR_FREE(def->os.machine); + def->os.machine = canonical; + } + + ret = 0; + +cleanup2: + VIR_FREE(machines); +cleanup: + if (close(newstdout) < 0) + ret = -1; + +rewait: + if (waitpid(child, &status, 0) != child) { + if (errno == EINTR) + goto rewait; + + VIR_ERROR(_("Unexpected exit status from qemu %d pid %lu"), + WEXITSTATUS(status), (unsigned long)child); + ret = -1; + } + /* Check & log unexpected exit status, but don't fail, + * as there's really no need to throw an error if we did + * actually read a valid version number above */ + if (WEXITSTATUS(status) != 0) { + VIR_WARN(_("Unexpected exit status '%d', qemu probably failed"), + WEXITSTATUS(status)); + } + + return ret; +} + static unsigned int qemudComputeCmdFlags(const char *help, unsigned int version, unsigned int is_kvm, diff --git a/src/qemu_conf.h b/src/qemu_conf.h index fbf2ab9..b668669 100644 --- a/src/qemu_conf.h +++ b/src/qemu_conf.h @@ -123,6 +123,9 @@ int qemudExtractVersionInfo (const char *qemu, unsigned int *version, unsigned int *flags);
+int qemudCanonicalizeMachine (virConnectPtr conn, + virDomainDefPtr def); + int qemudParseHelpStr (const char *str, unsigned int *flags, unsigned int *version, diff --git a/src/qemu_driver.c b/src/qemu_driver.c index d2db1a2..98f8e95 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -4102,6 +4102,9 @@ static virDomainPtr qemudDomainDefine(virConnectPtr conn, const char *xml) { } }
+ if (qemudCanonicalizeMachine(conn, def) < 0) + goto cleanup; + if (!(vm = virDomainAssignDef(conn, &driver->domains, def))) { -- 1.6.2.5
Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Hey, Here's a series of patches to implement danpb's suggestion of using 'qemu -M ?' to probe for valid machine types and then to build on that for the machine type canonicalization. Cheers, Mark.
On 07/23/2009 01:34 PM, Mark McLoughlin wrote:
Hey, Here's a series of patches to implement danpb's suggestion of using 'qemu -M ?' to probe for valid machine types and then to build on that for the machine type canonicalization.
Cheers, Mark.
What's the speed difference of GetCapabilities after these patches? Not that it needs to be fast, but it would be useful to know if it's slowwwwwwww. - Cole
On Thu, Jul 23, 2009 at 01:56:21PM -0400, Cole Robinson wrote:
On 07/23/2009 01:34 PM, Mark McLoughlin wrote:
Hey, Here's a series of patches to implement danpb's suggestion of using 'qemu -M ?' to probe for valid machine types and then to build on that for the machine type canonicalization.
What's the speed difference of GetCapabilities after these patches? Not that it needs to be fast, but it would be useful to know if it's slowwwwwwww.
Urgh, I forgot that that re-builds capabilities on every invocation :-( It might be better to move the refresh into the SIGHUP handler, and have an RPM %trigger against 'qemu' install to send libvirt SIGHUP Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 07/23/2009 02:20 PM, Daniel P. Berrange wrote:
On Thu, Jul 23, 2009 at 01:56:21PM -0400, Cole Robinson wrote:
On 07/23/2009 01:34 PM, Mark McLoughlin wrote:
Hey, Here's a series of patches to implement danpb's suggestion of using 'qemu -M ?' to probe for valid machine types and then to build on that for the machine type canonicalization.
What's the speed difference of GetCapabilities after these patches? Not that it needs to be fast, but it would be useful to know if it's slowwwwwwww.
Urgh, I forgot that that re-builds capabilities on every invocation :-( It might be better to move the refresh into the SIGHUP handler, and have an RPM %trigger against 'qemu' install to send libvirt SIGHUP
Daniel
Maybe internally we could add a CapsRefresh function, which can update the emulator list and scrape output from newly installed emulators, but assume that -M output doesn't change from the first run (or any other output if we ever wanted to scrape device lists, etc.). - Cole
On Thu, 2009-07-23 at 13:56 -0400, Cole Robinson wrote:
On 07/23/2009 01:34 PM, Mark McLoughlin wrote:
Hey, Here's a series of patches to implement danpb's suggestion of using 'qemu -M ?' to probe for valid machine types and then to build on that for the machine type canonicalization.
Cheers, Mark.
What's the speed difference of GetCapabilities after these patches? Not that it needs to be fast, but it would be useful to know if it's slowwwwwwww.
It's a fair point, especially since it wasn't long ago we discussed this: http://www.redhat.com/archives/libvir-list/2009-April/msg00537.html I took your little script, made it do 100 iterations, ran that 20 times on F-11 and averaged the results: - Before this series of patches, it's averaging at 10ms, giving 100us per call - After this series with only qemu-system-x86 installed, it's averaging at 1.7s, giving 17ms per call - With qemu-system-arm/mips/ppc/sparc and xenner installed, it's averaging at 5.8s, giving 58ms per call 60ms vs 100us is a huge slowdown, relatively speaking. Is 60ms an unacceptable worst-case got a GetCapabilities call? Probably not, but it is certainly approaching unacceptable. Cheers, Mark.
By probing for qemu machine types, we increased the time of a GetCapabilities call from 100us to a whopping 60ms. This patch takes the approach of only probing for machine types when the mtime of the emulator binary changed since the last time the capabilities were generated. This brings the time of the call back to what it was for the most common case. * src/capabilities.h: cache the emulator binary mtime * src/qemu_conf.c: add qemudGetOldMachines() to copy the machine types from the old caps struct if the mtime for the binary hasn't changed * src/qemu_conf.h, src/qemu_driver.c: pass the old caps pointer to qemudCapsInit() --- src/capabilities.h | 1 + src/qemu_conf.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++--- src/qemu_conf.h | 2 +- src/qemu_driver.c | 4 +- 4 files changed, 95 insertions(+), 9 deletions(-) diff --git a/src/capabilities.h b/src/capabilities.h index aab084c..b958d95 100644 --- a/src/capabilities.h +++ b/src/capabilities.h @@ -49,6 +49,7 @@ struct _virCapsGuestDomainInfo { char *loader; int nmachines; virCapsGuestMachinePtr *machines; + time_t emulator_mtime; /* do @machines need refreshing? */ }; typedef struct _virCapsGuestDomain virCapsGuestDomain; diff --git a/src/qemu_conf.c b/src/qemu_conf.c index a3986df..a9d559e 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -457,7 +457,72 @@ rewait: } static int +qemudGetOldMachines(const char *ostype, + const char *arch, + int wordsize, + const char *emulator, + time_t emulator_mtime, + virCapsPtr old_caps, + virCapsGuestMachinePtr **machines, + int *nmachines) +{ + int i; + + for (i = 0; i < old_caps->nguests; i++) { + virCapsGuestPtr guest = old_caps->guests[i]; + virCapsGuestDomainInfoPtr info = &guest->arch.defaultInfo; + virCapsGuestMachinePtr *list; + + if (!STREQ(ostype, guest->ostype) || + !STREQ(arch, guest->arch.name) || + wordsize != guest->arch.wordsize || + !STREQ(emulator, info->emulator)) + continue; + + if (emulator_mtime != info->emulator_mtime) { + VIR_DEBUG("mtime on %s has changed, refreshing machine types", + info->emulator); + return 0; + } + + /* It sucks to have to dup these, when we're most likely going + * to free the old caps anyway - except if an error occurs, we'll + * stick with the old caps. + * Also, if we get OOM here, just let the caller try and probe + * the binary directly, which will probably fail too. + */ + if (VIR_ALLOC_N(list, info->nmachines) < 0) + return 0; + + for (i = 0; i < info->nmachines; i++) { + if (VIR_ALLOC(list[i]) < 0) { + virCapabilitiesFreeMachines(list, info->nmachines); + return 0; + } + if (info->machines[i]->name && + !(list[i]->name = strdup(info->machines[i]->name))) { + virCapabilitiesFreeMachines(list, info->nmachines); + return 0; + } + if (info->machines[i]->canonical && + !(list[i]->canonical = strdup(info->machines[i]->canonical))) { + virCapabilitiesFreeMachines(list, info->nmachines); + return 0; + } + } + + *machines = list; + *nmachines = info->nmachines; + + return 1; + } + + return 0; +} + +static int qemudCapsInitGuest(virCapsPtr caps, + virCapsPtr old_caps, const char *hostmachine, const struct qemu_arch_info *info, int hvm) { @@ -467,8 +532,10 @@ qemudCapsInitGuest(virCapsPtr caps, int haskqemu = 0; const char *kvmbin = NULL; const char *binary = NULL; + time_t binary_mtime; virCapsGuestMachinePtr *machines = NULL; int nmachines = 0; + struct stat st; /* Check for existance of base emulator, or alternate base * which can be used with magic cpu choice @@ -507,6 +574,15 @@ qemudCapsInitGuest(virCapsPtr caps, if (!binary) return 0; + if (stat(binary, &st) == 0) { + binary_mtime = st.st_mtime; + } else { + char ebuf[1024]; + VIR_WARN(_("Failed to stat %s, most peculiar : %s"), + binary, virStrerror(errno, ebuf, sizeof(ebuf))); + binary_mtime = 0; + } + if (info->machine) { virCapsGuestMachinePtr machine; @@ -526,9 +602,16 @@ qemudCapsInitGuest(virCapsPtr caps, machines[0] = machine; nmachines = 1; - - } else if (qemudProbeMachineTypes(binary, &machines, &nmachines) < 0) - return -1; + } else { + int probe = 1; + if (old_caps && binary_mtime) + probe = !qemudGetOldMachines(hvm ? "hvm" : "xen", info->arch, + info->wordsize, binary, binary_mtime, + old_caps, &machines, &nmachines); + if (probe && + qemudProbeMachineTypes(binary, &machines, &nmachines) < 0) + return -1; + } /* We register kvm as the base emulator too, since we can * just give -no-kvm to disable acceleration if required */ @@ -548,6 +631,8 @@ qemudCapsInitGuest(virCapsPtr caps, return -1; } + guest->arch.defaultInfo.emulator_mtime = binary_mtime; + if (hvm) { if (virCapabilitiesAddGuestDomain(guest, "qemu", @@ -597,7 +682,7 @@ qemudCapsInitGuest(virCapsPtr caps, return 0; } -virCapsPtr qemudCapsInit(void) { +virCapsPtr qemudCapsInit(virCapsPtr old_caps) { struct utsname utsname; virCapsPtr caps; int i; @@ -626,7 +711,7 @@ virCapsPtr qemudCapsInit(void) { /* First the pure HVM guests */ for (i = 0 ; i < ARRAY_CARDINALITY(arch_info_hvm) ; i++) - if (qemudCapsInitGuest(caps, + if (qemudCapsInitGuest(caps, old_caps, utsname.machine, &arch_info_hvm[i], 1) < 0) goto no_memory; @@ -639,7 +724,7 @@ virCapsPtr qemudCapsInit(void) { if (STREQ(arch_info_xen[i].arch, utsname.machine) || (STREQ(utsname.machine, "x86_64") && STREQ(arch_info_xen[i].arch, "i686"))) { - if (qemudCapsInitGuest(caps, + if (qemudCapsInitGuest(caps, old_caps, utsname.machine, &arch_info_xen[i], 0) < 0) goto no_memory; diff --git a/src/qemu_conf.h b/src/qemu_conf.h index 316a787..ad68e31 100644 --- a/src/qemu_conf.h +++ b/src/qemu_conf.h @@ -125,7 +125,7 @@ struct qemud_driver { int qemudLoadDriverConfig(struct qemud_driver *driver, const char *filename); -virCapsPtr qemudCapsInit (void); +virCapsPtr qemudCapsInit (virCapsPtr old_caps); int qemudExtractVersion (virConnectPtr conn, struct qemud_driver *driver); diff --git a/src/qemu_driver.c b/src/qemu_driver.c index d10c0a1..95ee0b4 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -516,7 +516,7 @@ qemudStartup(int privileged) { virStrerror(-rc, buf, sizeof(buf))); } - if ((qemu_driver->caps = qemudCapsInit()) == NULL) + if ((qemu_driver->caps = qemudCapsInit(NULL)) == NULL) goto out_of_memory; if (qemudLoadDriverConfig(qemu_driver, driverConf) < 0) { @@ -2440,7 +2440,7 @@ static char *qemudGetCapabilities(virConnectPtr conn) { char *xml = NULL; qemuDriverLock(driver); - if ((caps = qemudCapsInit()) == NULL) { + if ((caps = qemudCapsInit(qemu_driver->caps)) == NULL) { virReportOOMError(conn); goto cleanup; } -- 1.6.2.5
Mark McLoughlin wrote:
By probing for qemu machine types, we increased the time of a GetCapabilities call from 100us to a whopping 60ms.
This patch takes the approach of only probing for machine types when the mtime of the emulator binary changed since the last time the capabilities were generated.
Good idea!
This brings the time of the call back to what it was for the most common case.
* src/capabilities.h: cache the emulator binary mtime
* src/qemu_conf.c: add qemudGetOldMachines() to copy the machine types from the old caps struct if the mtime for the binary hasn't changed
* src/qemu_conf.h, src/qemu_driver.c: pass the old caps pointer to qemudCapsInit() --- src/capabilities.h | 1 + src/qemu_conf.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++--- src/qemu_conf.h | 2 +- src/qemu_driver.c | 4 +- 4 files changed, 95 insertions(+), 9 deletions(-)
... ACK, patch looks good to me. - Cole
There's no need for the hasbase/hasaltbase confusion, just store the first binary path found in a variable. * src/qemu_conf.c: kill hasbase/hasaltbase logic in qemudCapsInitGuest() --- src/qemu_conf.c | 17 +++++++++-------- 1 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 96f83cb..4bd511a 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -309,17 +309,18 @@ qemudCapsInitGuest(virCapsPtr caps, int hvm) { virCapsGuestPtr guest; int i; - int hasbase = 0; - int hasaltbase = 0; int haskvm = 0; int haskqemu = 0; const char *kvmbin = NULL; + const char *binary = NULL; /* Check for existance of base emulator, or alternate base * which can be used with magic cpu choice */ - hasbase = (access(info->binary, X_OK) == 0); - hasaltbase = (info->altbinary && access(info->altbinary, X_OK) == 0); + if (access(info->binary, X_OK) == 0) + binary = info->binary; + else if (info->altbinary && access(info->altbinary, X_OK) == 0) + binary = info->altbinary; /* Can use acceleration for KVM/KQEMU if * - host & guest arches match @@ -337,6 +338,8 @@ qemudCapsInitGuest(virCapsPtr caps, access("/dev/kvm", F_OK) == 0) { haskvm = 1; kvmbin = kvmbins[i]; + if (!binary) + binary = kvmbin; break; } } @@ -345,8 +348,7 @@ qemudCapsInitGuest(virCapsPtr caps, haskqemu = 1; } - - if (!hasbase && !hasaltbase && !haskvm) + if (!binary) return 0; /* We register kvm as the base emulator too, since we can @@ -355,8 +357,7 @@ qemudCapsInitGuest(virCapsPtr caps, hvm ? "hvm" : "xen", info->arch, info->wordsize, - (hasbase ? info->binary : - (hasaltbase ? info->altbinary : kvmbin)), + binary, NULL, info->nmachines, info->machines)) == NULL) -- 1.6.2.5
On Thu, Jul 23, 2009 at 06:34:39PM +0100, Mark McLoughlin wrote:
There's no need for the hasbase/hasaltbase confusion, just store the first binary path found in a variable.
* src/qemu_conf.c: kill hasbase/hasaltbase logic in qemudCapsInitGuest()
simplifies things, ACK Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Currently we hardcode the QEMU machine types. We should really just parse the output of 'qemu -M ?' so the lists don't get out of sync. xenner doesn't support '-M ?', so we still need to hardcode that. The horrible (const char *const *) is removed in a subsequent patch. * src/qemu_conf.c: kill the arch_info*machines tables, retain the hardcoded xenner machine type, add qemudProbeMachineTypes() to run and parse 'qemu -M ?' and use it in qemudCapsInitGuest() --- src/qemu_conf.c | 201 ++++++++++++++++++++++++++++++++++++++++++------------- 1 files changed, 154 insertions(+), 47 deletions(-) diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 4bd511a..bdbeff2 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -222,31 +222,6 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, return 0; } -/* The list of possible machine types for various architectures, - as supported by QEMU - taken from 'qemu -M ?' for each arch */ -static const char *const arch_info_hvm_x86_machines[] = { - "pc", "isapc" -}; -static const char *const arch_info_hvm_arm_machines[] = { - "versatilepb","integratorcp","versatileab","realview", - "akita","spitz","borzoi","terrier","sx1-v1","sx1", - "cheetah","n800","n810","lm3s811evb","lm3s6965evb", - "connex","verdex","mainstone","musicpal","tosa", -}; -static const char *const arch_info_hvm_mips_machines[] = { - "mips" -}; -static const char *const arch_info_hvm_sparc_machines[] = { - "sun4m" -}; -static const char *const arch_info_hvm_ppc_machines[] = { - "g3beige", "mac99", "prep" -}; - -static const char *const arch_info_xen_x86_machines[] = { - "xenner" -}; - struct qemu_feature_flags { const char *name; const int default_on; @@ -256,8 +231,7 @@ struct qemu_feature_flags { struct qemu_arch_info { const char *arch; int wordsize; - const char *const *machines; - int nmachines; + const char *machine; const char *binary; const char *altbinary; const struct qemu_feature_flags *flags; @@ -279,29 +253,135 @@ 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, arch_info_hvm_x86_machines, 2, - "/usr/bin/qemu", "/usr/bin/qemu-system-x86_64", arch_info_i686_flags, 4 }, - { "x86_64", 64, arch_info_hvm_x86_machines, 2, - "/usr/bin/qemu-system-x86_64", NULL, arch_info_x86_64_flags, 2 }, - { "arm", 32, arch_info_hvm_arm_machines, 20, - "/usr/bin/qemu-system-arm", NULL, NULL, 0 }, - { "mips", 32, arch_info_hvm_mips_machines, 1, - "/usr/bin/qemu-system-mips", NULL, NULL, 0 }, - { "mipsel", 32, arch_info_hvm_mips_machines, 1, - "/usr/bin/qemu-system-mipsel", NULL, NULL, 0 }, - { "sparc", 32, arch_info_hvm_sparc_machines, 1, - "/usr/bin/qemu-system-sparc", NULL, NULL, 0 }, - { "ppc", 32, arch_info_hvm_ppc_machines, 3, - "/usr/bin/qemu-system-ppc", NULL, NULL, 0 }, + { "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", + 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 }, }; static const struct qemu_arch_info const arch_info_xen[] = { - { "i686", 32, arch_info_xen_x86_machines, 1, - "/usr/bin/xenner", NULL, arch_info_i686_flags, 4 }, - { "x86_64", 64, arch_info_xen_x86_machines, 1, - "/usr/bin/xenner", NULL, arch_info_x86_64_flags, 2 }, + { "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 }, }; + +/* Format is: + * <machine> <desc> [(default)] + */ +static int +qemudParseMachineTypesStr(const char *output, + char ***machines, + int *nmachines) +{ + const char *p = output; + const char *next; + char **list = NULL; + int i, nitems = 0; + + do { + const char *t; + char *machine; + + if ((next = strchr(p, '\n'))) + ++next; + + if (STRPREFIX(p, "Supported machines are:")) + continue; + + if (!(t = strchr(p, ' ')) || (next && t >= next)) + continue; + + if (!(machine = strndup(p, t - p))) + goto error; + + if (VIR_REALLOC_N(list, nitems + 1) < 0) { + VIR_FREE(machine); + goto error; + } + + p = t; + if (!(t = strstr(p, "(default)")) || (next && t >= next)) { + list[nitems++] = machine; + } else { + /* put the default first in the list */ + memmove(list + 1, list, sizeof(*list) * nitems); + list[0] = machine; + nitems++; + } + } while ((p = next)); + + *machines = list; + *nmachines = nitems; + + return 0; + +error: + for (i = 0; i < nitems; i++) + VIR_FREE(list[i]); + VIR_FREE(list); + return -1; +} + +static int +qemudProbeMachineTypes(const char *binary, + char ***machines, + int *nmachines) +{ + const char *const qemuarg[] = { binary, "-M", "?", NULL }; + const char *const qemuenv[] = { "LC_ALL=C", NULL }; + char *output; + enum { MAX_MACHINES_OUTPUT_SIZE = 1024*4 }; + pid_t child; + int newstdout = -1, len; + int ret = -1, status; + + if (virExec(NULL, qemuarg, qemuenv, NULL, + &child, -1, &newstdout, NULL, VIR_EXEC_CLEAR_CAPS) < 0) + return -1; + + len = virFileReadLimFD(newstdout, MAX_MACHINES_OUTPUT_SIZE, &output); + if (len < 0) { + virReportSystemError(NULL, errno, "%s", + _("Unable to read 'qemu -M ?' output")); + goto cleanup; + } + + if (qemudParseMachineTypesStr(output, machines, nmachines) < 0) + goto cleanup2; + + ret = 0; + +cleanup2: + VIR_FREE(output); +cleanup: + if (close(newstdout) < 0) + ret = -1; + +rewait: + if (waitpid(child, &status, 0) != child) { + if (errno == EINTR) + goto rewait; + + VIR_ERROR(_("Unexpected exit status from qemu %d pid %lu"), + WEXITSTATUS(status), (unsigned long)child); + ret = -1; + } + /* Check & log unexpected exit status, but don't fail, + * as there's really no need to throw an error if we did + * actually read a valid version number above */ + if (WEXITSTATUS(status) != 0) { + VIR_WARN(_("Unexpected exit status '%d', qemu probably failed"), + WEXITSTATUS(status)); + } + + return ret; +} + static int qemudCapsInitGuest(virCapsPtr caps, const char *hostmachine, @@ -313,6 +393,8 @@ qemudCapsInitGuest(virCapsPtr caps, int haskqemu = 0; const char *kvmbin = NULL; const char *binary = NULL; + char **machines = NULL; + int nmachines = 0; /* Check for existance of base emulator, or alternate base * which can be used with magic cpu choice @@ -351,6 +433,23 @@ qemudCapsInitGuest(virCapsPtr caps, if (!binary) return 0; + if (info->machine) { + char *machine; + + if (!(machine = strdup(info->machine))) + return -1; + + if (VIR_ALLOC_N(machines, nmachines) < 0) { + VIR_FREE(machine); + return -1; + } + + machines[0] = machine; + nmachines = 1; + + } else if (qemudProbeMachineTypes(binary, &machines, &nmachines) < 0) + return -1; + /* We register kvm as the base emulator too, since we can * just give -no-kvm to disable acceleration if required */ if ((guest = virCapabilitiesAddGuest(caps, @@ -359,9 +458,17 @@ qemudCapsInitGuest(virCapsPtr caps, info->wordsize, binary, NULL, - info->nmachines, - info->machines)) == NULL) + nmachines, + (const char *const *)machines)) == NULL) { + for (i = 0; i < nmachines; i++) + VIR_FREE(machines[i]); + VIR_FREE(machines); return -1; + } + + for (i = 0; i < nmachines; i++) + VIR_FREE(machines[i]); + VIR_FREE(machines); if (hvm) { if (virCapabilitiesAddGuestDomain(guest, -- 1.6.2.5
On Thu, Jul 23, 2009 at 06:34:40PM +0100, Mark McLoughlin wrote:
Currently we hardcode the QEMU machine types. We should really just parse the output of 'qemu -M ?' so the lists don't get out of sync.
yes that makes sense, maintaining that list in libvirt itself is a garanteed way to break things, just looking at the current version our ppc list lists only 3 machine types while the binary reports 7 !
xenner doesn't support '-M ?', so we still need to hardcode that.
The horrible (const char *const *) is removed in a subsequent patch.
* src/qemu_conf.c: kill the arch_info*machines tables, retain the hardcoded xenner machine type, add qemudProbeMachineTypes() to run and parse 'qemu -M ?' and use it in qemudCapsInitGuest() --- [...] +static int +qemudParseMachineTypesStr(const char *output, + char ***machines, + int *nmachines) +{ + const char *p = output; + const char *next; + char **list = NULL; + int i, nitems = 0; + + do { + const char *t; + char *machine; + + if ((next = strchr(p, '\n'))) + ++next; + + if (STRPREFIX(p, "Supported machines are:")) + continue;
It's supposed to happen only on the first line, maybe suppressing the first line if it ends with ':' is a safer bet in case the description changes in the future (might be localized for example). But good enough for now, if only they had used a structured description ...
+ if (!(t = strchr(p, ' ')) || (next && t >= next)) + continue;
Are we garanteed it will always be spaces, seems the case in what I looked but maybe tabs might break this code. Still good enough for now.
+ if (!(machine = strndup(p, t - p))) + goto error; + + if (VIR_REALLOC_N(list, nitems + 1) < 0) { + VIR_FREE(machine); + goto error; + }
Realloc in a loop, but I don't see how to avoid this except going twice though the loop ... anyway this should be small and infrequent. [...]
+static int +qemudProbeMachineTypes(const char *binary, + char ***machines, + int *nmachines) +{ + const char *const qemuarg[] = { binary, "-M", "?", NULL }; + const char *const qemuenv[] = { "LC_ALL=C", NULL };
okay we force the locale ... ACK, this is really an improvement, just that parsing unstructured data sucks, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
A subsequent commit will add a "canonical" field to this structure, this patch basically just prepares the way for that. The new type is added, along with virCapabilitiesAlloc/FreeMachines() helpers and a whole bunch of code to make the transition. One quirk is that virCapabilitiesAddGuestDomain() and virCapabilitiesAddGuest() take ownership of the machine list rather than duping it. This makes sense to avoid needless copying. * src/capabilities.h: add the virCapsGuestMachine struct and use it in virCapsGuestDomainInfo, add prototypes for new functions and update the AddGuest() prototypes * src/capabilities.c: add code for allocating and freeing the new type, change the machines parameter to AddGuest() etc. * src/libvirt_private.syms: export the new helpers * src/qemu_conf.c: update all the machine type code to use the new struct * src/xen_internal.c: ditto * tests/testutilsqemu.c: ditto --- src/capabilities.c | 92 ++++++++++++++++++++++++++++++++++------------ src/capabilities.h | 20 ++++++++- src/libvirt_private.syms | 2 + src/qemu_conf.c | 44 +++++++++++++--------- src/xen_internal.c | 12 +++++- tests/testutilsqemu.c | 27 ++++++++++++- 6 files changed, 147 insertions(+), 50 deletions(-) diff --git a/src/capabilities.c b/src/capabilities.c index def3fd0..afe4d38 100644 --- a/src/capabilities.c +++ b/src/capabilities.c @@ -69,6 +69,16 @@ virCapabilitiesFreeHostNUMACell(virCapsHostNUMACellPtr cell) } static void +virCapabilitiesFreeGuestMachine(virCapsGuestMachinePtr machine) +{ + if (machine == NULL) + return; + VIR_FREE(machine->name); + VIR_FREE(machine->canonical); + VIR_FREE(machine); +} + +static void virCapabilitiesFreeGuestDomain(virCapsGuestDomainPtr dom) { int i; @@ -78,7 +88,7 @@ virCapabilitiesFreeGuestDomain(virCapsGuestDomainPtr dom) VIR_FREE(dom->info.emulator); VIR_FREE(dom->info.loader); for (i = 0 ; i < dom->info.nmachines ; i++) - VIR_FREE(dom->info.machines[i]); + virCapabilitiesFreeGuestMachine(dom->info.machines[i]); VIR_FREE(dom->info.machines); VIR_FREE(dom->type); @@ -107,7 +117,7 @@ virCapabilitiesFreeGuest(virCapsGuestPtr guest) VIR_FREE(guest->arch.defaultInfo.emulator); VIR_FREE(guest->arch.defaultInfo.loader); for (i = 0 ; i < guest->arch.defaultInfo.nmachines ; i++) - VIR_FREE(guest->arch.defaultInfo.machines[i]); + virCapabilitiesFreeGuestMachine(guest->arch.defaultInfo.machines[i]); VIR_FREE(guest->arch.defaultInfo.machines); for (i = 0 ; i < guest->arch.ndomains ; i++) @@ -252,6 +262,53 @@ virCapabilitiesAddHostNUMACell(virCapsPtr caps, return 0; } +/** + * virCapabilitiesAllocMachines: + * @machines: machine variants for emulator ('pc', or 'isapc', etc) + * @nmachines: number of machine variants for emulator + * + * Allocate a table of virCapsGuestMachinePtr from the supplied table + * of machine names. + */ +virCapsGuestMachinePtr * +virCapabilitiesAllocMachines(const char *const *names, int nnames) +{ + virCapsGuestMachinePtr *machines; + int i; + + if (VIR_ALLOC_N(machines, nnames) < 0) + return NULL; + + for (i = 0; i < nnames; i++) { + if (VIR_ALLOC(machines[i]) < 0 || + !(machines[i]->name = strdup(names[i]))) { + virCapabilitiesFreeMachines(machines, nnames); + return NULL; + } + } + + return machines; +} + +/** + * virCapabilitiesFreeMachines: + * @machines: table of vircapsGuestMachinePtr + * + * Free a table of virCapsGuestMachinePtr + */ +void +virCapabilitiesFreeMachines(virCapsGuestMachinePtr *machines, + int nmachines) +{ + int i; + if (!machines) + return; + for (i = 0; i < nmachines && machines[i]; i++) { + virCapabilitiesFreeGuestMachine(machines[i]); + machines[i] = NULL; + } + VIR_FREE(machines); +} /** * virCapabilitiesAddGuest: @@ -276,10 +333,9 @@ virCapabilitiesAddGuest(virCapsPtr caps, const char *emulator, const char *loader, int nmachines, - const char *const *machines) + virCapsGuestMachinePtr *machines) { virCapsGuestPtr guest; - int i; if (VIR_ALLOC(guest) < 0) goto no_memory; @@ -298,14 +354,8 @@ virCapabilitiesAddGuest(virCapsPtr caps, (guest->arch.defaultInfo.loader = strdup(loader)) == NULL) goto no_memory; if (nmachines) { - if (VIR_ALLOC_N(guest->arch.defaultInfo.machines, - nmachines) < 0) - goto no_memory; - for (i = 0 ; i < nmachines ; i++) { - if ((guest->arch.defaultInfo.machines[i] = strdup(machines[i])) == NULL) - goto no_memory; - guest->arch.defaultInfo.nmachines++; - } + guest->arch.defaultInfo.nmachines = nmachines; + guest->arch.defaultInfo.machines = machines; } if (VIR_REALLOC_N(caps->guests, @@ -340,10 +390,9 @@ virCapabilitiesAddGuestDomain(virCapsGuestPtr guest, const char *emulator, const char *loader, int nmachines, - const char *const *machines) + virCapsGuestMachinePtr *machines) { virCapsGuestDomainPtr dom; - int i; if (VIR_ALLOC(dom) < 0) goto no_memory; @@ -358,13 +407,8 @@ virCapabilitiesAddGuestDomain(virCapsGuestPtr guest, (dom->info.loader = strdup(loader)) == NULL) goto no_memory; if (nmachines) { - if (VIR_ALLOC_N(dom->info.machines, nmachines) < 0) - goto no_memory; - for (i = 0 ; i < nmachines ; i++) { - if ((dom->info.machines[i] = strdup(machines[i])) == NULL) - goto no_memory; - dom->info.nmachines++; - } + dom->info.nmachines = nmachines; + dom->info.machines = machines; } if (VIR_REALLOC_N(guest->arch.domains, @@ -517,7 +561,7 @@ virCapabilitiesDefaultGuestMachine(virCapsPtr caps, if (STREQ(caps->guests[i]->ostype, ostype) && STREQ(caps->guests[i]->arch.name, arch) && caps->guests[i]->arch.defaultInfo.nmachines) - return caps->guests[i]->arch.defaultInfo.machines[0]; + return caps->guests[i]->arch.defaultInfo.machines[0]->name; } return NULL; } @@ -649,7 +693,7 @@ virCapabilitiesFormatXML(virCapsPtr caps) for (j = 0 ; j < caps->guests[i]->arch.defaultInfo.nmachines ; j++) { virBufferVSprintf(&xml, " <machine>%s</machine>\n", - caps->guests[i]->arch.defaultInfo.machines[j]); + caps->guests[i]->arch.defaultInfo.machines[j]->name); } for (j = 0 ; j < caps->guests[i]->arch.ndomains ; j++) { @@ -664,7 +708,7 @@ virCapabilitiesFormatXML(virCapsPtr caps) for (k = 0 ; k < caps->guests[i]->arch.domains[j]->info.nmachines ; k++) { virBufferVSprintf(&xml, " <machine>%s</machine>\n", - caps->guests[i]->arch.domains[j]->info.machines[k]); + caps->guests[i]->arch.domains[j]->info.machines[k]->name); } virBufferAddLit(&xml, " </domain>\n"); } diff --git a/src/capabilities.h b/src/capabilities.h index c3ca89a..aab084c 100644 --- a/src/capabilities.h +++ b/src/capabilities.h @@ -35,13 +35,20 @@ struct _virCapsGuestFeature { int toggle; }; +typedef struct _virCapsGuestMachine virCapsGuestMachine; +typedef virCapsGuestMachine *virCapsGuestMachinePtr; +struct _virCapsGuestMachine { + char *name; + char *canonical; +}; + typedef struct _virCapsGuestDomainInfo virCapsGuestDomainInfo; typedef virCapsGuestDomainInfo *virCapsGuestDomainInfoPtr; struct _virCapsGuestDomainInfo { char *emulator; char *loader; int nmachines; - char **machines; + virCapsGuestMachinePtr *machines; }; typedef struct _virCapsGuestDomain virCapsGuestDomain; @@ -152,6 +159,13 @@ virCapabilitiesAddHostNUMACell(virCapsPtr caps, +extern virCapsGuestMachinePtr * +virCapabilitiesAllocMachines(const char *const *names, + int nnames); +extern void +virCapabilitiesFreeMachines(virCapsGuestMachinePtr *machines, + int nmachines); + extern virCapsGuestPtr virCapabilitiesAddGuest(virCapsPtr caps, const char *ostype, @@ -160,7 +174,7 @@ virCapabilitiesAddGuest(virCapsPtr caps, const char *emulator, const char *loader, int nmachines, - const char *const *machines); + virCapsGuestMachinePtr *machines); extern virCapsGuestDomainPtr virCapabilitiesAddGuestDomain(virCapsGuestPtr guest, @@ -168,7 +182,7 @@ virCapabilitiesAddGuestDomain(virCapsGuestPtr guest, const char *emulator, const char *loader, int nmachines, - const char *const *machines); + virCapsGuestMachinePtr *machines); extern virCapsGuestFeaturePtr virCapabilitiesAddGuestFeature(virCapsGuestPtr guest, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7a62b67..8611e24 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -30,6 +30,8 @@ virCapabilitiesSetMacPrefix; virCapabilitiesGenerateMac; virCapabilitiesSetEmulatorRequired; virCapabilitiesIsEmulatorRequired; +virCapabilitiesAllocMachines; +virCapabilitiesFreeMachines; # conf.h diff --git a/src/qemu_conf.c b/src/qemu_conf.c index bdbeff2..022a7a9 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -275,17 +275,17 @@ static const struct qemu_arch_info const arch_info_xen[] = { */ static int qemudParseMachineTypesStr(const char *output, - char ***machines, + virCapsGuestMachinePtr **machines, int *nmachines) { const char *p = output; const char *next; - char **list = NULL; - int i, nitems = 0; + virCapsGuestMachinePtr *list = NULL; + int nitems = 0; do { const char *t; - char *machine; + virCapsGuestMachinePtr machine; if ((next = strchr(p, '\n'))) ++next; @@ -296,10 +296,16 @@ qemudParseMachineTypesStr(const char *output, if (!(t = strchr(p, ' ')) || (next && t >= next)) continue; - if (!(machine = strndup(p, t - p))) + if (VIR_ALLOC(machine) < 0) goto error; + if (!(machine->name = strndup(p, t - p))) { + VIR_FREE(machine); + goto error; + } + if (VIR_REALLOC_N(list, nitems + 1) < 0) { + VIR_FREE(machine->name); VIR_FREE(machine); goto error; } @@ -321,15 +327,13 @@ qemudParseMachineTypesStr(const char *output, return 0; error: - for (i = 0; i < nitems; i++) - VIR_FREE(list[i]); - VIR_FREE(list); + virCapabilitiesFreeMachines(list, nitems); return -1; } static int qemudProbeMachineTypes(const char *binary, - char ***machines, + virCapsGuestMachinePtr **machines, int *nmachines) { const char *const qemuarg[] = { binary, "-M", "?", NULL }; @@ -393,7 +397,7 @@ qemudCapsInitGuest(virCapsPtr caps, int haskqemu = 0; const char *kvmbin = NULL; const char *binary = NULL; - char **machines = NULL; + virCapsGuestMachinePtr *machines = NULL; int nmachines = 0; /* Check for existance of base emulator, or alternate base @@ -434,12 +438,18 @@ qemudCapsInitGuest(virCapsPtr caps, return 0; if (info->machine) { - char *machine; + virCapsGuestMachinePtr machine; + + if (VIR_ALLOC(machine) < 0) + return -1; - if (!(machine = strdup(info->machine))) + if (!(machine->name = strdup(info->machine))) { + VIR_FREE(machine); return -1; + } if (VIR_ALLOC_N(machines, nmachines) < 0) { + VIR_FREE(machine->name); VIR_FREE(machine); return -1; } @@ -459,17 +469,15 @@ qemudCapsInitGuest(virCapsPtr caps, binary, NULL, nmachines, - (const char *const *)machines)) == NULL) { - for (i = 0; i < nmachines; i++) + machines)) == NULL) { + for (i = 0; i < nmachines; i++) { + VIR_FREE(machines[i]->name); VIR_FREE(machines[i]); + } VIR_FREE(machines); return -1; } - for (i = 0; i < nmachines; i++) - VIR_FREE(machines[i]); - VIR_FREE(machines); - if (hvm) { if (virCapabilitiesAddGuestDomain(guest, "qemu", diff --git a/src/xen_internal.c b/src/xen_internal.c index cc5a8f9..37c8736 100644 --- a/src/xen_internal.c +++ b/src/xen_internal.c @@ -2202,7 +2202,11 @@ xenHypervisorBuildCapabilities(virConnectPtr conn, for (i = 0; i < nr_guest_archs; ++i) { virCapsGuestPtr guest; - char const *const machines[] = {guest_archs[i].hvm ? "xenfv" : "xenpv"}; + char const *const xen_machines[] = {guest_archs[i].hvm ? "xenfv" : "xenpv"}; + virCapsGuestMachinePtr *machines; + + if ((machines = virCapabiltiesAllocMachines(xen_machines, 1)) == NULL) + goto no_memory; if ((guest = virCapabilitiesAddGuest(caps, guest_archs[i].hvm ? "hvm" : "xen", @@ -2215,8 +2219,12 @@ xenHypervisorBuildCapabilities(virConnectPtr conn, "/usr/lib/xen/boot/hvmloader" : NULL), 1, - machines)) == NULL) + machines)) == NULL) { + virCapabilitiesFreeMachines(machines, 1); goto no_memory; + } + + virCapabilitiesFreeMachines(machines, 1); if (virCapabilitiesAddGuestDomain(guest, "xen", diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 176c533..58707e1 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -9,6 +9,8 @@ virCapsPtr testQemuCapsInit(void) { struct utsname utsname; virCapsPtr caps; virCapsGuestPtr guest; + virCapsGuestMachinePtr *machines; + int nmachines; static const char *const x86_machines[] = { "pc", "isapc" }; @@ -21,10 +23,16 @@ virCapsPtr testQemuCapsInit(void) { 0, 0)) == NULL) return NULL; + nmachines = 2; + if ((machines = virCapabilitiesAllocMachines(x86_machines, nmachines)) == NULL) + goto cleanup; + if ((guest = virCapabilitiesAddGuest(caps, "hvm", "i686", 32, "/usr/bin/qemu", NULL, - 2, x86_machines)) == NULL) + nmachines, machines)) == NULL) goto cleanup; + machines = NULL; + if (virCapabilitiesAddGuestDomain(guest, "qemu", NULL, @@ -33,10 +41,16 @@ virCapsPtr testQemuCapsInit(void) { NULL) == NULL) goto cleanup; + nmachines = 2; + if ((machines = virCapabilitiesAllocMachines(x86_machines, nmachines)) == NULL) + goto cleanup; + if ((guest = virCapabilitiesAddGuest(caps, "hvm", "x86_64", 64, "/usr/bin/qemu-system-x86_64", NULL, - 2, x86_machines)) == NULL) + nmachines, machines)) == NULL) goto cleanup; + machines = NULL; + if (virCapabilitiesAddGuestDomain(guest, "qemu", NULL, @@ -52,10 +66,16 @@ virCapsPtr testQemuCapsInit(void) { NULL) == NULL) goto cleanup; + nmachines = 1; + if ((machines = virCapabilitiesAllocMachines(xen_machines, nmachines)) == NULL) + goto cleanup; + if ((guest = virCapabilitiesAddGuest(caps, "xen", "x86_64", 64, "/usr/bin/xenner", NULL, - 1, xen_machines)) == NULL) + 1, machines)) == NULL) goto cleanup; + machines = NULL; + if (virCapabilitiesAddGuestDomain(guest, "kvm", "/usr/bin/kvm", @@ -67,6 +87,7 @@ virCapsPtr testQemuCapsInit(void) { return caps; cleanup: + virCapabilitiesFreeMachines(machines, nmachines); virCapabilitiesFree(caps); return NULL; } -- 1.6.2.5
On Thu, Jul 23, 2009 at 06:34:41PM +0100, Mark McLoughlin wrote:
A subsequent commit will add a "canonical" field to this structure, this patch basically just prepares the way for that.
The new type is added, along with virCapabilitiesAlloc/FreeMachines() helpers and a whole bunch of code to make the transition.
One quirk is that virCapabilitiesAddGuestDomain() and virCapabilitiesAddGuest() take ownership of the machine list rather than duping it. This makes sense to avoid needless copying.
* src/capabilities.h: add the virCapsGuestMachine struct and use it in virCapsGuestDomainInfo, add prototypes for new functions and update the AddGuest() prototypes
* src/capabilities.c: add code for allocating and freeing the new type, change the machines parameter to AddGuest() etc.
* src/libvirt_private.syms: export the new helpers
* src/qemu_conf.c: update all the machine type code to use the new struct
* src/xen_internal.c: ditto
* tests/testutilsqemu.c: ditto [...] +virCapsGuestMachinePtr * +virCapabilitiesAllocMachines(const char *const *names, int nnames) +{ + virCapsGuestMachinePtr *machines; + int i; + + if (VIR_ALLOC_N(machines, nnames) < 0) + return NULL; + + for (i = 0; i < nnames; i++) { + if (VIR_ALLOC(machines[i]) < 0 || + !(machines[i]->name = strdup(names[i]))) {
we should emit an OOM error here
@@ -296,10 +296,16 @@ qemudParseMachineTypesStr(const char *output, if (!(t = strchr(p, ' ')) || (next && t >= next)) continue;
- if (!(machine = strndup(p, t - p))) + if (VIR_ALLOC(machine) < 0) goto error;
+ if (!(machine->name = strndup(p, t - p))) { + VIR_FREE(machine); + goto error; + } + if (VIR_REALLOC_N(list, nitems + 1) < 0) { + VIR_FREE(machine->name); VIR_FREE(machine); goto error; }
Same here the strndup failure paths should emit OOM errors
@@ -321,15 +327,13 @@ qemudParseMachineTypesStr(const char *output, return 0;
error: - for (i = 0; i < nitems; i++) - VIR_FREE(list[i]); - VIR_FREE(list); + virCapabilitiesFreeMachines(list, nitems); return -1; }
maybe a no_memory: label with the call would be the simplest, we use that in other places.
@@ -434,12 +438,18 @@ qemudCapsInitGuest(virCapsPtr caps, return 0;
if (info->machine) { - char *machine; + virCapsGuestMachinePtr machine; + + if (VIR_ALLOC(machine) < 0) + return -1;
- if (!(machine = strdup(info->machine))) + if (!(machine->name = strdup(info->machine))) { + VIR_FREE(machine); return -1; + }
if (VIR_ALLOC_N(machines, nmachines) < 0) { + VIR_FREE(machine->name); VIR_FREE(machine); return -1; }
similar ACK, but it would be better if those where cleaned up before commit. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Mon, 2009-07-27 at 15:36 +0200, Daniel Veillard wrote:
On Thu, Jul 23, 2009 at 06:34:41PM +0100, Mark McLoughlin wrote:
A subsequent commit will add a "canonical" field to this structure, this patch basically just prepares the way for that.
The new type is added, along with virCapabilitiesAlloc/FreeMachines() helpers and a whole bunch of code to make the transition.
One quirk is that virCapabilitiesAddGuestDomain() and virCapabilitiesAddGuest() take ownership of the machine list rather than duping it. This makes sense to avoid needless copying.
* src/capabilities.h: add the virCapsGuestMachine struct and use it in virCapsGuestDomainInfo, add prototypes for new functions and update the AddGuest() prototypes
* src/capabilities.c: add code for allocating and freeing the new type, change the machines parameter to AddGuest() etc.
* src/libvirt_private.syms: export the new helpers
* src/qemu_conf.c: update all the machine type code to use the new struct
* src/xen_internal.c: ditto
* tests/testutilsqemu.c: ditto [...] +virCapsGuestMachinePtr * +virCapabilitiesAllocMachines(const char *const *names, int nnames) +{ + virCapsGuestMachinePtr *machines; + int i; + + if (VIR_ALLOC_N(machines, nnames) < 0) + return NULL; + + for (i = 0; i < nnames; i++) { + if (VIR_ALLOC(machines[i]) < 0 || + !(machines[i]->name = strdup(names[i]))) {
we should emit an OOM error here
Caller should emit the OOM error if we return NULL, I think that's common for allocators.
@@ -296,10 +296,16 @@ qemudParseMachineTypesStr(const char *output, if (!(t = strchr(p, ' ')) || (next && t >= next)) continue;
- if (!(machine = strndup(p, t - p))) + if (VIR_ALLOC(machine) < 0) goto error;
+ if (!(machine->name = strndup(p, t - p))) { + VIR_FREE(machine); + goto error; + } + if (VIR_REALLOC_N(list, nitems + 1) < 0) { + VIR_FREE(machine->name); VIR_FREE(machine); goto error; }
Same here the strndup failure paths should emit OOM errors
@@ -321,15 +327,13 @@ qemudParseMachineTypesStr(const char *output, return 0;
error: - for (i = 0; i < nitems; i++) - VIR_FREE(list[i]); - VIR_FREE(list); + virCapabilitiesFreeMachines(list, nitems); return -1; }
maybe a no_memory: label with the call would be the simplest, we use that in other places.
Caller is supposed to set error on OOM, I've fixed qemudCanonicalizeMachineDirect() to do that See below - it's the caller of qemudCapsInit() which eventually sets the error
@@ -434,12 +438,18 @@ qemudCapsInitGuest(virCapsPtr caps, return 0;
if (info->machine) { - char *machine; + virCapsGuestMachinePtr machine; + + if (VIR_ALLOC(machine) < 0) + return -1;
- if (!(machine = strdup(info->machine))) + if (!(machine->name = strdup(info->machine))) { + VIR_FREE(machine); return -1; + }
if (VIR_ALLOC_N(machines, nmachines) < 0) { + VIR_FREE(machine->name); VIR_FREE(machine); return -1; }
similar
There's lots of other cases in this function we don't set an error on OOM because the caller eventually sets an error: if ((qemu_driver->caps = qemudCapsInit(NULL)) == NULL) goto out_of_memory; Cheers, Mark.
On Mon, Jul 27, 2009 at 03:05:57PM +0100, Mark McLoughlin wrote:
On Mon, 2009-07-27 at 15:36 +0200, Daniel Veillard wrote:
On Thu, Jul 23, 2009 at 06:34:41PM +0100, Mark McLoughlin wrote:
A subsequent commit will add a "canonical" field to this structure, this patch basically just prepares the way for that.
The new type is added, along with virCapabilitiesAlloc/FreeMachines() helpers and a whole bunch of code to make the transition.
One quirk is that virCapabilitiesAddGuestDomain() and virCapabilitiesAddGuest() take ownership of the machine list rather than duping it. This makes sense to avoid needless copying.
* src/capabilities.h: add the virCapsGuestMachine struct and use it in virCapsGuestDomainInfo, add prototypes for new functions and update the AddGuest() prototypes
* src/capabilities.c: add code for allocating and freeing the new type, change the machines parameter to AddGuest() etc.
* src/libvirt_private.syms: export the new helpers
* src/qemu_conf.c: update all the machine type code to use the new struct
* src/xen_internal.c: ditto
* tests/testutilsqemu.c: ditto [...] +virCapsGuestMachinePtr * +virCapabilitiesAllocMachines(const char *const *names, int nnames) +{ + virCapsGuestMachinePtr *machines; + int i; + + if (VIR_ALLOC_N(machines, nnames) < 0) + return NULL; + + for (i = 0; i < nnames; i++) { + if (VIR_ALLOC(machines[i]) < 0 || + !(machines[i]->name = strdup(names[i]))) {
we should emit an OOM error here
Caller should emit the OOM error if we return NULL, I think that's common for allocators.
@@ -296,10 +296,16 @@ qemudParseMachineTypesStr(const char *output, if (!(t = strchr(p, ' ')) || (next && t >= next)) continue;
- if (!(machine = strndup(p, t - p))) + if (VIR_ALLOC(machine) < 0) goto error;
+ if (!(machine->name = strndup(p, t - p))) { + VIR_FREE(machine); + goto error; + } + if (VIR_REALLOC_N(list, nitems + 1) < 0) { + VIR_FREE(machine->name); VIR_FREE(machine); goto error; }
Same here the strndup failure paths should emit OOM errors
@@ -321,15 +327,13 @@ qemudParseMachineTypesStr(const char *output, return 0;
error: - for (i = 0; i < nitems; i++) - VIR_FREE(list[i]); - VIR_FREE(list); + virCapabilitiesFreeMachines(list, nitems); return -1; }
maybe a no_memory: label with the call would be the simplest, we use that in other places.
Caller is supposed to set error on OOM, I've fixed qemudCanonicalizeMachineDirect() to do that
See below - it's the caller of qemudCapsInit() which eventually sets the error
@@ -434,12 +438,18 @@ qemudCapsInitGuest(virCapsPtr caps, return 0;
if (info->machine) { - char *machine; + virCapsGuestMachinePtr machine; + + if (VIR_ALLOC(machine) < 0) + return -1;
- if (!(machine = strdup(info->machine))) + if (!(machine->name = strdup(info->machine))) { + VIR_FREE(machine); return -1; + }
if (VIR_ALLOC_N(machines, nmachines) < 0) { + VIR_FREE(machine->name); VIR_FREE(machine); return -1; }
similar
There's lots of other cases in this function we don't set an error on OOM because the caller eventually sets an error:
if ((qemu_driver->caps = qemudCapsInit(NULL)) == NULL) goto out_of_memory;
Okay, thanks for the explanations, I lacked the context ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
In qemu-0.11 there is a 'pc-0.10' machine type which allows you to run guests with a machine which is compatible with the pc machine in qemu-0.10 - e.g. using the original PCI class for virtio-blk and virtio-console and disabling MSI support in virtio-net. The idea here is that we don't want to suprise guests by changing the hardware when qemu is updated. I've just posted some patches for qemu-0.11 which allows libvirt to canonicalize the 'pc' machine alias to the latest machine version. This patches makes us use that so that when a guest is configured to use the 'pc' machine type, we resolve that to 'pc-0.11' machine and save that in the guest XML. See also: https://fedoraproject.org/wiki/Features/KVM_Stable_Guest_ABI * src/qemu_conf.c: add qemudCanonicalizeMachine() to canonicalize the machine type according to the machine aliases in capabilities * src/qemu_driver.c: parse aliases in qemudParseMachineTypesStr() --- src/qemu_conf.c | 11 +++++++- src/qemu_driver.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 1 deletions(-) diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 022a7a9..6f89f33 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -271,7 +271,7 @@ static const struct qemu_arch_info const arch_info_xen[] = { /* Format is: - * <machine> <desc> [(default)] + * <machine> <desc> [(default)|(alias of <canonical>)] */ static int qemudParseMachineTypesStr(const char *output, @@ -319,6 +319,15 @@ qemudParseMachineTypesStr(const char *output, list[0] = machine; nitems++; } + + if ((t = strstr(p, "(alias of ")) && (!next || t < next)) { + p = t + strlen("(alias of "); + if (!(t = strchr(p, ')')) || (next && t >= next)) + continue; + + if (!(machine->canonical = strndup(p, t - p))) + goto error; + } } while ((p = next)); *machines = list; diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 3b56092..e2fa4d4 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -4056,6 +4056,77 @@ cleanup: return ret; } +static int +qemudCanonicalizeMachineFromInfo(virDomainDefPtr def, + virCapsGuestDomainInfoPtr info, + char **canonical) +{ + int i; + + *canonical = NULL; + + for (i = 0; i < info->nmachines; i++) { + virCapsGuestMachinePtr machine = info->machines[i]; + + if (!machine->canonical) + continue; + + if (strcmp(def->os.machine, machine->name) != 0) + continue; + + if (!(*canonical = strdup(machine->canonical))) + return -1; + + break; + } + + return 0; +} + +static int +qemudCanonicalizeMachine(virConnectPtr conn, virDomainDefPtr def) +{ + struct qemud_driver *driver = conn->privateData; + char *canonical = NULL; + int i; + + for (i = 0; i < driver->caps->nguests; i++) { + virCapsGuestPtr guest = driver->caps->guests[i]; + int j; + + for (j = 0; j < guest->arch.ndomains; j++) { + virCapsGuestDomainPtr dom = guest->arch.domains[j]; + + if (dom->info.emulator && + STREQ(dom->info.emulator, def->emulator)) { + if (qemudCanonicalizeMachineFromInfo(def, &dom->info, + &canonical) < 0) + return -1; + if (canonical) + goto out; + break; + } + } + + /* if we matched one of the domain's emulators, or if + * we match the default emulator + */ + if (j < guest->arch.ndomains || + (guest->arch.defaultInfo.emulator && + STREQ(guest->arch.defaultInfo.emulator, def->emulator))) { + if (qemudCanonicalizeMachineFromInfo(def, &guest->arch.defaultInfo, + &canonical) < 0) + return -1; + goto out; + } + } +out: + if (canonical) { + VIR_FREE(def->os.machine); + def->os.machine = canonical; + } + return 0; +} static virDomainPtr qemudDomainDefine(virConnectPtr conn, const char *xml) { struct qemud_driver *driver = conn->privateData; @@ -4102,6 +4173,9 @@ static virDomainPtr qemudDomainDefine(virConnectPtr conn, const char *xml) { } } + if (qemudCanonicalizeMachine(conn, def) < 0) + goto cleanup; + if (!(vm = virDomainAssignDef(conn, &driver->domains, def))) { -- 1.6.2.5
On Thu, Jul 23, 2009 at 06:34:42PM +0100, Mark McLoughlin wrote:
In qemu-0.11 there is a 'pc-0.10' machine type which allows you to run guests with a machine which is compatible with the pc machine in qemu-0.10 - e.g. using the original PCI class for virtio-blk and virtio-console and disabling MSI support in virtio-net. The idea here is that we don't want to suprise guests by changing the hardware when qemu is updated.
I've just posted some patches for qemu-0.11 which allows libvirt to canonicalize the 'pc' machine alias to the latest machine version.
This patches makes us use that so that when a guest is configured to use the 'pc' machine type, we resolve that to 'pc-0.11' machine and save that in the guest XML.
See also:
https://fedoraproject.org/wiki/Features/KVM_Stable_Guest_ABI
* src/qemu_conf.c: add qemudCanonicalizeMachine() to canonicalize the machine type according to the machine aliases in capabilities
[...]
/* Format is: - * <machine> <desc> [(default)] + * <machine> <desc> [(default)|(alias of <canonical>)] */ static int qemudParseMachineTypesStr(const char *output, @@ -319,6 +319,15 @@ qemudParseMachineTypesStr(const char *output, list[0] = machine; nitems++; } + + if ((t = strstr(p, "(alias of ")) && (!next || t < next)) { + p = t + strlen("(alias of "); + if (!(t = strchr(p, ')')) || (next && t >= next)) + continue; + + if (!(machine->canonical = strndup(p, t - p))) + goto error; + } } while ((p = next));
hum, if you get (alias of foo ) your canonical will end up being " foo " instead of "foo" maybe spaces should be stripped before and after the name (i.e. it would have been perfect in an XML attribute but well ...) [...]
+static int +qemudCanonicalizeMachineFromInfo(virDomainDefPtr def, + virCapsGuestDomainInfoPtr info, + char **canonical) +{ + int i; + + *canonical = NULL; + + for (i = 0; i < info->nmachines; i++) { + virCapsGuestMachinePtr machine = info->machines[i]; + + if (!machine->canonical) + continue; + + if (strcmp(def->os.machine, machine->name) != 0) + continue; + + if (!(*canonical = strdup(machine->canonical)))
virReportOOMError(NULL) please :-)
+ return -1; + + break;
ACK Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Mon, 2009-07-27 at 15:42 +0200, Daniel Veillard wrote:
On Thu, Jul 23, 2009 at 06:34:42PM +0100, Mark McLoughlin wrote:
In qemu-0.11 there is a 'pc-0.10' machine type which allows you to run guests with a machine which is compatible with the pc machine in qemu-0.10 - e.g. using the original PCI class for virtio-blk and virtio-console and disabling MSI support in virtio-net. The idea here is that we don't want to suprise guests by changing the hardware when qemu is updated.
I've just posted some patches for qemu-0.11 which allows libvirt to canonicalize the 'pc' machine alias to the latest machine version.
This patches makes us use that so that when a guest is configured to use the 'pc' machine type, we resolve that to 'pc-0.11' machine and save that in the guest XML.
See also:
https://fedoraproject.org/wiki/Features/KVM_Stable_Guest_ABI
* src/qemu_conf.c: add qemudCanonicalizeMachine() to canonicalize the machine type according to the machine aliases in capabilities
[...]
/* Format is: - * <machine> <desc> [(default)] + * <machine> <desc> [(default)|(alias of <canonical>)] */ static int qemudParseMachineTypesStr(const char *output, @@ -319,6 +319,15 @@ qemudParseMachineTypesStr(const char *output, list[0] = machine; nitems++; } + + if ((t = strstr(p, "(alias of ")) && (!next || t < next)) { + p = t + strlen("(alias of "); + if (!(t = strchr(p, ')')) || (next && t >= next)) + continue; + + if (!(machine->canonical = strndup(p, t - p))) + goto error; + } } while ((p = next));
hum, if you get (alias of foo ) your canonical will end up being " foo " instead of "foo" maybe spaces should be stripped before and after the name (i.e. it would have been perfect in an XML attribute but well ...)
If only we had something like g_strstrip() :-)
[...]
+static int +qemudCanonicalizeMachineFromInfo(virDomainDefPtr def, + virCapsGuestDomainInfoPtr info, + char **canonical) +{ + int i; + + *canonical = NULL; + + for (i = 0; i < info->nmachines; i++) { + virCapsGuestMachinePtr machine = info->machines[i]; + + if (!machine->canonical) + continue; + + if (strcmp(def->os.machine, machine->name) != 0) + continue; + + if (!(*canonical = strdup(machine->canonical)))
virReportOOMError(NULL) please :-)
Yep, thanks. Cheers, Mark.
On Mon, Jul 27, 2009 at 03:11:10PM +0100, Mark McLoughlin wrote:
On Mon, 2009-07-27 at 15:42 +0200, Daniel Veillard wrote:
On Thu, Jul 23, 2009 at 06:34:42PM +0100, Mark McLoughlin wrote:
In qemu-0.11 there is a 'pc-0.10' machine type which allows you to run guests with a machine which is compatible with the pc machine in qemu-0.10 - e.g. using the original PCI class for virtio-blk and virtio-console and disabling MSI support in virtio-net. The idea here is that we don't want to suprise guests by changing the hardware when qemu is updated.
I've just posted some patches for qemu-0.11 which allows libvirt to canonicalize the 'pc' machine alias to the latest machine version.
This patches makes us use that so that when a guest is configured to use the 'pc' machine type, we resolve that to 'pc-0.11' machine and save that in the guest XML.
See also:
https://fedoraproject.org/wiki/Features/KVM_Stable_Guest_ABI
* src/qemu_conf.c: add qemudCanonicalizeMachine() to canonicalize the machine type according to the machine aliases in capabilities
[...]
/* Format is: - * <machine> <desc> [(default)] + * <machine> <desc> [(default)|(alias of <canonical>)] */ static int qemudParseMachineTypesStr(const char *output, @@ -319,6 +319,15 @@ qemudParseMachineTypesStr(const char *output, list[0] = machine; nitems++; } + + if ((t = strstr(p, "(alias of ")) && (!next || t < next)) { + p = t + strlen("(alias of "); + if (!(t = strchr(p, ')')) || (next && t >= next)) + continue; + + if (!(machine->canonical = strndup(p, t - p))) + goto error; + } } while ((p = next));
hum, if you get (alias of foo ) your canonical will end up being " foo " instead of "foo" maybe spaces should be stripped before and after the name (i.e. it would have been perfect in an XML attribute but well ...)
If only we had something like g_strstrip() :-)
oh, I'm sure we have something like that in libxml2 but the name is probably even worse :-)
[...]
+static int +qemudCanonicalizeMachineFromInfo(virDomainDefPtr def, + virCapsGuestDomainInfoPtr info, + char **canonical) +{ + int i; + + *canonical = NULL; + + for (i = 0; i < info->nmachines; i++) { + virCapsGuestMachinePtr machine = info->machines[i]; + + if (!machine->canonical) + continue; + + if (strcmp(def->os.machine, machine->name) != 0) + continue; + + if (!(*canonical = strdup(machine->canonical)))
virReportOOMError(NULL) please :-)
Yep, thanks.
np :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Not all possible emulators are actually in the capabilities, so if we don't find the supplied emulator we should probe it directly for machine types. * src/qemu_driver.c: add qemudCanonicalizeMachineDirect() to directly probe an emulator for the canonical machine type --- src/qemu_conf.c | 2 +- src/qemu_conf.h | 4 ++++ src/qemu_driver.c | 30 ++++++++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 1 deletions(-) diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 6f89f33..8325bfa 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -340,7 +340,7 @@ error: return -1; } -static int +int qemudProbeMachineTypes(const char *binary, virCapsGuestMachinePtr **machines, int *nmachines) diff --git a/src/qemu_conf.h b/src/qemu_conf.h index 50d7c0a..379cac4 100644 --- a/src/qemu_conf.h +++ b/src/qemu_conf.h @@ -163,6 +163,10 @@ int qemuBuildNicStr (virConnectPtr conn, int qemuAssignNetNames (virDomainDefPtr def, virDomainNetDefPtr net); +int qemudProbeMachineTypes (const char *binary, + virCapsGuestMachinePtr **machines, + int *nmachines); + virDomainDefPtr qemuParseCommandLine(virConnectPtr conn, virCapsPtr caps, const char **progenv, diff --git a/src/qemu_driver.c b/src/qemu_driver.c index e2fa4d4..3e33e0d 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -4084,6 +4084,32 @@ qemudCanonicalizeMachineFromInfo(virDomainDefPtr def, } static int +qemudCanonicalizeMachineDirect(virDomainDefPtr def, char **canonical) +{ + virCapsGuestMachinePtr *machines = NULL; + int i, nmachines = 0; + + if (qemudProbeMachineTypes(def->emulator, &machines, &nmachines) < 0) + return -1; + + for (i = 0; i < nmachines; i++) { + if (!machines[i]->canonical) + continue; + + if (strcmp(def->os.machine, machines[i]->name) != 0) + continue; + + *canonical = machines[i]->canonical; + machines[i]->canonical = NULL; + break; + } + + virCapabilitiesFreeMachines(machines, nmachines); + + return 0; +} + +static int qemudCanonicalizeMachine(virConnectPtr conn, virDomainDefPtr def) { struct qemud_driver *driver = conn->privateData; @@ -4120,6 +4146,10 @@ qemudCanonicalizeMachine(virConnectPtr conn, virDomainDefPtr def) goto out; } } + + if (qemudCanonicalizeMachineDirect(def, &canonical) < 0) + return -1; + out: if (canonical) { VIR_FREE(def->os.machine); -- 1.6.2.5
On Thu, Jul 23, 2009 at 06:34:43PM +0100, Mark McLoughlin wrote:
Not all possible emulators are actually in the capabilities, so if we don't find the supplied emulator we should probe it directly for machine types.
ACK Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
e.g. <machine canonical='pc'>pc-0.11</machine> * src/capabilities.c: output the canonical machine names in the capabilities output, if available --- src/capabilities.c | 14 ++++++++++---- 1 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/capabilities.c b/src/capabilities.c index afe4d38..d186961 100644 --- a/src/capabilities.c +++ b/src/capabilities.c @@ -692,8 +692,11 @@ virCapabilitiesFormatXML(virCapsPtr caps) caps->guests[i]->arch.defaultInfo.loader); for (j = 0 ; j < caps->guests[i]->arch.defaultInfo.nmachines ; j++) { - virBufferVSprintf(&xml, " <machine>%s</machine>\n", - caps->guests[i]->arch.defaultInfo.machines[j]->name); + virCapsGuestMachinePtr machine = caps->guests[i]->arch.defaultInfo.machines[j]; + virBufferAddLit(&xml, " <machine"); + if (machine->canonical) + virBufferVSprintf(&xml, " canonical='%s'", machine->canonical); + virBufferVSprintf(&xml, ">%s</machine>\n", machine->name); } for (j = 0 ; j < caps->guests[i]->arch.ndomains ; j++) { @@ -707,8 +710,11 @@ virCapabilitiesFormatXML(virCapsPtr caps) caps->guests[i]->arch.domains[j]->info.loader); for (k = 0 ; k < caps->guests[i]->arch.domains[j]->info.nmachines ; k++) { - virBufferVSprintf(&xml, " <machine>%s</machine>\n", - caps->guests[i]->arch.domains[j]->info.machines[k]->name); + virCapsGuestMachinePtr machine = caps->guests[i]->arch.domains[j]->info.machines[k]; + virBufferAddLit(&xml, " <machine"); + if (machine->canonical) + virBufferVSprintf(&xml, " canonical='%s'", machine->canonical); + virBufferVSprintf(&xml, ">%s</machine>\n", machine->name); } virBufferAddLit(&xml, " </domain>\n"); } -- 1.6.2.5
On Thu, Jul 23, 2009 at 06:34:44PM +0100, Mark McLoughlin wrote:
e.g. <machine canonical='pc'>pc-0.11</machine>
* src/capabilities.c: output the canonical machine names in the capabilities output, if available
ACK, but this is missing the rng extention and the documentation for the extension, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Mon, 2009-07-27 at 15:45 +0200, Daniel Veillard wrote:
On Thu, Jul 23, 2009 at 06:34:44PM +0100, Mark McLoughlin wrote:
e.g. <machine canonical='pc'>pc-0.11</machine>
* src/capabilities.c: output the canonical machine names in the capabilities output, if available
ACK, but this is missing the rng extention and the documentation for the extension,
Right, patch below to add it to the rng, but the capabilities docs needs a good bit of work before adding docs on this little piece wouldn't seem so much out of date - e.g. they're mostly about xen and not very specific. Cheers, Mark. diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng index f085e55..775bbdb 100644 --- a/docs/schemas/capability.rng +++ b/docs/schemas/capability.rng @@ -196,6 +196,11 @@ <define name='machine'> <element name='machine'> + <optional> + <attribute name='canonical'> + <text/> + </attribute> + </optional> <text/> </element> </define>
On Mon, Jul 27, 2009 at 03:30:01PM +0100, Mark McLoughlin wrote:
On Mon, 2009-07-27 at 15:45 +0200, Daniel Veillard wrote:
On Thu, Jul 23, 2009 at 06:34:44PM +0100, Mark McLoughlin wrote:
e.g. <machine canonical='pc'>pc-0.11</machine>
* src/capabilities.c: output the canonical machine names in the capabilities output, if available
ACK, but this is missing the rng extention and the documentation for the extension,
Right, patch below to add it to the rng, but the capabilities docs needs a good bit of work before adding docs on this little piece wouldn't seem so much out of date - e.g. they're mostly about xen and not very specific.
okay
diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng index f085e55..775bbdb 100644 --- a/docs/schemas/capability.rng +++ b/docs/schemas/capability.rng @@ -196,6 +196,11 @@
<define name='machine'> <element name='machine'> + <optional> + <attribute name='canonical'> + <text/> + </attribute> + </optional> <text/> </element> </define>
I suppose QEmu doesn't define a syntax for allowed machine names :-) ACK Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (4)
-
Cole Robinson -
Daniel P. Berrange -
Daniel Veillard -
Mark McLoughlin