Re: [libvirt] [PATCH V5 08/12] src/xenxs: Refactor code formating CPU features config

Kiarie Kahurani wrote: > introduce functions > xenFormatXMCPUFeatures(virConfPtr conf, ......); > which formats CPU features config instead > > Signed-off-by: Kiarie Kahurani <davidkiarie4(a)gmail.com&gt; > --- > src/xenxs/xen_xm.c | 118 ++++++++++++--------- > tests/xmconfigdata/test-escape-paths.cfg | 6 +- > tests/xmconfigdata/test-fullvirt-force-hpet.cfg | 6 +- > tests/xmconfigdata/test-fullvirt-force-nohpet.cfg | 6 +- > tests/xmconfigdata/test-fullvirt-localtime.cfg | 6 +- > tests/xmconfigdata/test-fullvirt-net-ioemu.cfg | 6 +- > tests/xmconfigdata/test-fullvirt-net-netfront.cfg | 6 +- > tests/xmconfigdata/test-fullvirt-new-cdrom.cfg | 6 +- > tests/xmconfigdata/test-fullvirt-old-cdrom.cfg | 6 +- > tests/xmconfigdata/test-fullvirt-parallel-tcp.cfg | 6 +- > .../test-fullvirt-serial-dev-2-ports.cfg | 6 +- > .../test-fullvirt-serial-dev-2nd-port.cfg | 6 +- > tests/xmconfigdata/test-fullvirt-serial-file.cfg | 6 +- > tests/xmconfigdata/test-fullvirt-serial-null.cfg | 6 +- > tests/xmconfigdata/test-fullvirt-serial-pipe.cfg | 6 +- > tests/xmconfigdata/test-fullvirt-serial-pty.cfg | 6 +- > tests/xmconfigdata/test-fullvirt-serial-stdio.cfg | 6 +- > .../test-fullvirt-serial-tcp-telnet.cfg | 6 +- > tests/xmconfigdata/test-fullvirt-serial-tcp.cfg | 6 +- > tests/xmconfigdata/test-fullvirt-serial-udp.cfg | 6 +- > tests/xmconfigdata/test-fullvirt-serial-unix.cfg | 6 +- > tests/xmconfigdata/test-fullvirt-sound.cfg | 6 +- > tests/xmconfigdata/test-fullvirt-usbmouse.cfg | 6 +- > tests/xmconfigdata/test-fullvirt-usbtablet.cfg | 6 +- > tests/xmconfigdata/test-fullvirt-utc.cfg | 6 +- > tests/xmconfigdata/test-no-source-cdrom.cfg | 6 +- > tests/xmconfigdata/test-pci-devs.cfg | 6 +- > 27 files changed, 146 insertions(+), 128 deletions(-) > > diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c > index 2c381f8..a856698 100644 > --- a/src/xenxs/xen_xm.c > +++ b/src/xenxs/xen_xm.c > @@ -1950,6 +1950,73 @@ xenFormatXMDomainDisks(virConfPtr conf, virDomainDefPtr def, > virConfFreeValue(diskVal); > return -1; > } > + > + > +static int > +xenFormatXMCPUFeatures(virConfPtr conf, virDomainDefPtr def, > + int xendConfigVersion) > +{ > + char *cpus = NULL; > + size_t i; > + > + if (xenXMConfigSetInt(conf, "vcpus", def->maxvcpus) < 0) > + return -1; > + /* Computing the vcpu_avail bitmask works because MAX_VIRT_CPUS is > + either 32, or 64 on a platform where long is big enough. */ > + if (def->vcpus < def->maxvcpus && > + xenXMConfigSetInt(conf, "vcpu_avail", (1UL << def->vcpus) - 1) < 0) > + return -1; > + > + if ((def->cpumask != NULL) && > + ((cpus = virBitmapFormat(def->cpumask)) == NULL)) { > + return -1; > + } > + > + if (cpus && > + xenXMConfigSetString(conf, "cpus", cpus) < 0) > + return -1; > + > + VIR_FREE(cpus); > I'd consider these to be allocation-related, as opposed to features. I've changed the patch a bit to introduce xenFormatXMCPUAllocation and xenFormatXMCPUFeatures. > + if (STREQ(def->os.type, "hvm")) { > + if (xenXMConfigSetInt(conf, "pae", > + (def->features[VIR_DOMAIN_FEATURE_PAE] == > + VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) > + return -1; > + > + if (xenXMConfigSetInt(conf, "acpi", > + (def->features[VIR_DOMAIN_FEATURE_ACPI] == > + VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) > + return -1; > + > + if (xenXMConfigSetInt(conf, "apic", > + (def->features[VIR_DOMAIN_FEATURE_APIC] == > + VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) > + return -1; > + > + if (xendConfigVersion >= XEND_CONFIG_VERSION_3_0_4) { > + if (xenXMConfigSetInt(conf, "hap", > + (def->features[VIR_DOMAIN_FEATURE_HAP] == > + VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) > + return -1; > + > + if (xenXMConfigSetInt(conf, "viridian", > + (def->features[VIR_DOMAIN_FEATURE_VIRIDIAN] == > + VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) > + return -1; > + } > + > + for (i = 0; i < def->clock.ntimers; i++) { > + if (def->clock.timers[i]->name == VIR_DOMAIN_TIMER_NAME_HPET && > + def->clock.timers[i]->present != -1 && > + xenXMConfigSetInt(conf, "hpet", def->clock.timers[i]->present) < 0) > + return -1; > + } > + } > + > + return 0; > +} > + > + > /* Computing the vcpu_avail bitmask works because MAX_VIRT_CPUS is > either 32, or 64 on a platform where long is big enough. */ > verify(MAX_VIRT_CPUS <= sizeof(1UL) * CHAR_BIT); > @@ -1974,24 +2041,9 @@ xenFormatXM(virConnectPtr conn, > if (xenFormatXMMem(conf, def) < 0) > goto cleanup; > > - if (xenXMConfigSetInt(conf, "vcpus", def->maxvcpus) < 0) > - goto cleanup; > - /* Computing the vcpu_avail bitmask works because MAX_VIRT_CPUS is > - either 32, or 64 on a platform where long is big enough. */ > - if (def->vcpus < def->maxvcpus && > - xenXMConfigSetInt(conf, "vcpu_avail", (1UL << def->vcpus) - 1) < 0) > + if (xenFormatXMCPUFeatures(conf, def, xendConfigVersion) < 0) > With my changes, allocations can be formatted here > goto cleanup; > > - if ((def->cpumask != NULL) && > - ((cpus = virBitmapFormat(def->cpumask)) == NULL)) { > - goto cleanup; > - } > - > - if (cpus && > - xenXMConfigSetString(conf, "cpus", cpus) < 0) > - goto cleanup; > - VIR_FREE(cpus); > - > hvm = STREQ(def->os.type, "hvm") ? 1 : 0; > > if (hvm) { > @@ -2030,40 +2082,6 @@ xenFormatXM(virConnectPtr conn, > if (xenXMConfigSetString(conf, "boot", boot) < 0) > goto cleanup; > > - if (xenXMConfigSetInt(conf, "pae", > - (def->features[VIR_DOMAIN_FEATURE_PAE] == > - VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) > - goto cleanup; > - > - if (xenXMConfigSetInt(conf, "acpi", > - (def->features[VIR_DOMAIN_FEATURE_ACPI] == > - VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) > - goto cleanup; > - > - if (xenXMConfigSetInt(conf, "apic", > - (def->features[VIR_DOMAIN_FEATURE_APIC] == > - VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) > - goto cleanup; > - > - if (xendConfigVersion >= XEND_CONFIG_VERSION_3_0_4) { > - if (xenXMConfigSetInt(conf, "hap", > - (def->features[VIR_DOMAIN_FEATURE_HAP] == > - VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) > - goto cleanup; > - > - if (xenXMConfigSetInt(conf, "viridian", > - (def->features[VIR_DOMAIN_FEATURE_VIRIDIAN] == > - VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) > - goto cleanup; > - } > - > - for (i = 0; i < def->clock.ntimers; i++) { > - if (def->clock.timers[i]->name == VIR_DOMAIN_TIMER_NAME_HPET && > - def->clock.timers[i]->present != -1 && > - xenXMConfigSetInt(conf, "hpet", def->clock.timers[i]->present) < 0) > - goto cleanup; > - } > - > and features formatted here. > if (xendConfigVersion == XEND_CONFIG_VERSION_3_0_2) { > for (i = 0; i < def->ndisks; i++) { > if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_CDROM && > diff --git a/tests/xmconfigdata/test-escape-paths.cfg b/tests/xmconfigdata/test-escape-paths.cfg > index 13be2a0..4a18cc1 100644 > --- a/tests/xmconfigdata/test-escape-paths.cfg > +++ b/tests/xmconfigdata/test-escape-paths.cfg > With this approach, all the test data file changes are avoided. Simplified patch below. Regards, Jim

From 88e7f74c646b555cea64501254e53a19ff6ba780 Mon Sep 17 00:00:00 2001 From: Kiarie Kahurani <davidkiarie4(a)gmail.com&gt; Date: Tue, 12 Aug 2014 00:21:31 +0300 Subject: [PATCH 07/11] src/xenxs: Refactor code formating CPU config introduce functions xenFormatXMCPUAllocation(virConfPtr conf, ......); xenFormatXMCPUFeatures(virConfPtr conf, ......); which formats CPU allocation and features config Signed-off-by: Kiarie Kahurani <davidkiarie4(a)gmail.com&gt; Signed-off-by: Jim Fehlig <jfehlig(a)suse.com&gt; --- src/xenxs/xen_xm.c | 131 +++++++++++++++++++++++++++++++++-------------------- 1 file changed, 81 insertions(+), 50 deletions(-) diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index 36a5ea1..432fee2 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -1942,6 +1942,85 @@ xenFormatXMDisks(virConfPtr conf, virDomainDefPtr def, int xendConfigVersion) } +static int +xenFormatXMCPUAllocation(virConfPtr conf, virDomainDefPtr def) +{ + int ret = -1; + char *cpus = NULL; + + if (xenXMConfigSetInt(conf, "vcpus", def->maxvcpus) < 0) + goto cleanup; + + /* Computing the vcpu_avail bitmask works because MAX_VIRT_CPUS is + either 32, or 64 on a platform where long is big enough. */ + if (def->vcpus < def->maxvcpus && + xenXMConfigSetInt(conf, "vcpu_avail", (1UL << def->vcpus) - 1) < 0) + goto cleanup; + + if ((def->cpumask != NULL) && + ((cpus = virBitmapFormat(def->cpumask)) == NULL)) { + goto cleanup; + } + + if (cpus && + xenXMConfigSetString(conf, "cpus", cpus) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(cpus); + return ret; +} + + +static int +xenFormatXMCPUFeatures(virConfPtr conf, + virDomainDefPtr def, + int xendConfigVersion) +{ + size_t i; + + if (STREQ(def->os.type, "hvm")) { + if (xenXMConfigSetInt(conf, "pae", + (def->features[VIR_DOMAIN_FEATURE_PAE] == + VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) + return -1; + + if (xenXMConfigSetInt(conf, "acpi", + (def->features[VIR_DOMAIN_FEATURE_ACPI] == + VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) + return -1; + + if (xenXMConfigSetInt(conf, "apic", + (def->features[VIR_DOMAIN_FEATURE_APIC] == + VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) + return -1; + + if (xendConfigVersion >= XEND_CONFIG_VERSION_3_0_4) { + if (xenXMConfigSetInt(conf, "hap", + (def->features[VIR_DOMAIN_FEATURE_HAP] == + VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) + return -1; + + if (xenXMConfigSetInt(conf, "viridian", + (def->features[VIR_DOMAIN_FEATURE_VIRIDIAN] == + VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) + return -1; + } + + for (i = 0; i < def->clock.ntimers; i++) { + if (def->clock.timers[i]->name == VIR_DOMAIN_TIMER_NAME_HPET && + def->clock.timers[i]->present != -1 && + xenXMConfigSetInt(conf, "hpet", def->clock.timers[i]->present) < 0) + return -1; + } + } + + return 0; +} + + /* Computing the vcpu_avail bitmask works because MAX_VIRT_CPUS is either 32, or 64 on a platform where long is big enough. */ verify(MAX_VIRT_CPUS <= sizeof(1UL) * CHAR_BIT); @@ -1954,7 +2033,6 @@ xenFormatXM(virConnectPtr conn, virConfPtr conf = NULL; int hvm = 0; size_t i; - char *cpus = NULL; virConfValuePtr netVal = NULL; if (!(conf = virConfNew())) @@ -1966,23 +2044,8 @@ xenFormatXM(virConnectPtr conn, if (xenFormatXMMem(conf, def) < 0) goto cleanup; - if (xenXMConfigSetInt(conf, "vcpus", def->maxvcpus) < 0) + if (xenFormatXMCPUAllocation(conf, def) < 0) goto cleanup; - /* Computing the vcpu_avail bitmask works because MAX_VIRT_CPUS is - either 32, or 64 on a platform where long is big enough. */ - if (def->vcpus < def->maxvcpus && - xenXMConfigSetInt(conf, "vcpu_avail", (1UL << def->vcpus) - 1) < 0) - goto cleanup; - - if ((def->cpumask != NULL) && - ((cpus = virBitmapFormat(def->cpumask)) == NULL)) { - goto cleanup; - } - - if (cpus && - xenXMConfigSetString(conf, "cpus", cpus) < 0) - goto cleanup; - VIR_FREE(cpus); hvm = STREQ(def->os.type, "hvm") ? 1 : 0; @@ -2022,40 +2085,9 @@ xenFormatXM(virConnectPtr conn, if (xenXMConfigSetString(conf, "boot", boot) < 0) goto cleanup; - if (xenXMConfigSetInt(conf, "pae", - (def->features[VIR_DOMAIN_FEATURE_PAE] == - VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) + if (xenFormatXMCPUFeatures(conf, def, xendConfigVersion) < 0) goto cleanup; - if (xenXMConfigSetInt(conf, "acpi", - (def->features[VIR_DOMAIN_FEATURE_ACPI] == - VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) - goto cleanup; - - if (xenXMConfigSetInt(conf, "apic", - (def->features[VIR_DOMAIN_FEATURE_APIC] == - VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) - goto cleanup; - - if (xendConfigVersion >= XEND_CONFIG_VERSION_3_0_4) { - if (xenXMConfigSetInt(conf, "hap", - (def->features[VIR_DOMAIN_FEATURE_HAP] == - VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) - goto cleanup; - - if (xenXMConfigSetInt(conf, "viridian", - (def->features[VIR_DOMAIN_FEATURE_VIRIDIAN] == - VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0) - goto cleanup; - } - - for (i = 0; i < def->clock.ntimers; i++) { - if (def->clock.timers[i]->name == VIR_DOMAIN_TIMER_NAME_HPET && - def->clock.timers[i]->present != -1 && - xenXMConfigSetInt(conf, "hpet", def->clock.timers[i]->present) < 0) - goto cleanup; - } - if (xendConfigVersion == XEND_CONFIG_VERSION_3_0_2) { for (i = 0; i < def->ndisks; i++) { if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_CDROM && @@ -2268,7 +2300,6 @@ xenFormatXM(virConnectPtr conn, cleanup: virConfFreeValue(netVal); - VIR_FREE(cpus); if (conf) virConfFree(conf); return NULL; -- 1.8.4.5