[libvirt] [PATCH V5 08/12] src/xenxs: Refactor code formating CPU features config
David Kiarie
davidkiarie4 at gmail.com
Thu Aug 14 14:58:16 UTC 2014
On Wed, Aug 13, 2014 at 05:35:52PM -0600, Jim Fehlig wrote:
> Kiarie Kahurani wrote:
> > introduce functions
> > xenFormatXMCPUFeatures(virConfPtr conf, ......);
> > which formats CPU features config instead
> >
> > Signed-off-by: Kiarie Kahurani <davidkiarie4 at gmail.com>
> > ---
> > 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 at gmail.com>
> 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 at gmail.com>
> Signed-off-by: Jim Fehlig <jfehlig at suse.com>
> ---
> 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
>
Good one, it also paves way for the inclusion of the many cpu allocation and features
config options that xl has to offer
More information about the libvir-list
mailing list