[libvirt] [PATCH 2/3] conf: Refactor storing and usage of feature flags
Ján Tomko
jtomko at redhat.com
Wed Sep 25 15:43:05 UTC 2013
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
More information about the libvir-list
mailing list