On Tue, Dec 11, 2012 at 14:53:39 +0000, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> Convert the host capabilities and domain config structs to
> use the virArch datatype. Update the parsers and all drivers
> to take account of datatype change
...
> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> index a8ee2cf..97d1b80 100644
> --- a/src/conf/capabilities.c
> +++ b/src/conf/capabilities.c
...
> @@ -556,12 +546,12 @@ virCapabilitiesSupportsGuestOSType(virCapsPtr caps,
> extern int
> virCapabilitiesSupportsGuestOSTypeArch(virCapsPtr caps,
> const char *ostype,
> - const char *arch)
> + virArch arch)
> {
> int i;
> for (i = 0 ; i < caps->nguests ; i++) {
> if (STREQ(caps->guests[i]->ostype, ostype) &&
> - STREQ(caps->guests[i]->arch.name, arch))
> + (caps->guests[i]->arch.id == arch))
Unneeded ().
> return 1;
> }
> return 0;
> @@ -576,28 +566,35 @@ virCapabilitiesSupportsGuestOSTypeArch(virCapsPtr caps,
> * Returns the first architecture able to run the
> * requested operating system type
> */
> -extern const char *
> +extern virArch
> virCapabilitiesDefaultGuestArch(virCapsPtr caps,
> const char *ostype,
> const char *domain)
> {
> int i, j;
> - const char *arch = NULL;
> +
> + /* First try to find one matching host arch */
> for (i = 0 ; i < caps->nguests ; i++) {
> if (STREQ(caps->guests[i]->ostype, ostype)) {
> for (j = 0 ; j < caps->guests[i]->arch.ndomains ; j++) {
> - if (STREQ(caps->guests[i]->arch.domains[j]->type, domain)) {
> - /* Use the first match... */
> - if (!arch)
> - arch = caps->guests[i]->arch.name;
> - /* ...unless we can match the host's architecture. */
> - if (STREQ(caps->guests[i]->arch.name, caps->host.arch))
> - return caps->guests[i]->arch.name;
> - }
> + if (STREQ(caps->guests[i]->arch.domains[j]->type, domain) &&
> + (caps->guests[i]->arch.id == caps->host.arch))
You've started to love these extra () recently :-)
> + return caps->guests[i]->arch.id;
> }
> }
> }
> - return arch;
> +
> + /* Otherwise find the first match */
> + for (i = 0 ; i < caps->nguests ; i++) {
> + if (STREQ(caps->guests[i]->ostype, ostype)) {
> + for (j = 0 ; j < caps->guests[i]->arch.ndomains ; j++) {
> + if (STREQ(caps->guests[i]->arch.domains[j]->type, domain))
> + return caps->guests[i]->arch.id;
> + }
> + }
> + }
> +
I think both the original and the new version are equally readable, but
if you think this new version is better, I'm fine with it.
> + return VIR_ARCH_I686;
However, this should definitely return VIR_ARCH_NONE. The original
version would return NULL in case of no match.
> }
>
> /**
...
> @@ -623,11 +620,12 @@ virCapabilitiesDefaultGuestMachine(virCapsPtr caps,
> virCapsGuestPtr guest = caps->guests[i];
> int j;
>
> - if (!STREQ(guest->ostype, ostype) || !STREQ(guest->arch.name, arch))
> + if (!STREQ(guest->ostype, ostype) ||
> + (guest->arch.id != arch))
()
> continue;
>
> for (j = 0; j < guest->arch.ndomains; j++) {
> - virCapsGuestDomainPtr dom= guest->arch.domains[j];
> + virCapsGuestDomainPtr dom = guest->arch.domains[j];
>
> if (!STREQ(dom->type, domain))
> continue;
> @@ -659,14 +657,14 @@ virCapabilitiesDefaultGuestMachine(virCapsPtr caps,
> extern const char *
> virCapabilitiesDefaultGuestEmulator(virCapsPtr caps,
> const char *ostype,
> - const char *arch,
> + virArch arch,
> const char *domain)
> {
> int i, j;
> for (i = 0 ; i < caps->nguests ; i++) {
> char *emulator;
> if (STREQ(caps->guests[i]->ostype, ostype) &&
> - STREQ(caps->guests[i]->arch.name, arch)) {
> + (caps->guests[i]->arch.id == arch)) {
()
> emulator = caps->guests[i]->arch.defaultInfo.emulator;
> for (j = 0 ; j < caps->guests[i]->arch.ndomains ; j++) {
> if (STREQ(caps->guests[i]->arch.domains[j]->type, domain)) {
...
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6aa5f79..1f978db 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
...
> @@ -9412,12 +9411,20 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
> goto error;
> }
>
> - def->os.arch = virXPathString("string(./os/type[1]/@arch)", ctxt);
> - if (def->os.arch) {
> + tmp = virXPathString("string(./os/type[1]/@arch)", ctxt);
> + if (tmp) {
> + virArch arch = virArchFromString(tmp);
> + if (!arch) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Unknown architecture %s"),
> + tmp);
> + goto error;
> + }
> +
Memory leak, you need to call VIR_FREE(tmp) here.
> if (!virCapabilitiesSupportsGuestArch(caps, def->os.arch)) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("No guest options available for arch '%s'"),
> - def->os.arch);
> + virArchToString(def->os.arch));
> goto error;
> }
>
> @@ -9426,20 +9433,17 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
> def->os.arch)) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("No os type '%s' available for arch '%s'"),
> - def->os.type, def->os.arch);
> + def->os.type, virArchToString(def->os.arch));
> goto error;
> }
> } else {
> - const char *defaultArch = virCapabilitiesDefaultGuestArch(caps, def->os.type, virDomainVirtTypeToString(def->virtType));
> - if (defaultArch == NULL) {
> + def->os.arch = virCapabilitiesDefaultGuestArch(caps, def->os.type, virDomainVirtTypeToString(def->virtType));
Make this line shorter while changing it.
> + if (!def->os.arch) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("no supported architecture for os type '%s'"),
> def->os.type);
> goto error;
> }
> - if (!(def->os.arch = strdup(defaultArch))) {
> - goto no_memory;
> - }
> }
>
> def->os.machine = virXPathString("string(./os/type[1]/@machine)", ctxt);
> @@ -10050,7 +10054,8 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
>
> def->memballoon = memballoon;
> VIR_FREE(nodes);
> - } else if (!STREQ(def->os.arch,"s390x")) {
> + } else if ((def->os.arch != VIR_ARCH_S390) &&
> + (def->os.arch != VIR_ARCH_S390X)) {
Too many extra (), I won't report them anymore :-)
> /* TODO: currently no balloon support on s390 -> no default balloon */
> if (def->virtType == VIR_DOMAIN_VIRT_XEN ||
> def->virtType == VIR_DOMAIN_VIRT_QEMU ||
...
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 302f81c..c02cb19 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
...
> @@ -334,14 +333,10 @@ libxlDoNodeGetInfo(libxlDriverPrivatePtr driver, virNodeInfoPtr info)
> return -1;
> }
>
> - uname(&utsname);
> - if (virStrncpy(info->model,
> - utsname.machine,
> - strlen(utsname.machine),
> - sizeof(info->model)) == NULL) {
> + if (virStrcpyStatic(nodeinfo->model, virArchToString(hostarch)) == NULL) {
This wouldn't even compile; s/nodeinfo/info/
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("machine type %s too big for destination"),
> - utsname.machine);
> + virArchToString(hostarch));
> return -1;
> }
>
...
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 01a1b98..c9146c4 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
...
> @@ -899,11 +882,14 @@ virCapsPtr qemuCapsInit(qemuCapsCachePtr cache)
> virCapabilitiesAddHostMigrateTransport(caps,
> "tcp");
>
> - /* First the pure HVM guests */
> - for (i = 0 ; i < ARRAY_CARDINALITY(arches) ; i++)
> + /* QEMU can support pretty much every arch that exists,
> + * so just probe for them all - we gracefully fail
> + * if a qemu-system-$ARCH bvinary can't be found
s/bvinary/binary/
> + */
> + for (i = 0 ; i < VIR_ARCH_LAST ; i++)
> if (qemuCapsInitGuest(caps, cache,
> - utsname.machine,
> - arches[i]) < 0)
> + virArchFromHost(),
> + i) < 0)
> goto error;
>
> /* QEMU Requires an emulator in the XML */
...
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 6968f74..267dce7 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
...
> @@ -8338,16 +8337,17 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
> else
> path = strstr(def->emulator, "qemu");
> if (def->virtType == VIR_DOMAIN_VIRT_KVM)
> - def->os.arch = strdup(caps->host.cpu->arch);
> + def->os.arch = caps->host.arch;
> else if (path &&
> - STRPREFIX(path, "qemu-system-"))
> - def->os.arch = strdup(path + strlen("qemu-system-"));
> - else
> - def->os.arch = strdup("i686");
> + STRPREFIX(path, "qemu-system-")) {
> + def->os.arch = virArchFromString(path + strlen("qemu-system-"));
> + } else
> + def->os.arch = VIR_ARCH_I686;
> if (!def->os.arch)
> goto no_memory;
No need for the extra {}, however, if you want to add them, add them to
all branches of this if statement.
>
> - if (STREQ(def->os.arch, "i686")||STREQ(def->os.arch, "x86_64"))
> + if ((def->os.arch == VIR_ARCH_I686) ||
> + (def->os.arch == VIR_ARCH_X86_64))
> def->features |= (1 << VIR_DOMAIN_FEATURE_ACPI)
> /*| (1 << VIR_DOMAIN_FEATURE_APIC)*/;
> #define WANT_VALUE() \
...
> diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c
> index 23b3037..1b04b4c 100644
> --- a/src/xenxs/xen_xm.c
> +++ b/src/xenxs/xen_xm.c
...
> @@ -290,15 +290,13 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
> if (!(def->os.type = strdup(hvm ? "hvm" : "xen")))
> goto no_memory;
>
> - defaultArch = virCapabilitiesDefaultGuestArch(caps, def->os.type, virDomainVirtTypeToString(def->virtType));
> - if (defaultArch == NULL) {
> + def->os.arch = virCapabilitiesDefaultGuestArch(caps, def->os.type, virDomainVirtTypeToString(def->virtType));
Line too long.
> + if (!def->os.arch) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("no supported architecture for os type '%s'"),
> def->os.type);
> goto cleanup;
> }
> - if (!(def->os.arch = strdup(defaultArch)))
> - goto no_memory;
>
> defaultMachine = virCapabilitiesDefaultGuestMachine(caps,
> def->os.type,
ACK with the small issues fixed.
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list