[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 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 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 gmail com>
> Signed-off-by: Jim Fehlig <jfehlig 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



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]