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

Re: [libvirt] [PATCH 2/3] conf: Refactor storing and usage of feature flags



On 09/24/2013 10:43 AM, Peter Krempa wrote:
> To allow a finer control for some future flags, refactor the code to
> store them in an array instead of a bitmap.
> ---
>  src/conf/domain_conf.c     | 166 ++++++++++++++++++++++++++++++---------------
>  src/conf/domain_conf.h     |   2 +-
>  src/libxl/libxl_conf.c     |   9 ++-
>  src/lxc/lxc_container.c    |   6 +-
>  src/qemu/qemu_command.c    |  16 ++---
>  src/vbox/vbox_tmpl.c       |  45 ++++++------
>  src/xenapi/xenapi_driver.c |  10 +--
>  src/xenapi/xenapi_utils.c  |  22 +++---
>  src/xenxs/xen_sxpr.c       |  20 +++---
>  src/xenxs/xen_xm.c         |  30 ++++----
>  10 files changed, 191 insertions(+), 135 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 8953579..e1a1d6d 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -11367,29 +11367,40 @@ virDomainDefParseXML(xmlDocPtr xml,
>          int val = virDomainFeatureTypeFromString((const char *)nodes[i]->name);
>          if (val < 0) {
>              virReportError(VIR_ERR_INTERNAL_ERROR,
> -                           _("unexpected feature %s"),
> -                           nodes[i]->name);
> +                           _("unexpected feature '%s'"), nodes[i]->name);

Unrelated change.

>              goto error;
>          }
> -        def->features |= (1 << val);
> -        if (val == VIR_DOMAIN_FEATURE_APIC) {
> -            tmp = virXPathString("string(./features/apic/@eoi)", ctxt);
> -            if (tmp) {
> +
> +        switch ((enum virDomainFeature) val) {
> +        case VIR_DOMAIN_FEATURE_APIC:
> +            if ((tmp = virXPathString("string(./features/apic/@eoi)", ctxt))) {
>                  int eoi;

>                  if ((eoi = virDomainFeatureStateTypeFromString(tmp)) <= 0) {
>                      virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                                   _("unknown value for attribute eoi: %s"),
> +                                   _("unknown value for attribute eoi: '%s'"),

Another unrelated change.

>                                     tmp);
>                      goto error;
>                  }
> -                def->apic_eoi = eoi;
> +               def->apic_eoi = eoi;

Indentantion is off.

>                  VIR_FREE(tmp);
>              }
> +            /* fallthrough */
> +        case VIR_DOMAIN_FEATURE_ACPI:
> +        case VIR_DOMAIN_FEATURE_PAE:
> +        case VIR_DOMAIN_FEATURE_HAP:
> +        case VIR_DOMAIN_FEATURE_VIRIDIAN:
> +        case VIR_DOMAIN_FEATURE_PRIVNET:
> +        case VIR_DOMAIN_FEATURE_HYPERV:
> +            def->features[val] = VIR_DOMAIN_FEATURE_STATE_ON;
> +            break;
> +
> +        case VIR_DOMAIN_FEATURE_LAST:
> +            break;
>          }
>      }
>      VIR_FREE(nodes);
> 
> -    if (def->features & (1 << VIR_DOMAIN_FEATURE_HYPERV)) {
> +    if (def->features[VIR_DOMAIN_FEATURE_HYPERV] == VIR_DOMAIN_FEATURE_STATE_ON) {
>          int feature;
>          int value;
>          node = ctxt->node;
> @@ -13361,12 +13372,16 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
>  {
>      size_t i;
> 
> -    /* basic check */
> -    if (src->features != dst->features) {
> -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                       _("Target domain features %d does not match source %d"),
> -                       dst->features, src->features);
> -        return false;
> +    for (i = 0; i < VIR_DOMAIN_FEATURE_LAST; i++) {
> +        if (src->features[i] != dst->features[i]) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("State of feature '%s' differs: "
> +                             "source: '%s', destination: '%s'"),
> +                           virDomainFeatureTypeToString(i),
> +                           virDomainFeatureStateTypeToString(src->features[i]),
> +                           virDomainFeatureStateTypeToString(dst->features[i]));
> +            return false;
> +        }

Yay, frendlier error message!

> @@ -16703,58 +16718,99 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>          virBufferAddLit(buf, "  </idmap>\n");
>      }
> 
> +    for (i = 0; i < VIR_DOMAIN_FEATURE_LAST; i++) {
> +        if (def->features[i] != VIR_DOMAIN_FEATURE_STATE_DEFAULT)
> +            break;
> +    }
> 
> -    if (def->features) {
> +    if (i != VIR_DOMAIN_FEATURE_LAST) {
>          virBufferAddLit(buf, "  <features>\n");
> +
>          for (i = 0; i < VIR_DOMAIN_FEATURE_LAST; i++) {
> -            if (def->features & (1 << i) && i != VIR_DOMAIN_FEATURE_HYPERV) {
> -                const char *name = virDomainFeatureTypeToString(i);
> -                if (!name) {
> -                    virReportError(VIR_ERR_INTERNAL_ERROR,
> -                                   _("unexpected feature %zu"), i);
> -                    goto error;
> -                }
> -                virBufferAsprintf(buf, "    <%s", name);
> -                if (i == VIR_DOMAIN_FEATURE_APIC && def->apic_eoi) {
> -                    virBufferAsprintf(buf,
> -                                      " eoi='%s'",
> -                                      virDomainFeatureStateTypeToString(def->apic_eoi));
> -                }
> -                virBufferAddLit(buf, "/>\n");
> +            const char *name = virDomainFeatureTypeToString(i);
> +            size_t j;
> +
> +            if (!name) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("unexpected feature %zu"), i);
> +                goto error;
>              }
> -        }
> 
> -        if (def->features & (1 << VIR_DOMAIN_FEATURE_HYPERV)) {
> -            virBufferAddLit(buf, "    <hyperv>\n");
> -            for (i = 0; i < VIR_DOMAIN_HYPERV_LAST; i++) {
> -                switch ((enum virDomainHyperv) i) {
> -                case VIR_DOMAIN_HYPERV_RELAXED:
> -                case VIR_DOMAIN_HYPERV_VAPIC:
> -                    if (def->hyperv_features[i])
> -                        virBufferAsprintf(buf, "      <%s state='%s'/>\n",
> -                                          virDomainHypervTypeToString(i),
> -                                          virDomainFeatureStateTypeToString(
> -                                              def->hyperv_features[i]));
> +            switch ((enum virDomainFeature) i) {
> +            case VIR_DOMAIN_FEATURE_ACPI:
> +            case VIR_DOMAIN_FEATURE_PAE:
> +            case VIR_DOMAIN_FEATURE_HAP:
> +            case VIR_DOMAIN_FEATURE_VIRIDIAN:
> +            case VIR_DOMAIN_FEATURE_PRIVNET:

> +                switch ((enum virDomainFeatureState) def->features[i]) {
> +                case VIR_DOMAIN_FEATURE_STATE_ON:
> +                    /* output just the element */
> +                   virBufferAsprintf(buf, "    <%s/>\n", name);
> +                   break;
> +

> +                case VIR_DOMAIN_FEATURE_STATE_OFF:
> +                   /* explicit off state */
> +                   virBufferAsprintf(buf, "    <%s state='off'/>\n", name);
> +                   break;

These features can't be in STATE_OFF, they are either on or we use the
default. This should be an internal error instead.

> +
> +                case VIR_DOMAIN_FEATURE_STATE_DEFAULT:
> +                case VIR_DOMAIN_FEATURE_STATE_LAST:
> +                    /* don't output the element */
>                      break;
> +                }


> 
> -                case VIR_DOMAIN_HYPERV_SPINLOCKS:
> -                    if (def->hyperv_features[i] == 0)
> -                        break;
> +                break;
> 
> -                    virBufferAsprintf(buf, "      <spinlocks state='%s'",
> -                                      virDomainFeatureStateTypeToString(
> -                                          def->hyperv_features[i]));
> -                    if (def->hyperv_features[i] == VIR_DOMAIN_FEATURE_STATE_ON)
> -                        virBufferAsprintf(buf, " retries='%d'",
> -                                          def->hyperv_spinlocks);


> @@ -10994,13 +10994,13 @@ qemuParseCommandLine(virCapsPtr qemuCaps,
> 
>      if ((def->os.arch == VIR_ARCH_I686) ||
>          (def->os.arch == VIR_ARCH_X86_64))
> -        def->features |= (1 << VIR_DOMAIN_FEATURE_ACPI)
> +        def->features[VIR_DOMAIN_FEATURE_ACPI] = VIR_DOMAIN_FEATURE_STATE_ON;
>          /*| (1 << VIR_DOMAIN_FEATURE_APIC)*/;

This comment would need to be rewritten or deleted.

> 
>  #define WANT_VALUE()                                                   \
>      const char *val = progargv[++i];                                   \
>      if (!val) {                                                        \
> -        virReportError(VIR_ERR_INTERNAL_ERROR,                        \
> +        virReportError(VIR_ERR_INTERNAL_ERROR,                         \
>                         _("missing value for %s argument"), arg);       \
>          goto error;                                                    \
>      }

NACK to this hunk.

Jan


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