Re: [libvirt] [PATCH 10/11] conf: Improve HPT feature handling

2018-02-12 Thread John Ferlan 


On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
> Instead of storing separately whether the feature is enabled
> or not and what resizing policy should be used, store both of
> them in a single place.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/conf/domain_conf.c  | 26 --
>  src/conf/domain_conf.h  |  4 ++--
>  src/qemu/qemu_command.c |  4 ++--
>  src/qemu/qemu_domain.c  |  2 +-
>  4 files changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 19884ec13..c1d549594 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -906,6 +906,7 @@ VIR_ENUM_IMPL(virDomainIOAPIC,
>  
>  VIR_ENUM_IMPL(virDomainHPTResizing,
>VIR_DOMAIN_HPT_RESIZING_LAST,
> +  "none",
>"enabled",
>"disabled",
>"required",
> @@ -19239,14 +19240,13 @@ virDomainDefParseXML(xmlDocPtr xml,
>  tmp = virXMLPropString(nodes[i], "resizing");
>  if (tmp) {
>  int value = virDomainHPTResizingTypeFromString(tmp);
> -if (value < 0) {
> +if (value < 0 || value == VIR_DOMAIN_HPT_RESIZING_NONE) {

Similar note as w/ previous and rng schema. Here too I'm fine with this
way, but if someone else isn't hopefully they speak up.

>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("Unknown HPT resizing setting: %s"),
> tmp);
>  goto error;
>  }
> -def->hpt_resizing = value;
> -def->features[val] = VIR_TRISTATE_SWITCH_ON;
> +def->features[val] = value;
>  VIR_FREE(tmp);
>  }
>  break;
> @@ -21377,16 +21377,13 @@ 
> virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
>  break;
>  
>  case VIR_DOMAIN_FEATURE_HPT:
> -if (src->features[i] != dst->features[i] ||
> -src->hpt_resizing != dst->hpt_resizing) {
> +if (src->features[i] != dst->features[i]) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("State of feature '%s:%s' differs: "
> - "source: '%s:%s', destination: '%s:%s'"),
> + "source: '%s', destination: '%s'"),
> featureName, "resizing",
> -   
> virTristateSwitchTypeToString(src->features[i]),
> -   
> virDomainHPTResizingTypeToString(src->hpt_resizing),
> -   
> virTristateSwitchTypeToString(dst->features[i]),
> -   
> virDomainHPTResizingTypeToString(dst->hpt_resizing));
> +   
> virDomainHPTResizingTypeToString(src->features[i]),
> +   
> virDomainHPTResizingTypeToString(dst->features[i]));
>  return false;
>  }
>  break;
> @@ -26947,10 +26944,11 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>  break;
>  
>  case VIR_DOMAIN_FEATURE_HPT:
> -if (def->features[i] == VIR_TRISTATE_SWITCH_ON) {
> -virBufferAsprintf(buf, "\n",
> -  
> virDomainHPTResizingTypeToString(def->hpt_resizing));
> -}
> +if (def->features[i] == VIR_DOMAIN_HPT_RESIZING_NONE)
> +break;
> +
> +virBufferAsprintf(buf, "\n",
> +  
> virDomainHPTResizingTypeToString(def->features[i]));
>  break;
>  
>  /* coverity[dead_error_begin] */
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 20f0efc36..4e9044ae6 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1871,7 +1871,8 @@ typedef enum {
>  VIR_ENUM_DECL(virDomainIOAPIC);
>  
>  typedef enum {
> -VIR_DOMAIN_HPT_RESIZING_ENABLED = 0,
> +VIR_DOMAIN_HPT_RESIZING_NONE = 0,
> +VIR_DOMAIN_HPT_RESIZING_ENABLED,
>  VIR_DOMAIN_HPT_RESIZING_DISABLED,
>  VIR_DOMAIN_HPT_RESIZING_REQUIRED,
>  
> @@ -2364,7 +2365,6 @@ struct _virDomainDef {

The comment a few lines above here should be updated to include:

 * VIR_DOMAIN_FEATURE_HPT is of type virDomainHPTResizing

Reviewed-by: John Ferlan 

John

>  unsigned int hyperv_spinlocks;
>  virGICVersion gic_version;
>  char *hyperv_vendor_id;
> -virDomainHPTResizing hpt_resizing;
>  
>  /* These options are of type virTristateSwitch: ON = keep, OFF = drop */
>  int caps_features[VIR_DOMAIN_CAPS_FEATURE_LAST];

[...]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 10/11] conf: Improve HPT feature handling

2018-02-06 Thread Andrea Bolognani 
Instead of storing separately whether the feature is enabled
or not and what resizing policy should be used, store both of
them in a single place.

Signed-off-by: Andrea Bolognani 
---
 src/conf/domain_conf.c  | 26 --
 src/conf/domain_conf.h  |  4 ++--
 src/qemu/qemu_command.c |  4 ++--
 src/qemu/qemu_domain.c  |  2 +-
 4 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 19884ec13..c1d549594 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -906,6 +906,7 @@ VIR_ENUM_IMPL(virDomainIOAPIC,
 
 VIR_ENUM_IMPL(virDomainHPTResizing,
   VIR_DOMAIN_HPT_RESIZING_LAST,
+  "none",
   "enabled",
   "disabled",
   "required",
@@ -19239,14 +19240,13 @@ virDomainDefParseXML(xmlDocPtr xml,
 tmp = virXMLPropString(nodes[i], "resizing");
 if (tmp) {
 int value = virDomainHPTResizingTypeFromString(tmp);
-if (value < 0) {
+if (value < 0 || value == VIR_DOMAIN_HPT_RESIZING_NONE) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unknown HPT resizing setting: %s"),
tmp);
 goto error;
 }
-def->hpt_resizing = value;
-def->features[val] = VIR_TRISTATE_SWITCH_ON;
+def->features[val] = value;
 VIR_FREE(tmp);
 }
 break;
@@ -21377,16 +21377,13 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr 
src,
 break;
 
 case VIR_DOMAIN_FEATURE_HPT:
-if (src->features[i] != dst->features[i] ||
-src->hpt_resizing != dst->hpt_resizing) {
+if (src->features[i] != dst->features[i]) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("State of feature '%s:%s' differs: "
- "source: '%s:%s', destination: '%s:%s'"),
+ "source: '%s', destination: '%s'"),
featureName, "resizing",
-   virTristateSwitchTypeToString(src->features[i]),
-   
virDomainHPTResizingTypeToString(src->hpt_resizing),
-   virTristateSwitchTypeToString(dst->features[i]),
-   
virDomainHPTResizingTypeToString(dst->hpt_resizing));
+   
virDomainHPTResizingTypeToString(src->features[i]),
+   
virDomainHPTResizingTypeToString(dst->features[i]));
 return false;
 }
 break;
@@ -26947,10 +26944,11 @@ virDomainDefFormatInternal(virDomainDefPtr def,
 break;
 
 case VIR_DOMAIN_FEATURE_HPT:
-if (def->features[i] == VIR_TRISTATE_SWITCH_ON) {
-virBufferAsprintf(buf, "\n",
-  
virDomainHPTResizingTypeToString(def->hpt_resizing));
-}
+if (def->features[i] == VIR_DOMAIN_HPT_RESIZING_NONE)
+break;
+
+virBufferAsprintf(buf, "\n",
+  
virDomainHPTResizingTypeToString(def->features[i]));
 break;
 
 /* coverity[dead_error_begin] */
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 20f0efc36..4e9044ae6 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1871,7 +1871,8 @@ typedef enum {
 VIR_ENUM_DECL(virDomainIOAPIC);
 
 typedef enum {
-VIR_DOMAIN_HPT_RESIZING_ENABLED = 0,
+VIR_DOMAIN_HPT_RESIZING_NONE = 0,
+VIR_DOMAIN_HPT_RESIZING_ENABLED,
 VIR_DOMAIN_HPT_RESIZING_DISABLED,
 VIR_DOMAIN_HPT_RESIZING_REQUIRED,
 
@@ -2364,7 +2365,6 @@ struct _virDomainDef {
 unsigned int hyperv_spinlocks;
 virGICVersion gic_version;
 char *hyperv_vendor_id;
-virDomainHPTResizing hpt_resizing;
 
 /* These options are of type virTristateSwitch: ON = keep, OFF = drop */
 int caps_features[VIR_DOMAIN_CAPS_FEATURE_LAST];
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 3aabdf7a2..faf09a599 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7266,7 +7266,7 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
 }
 }
 
-if (def->features[VIR_DOMAIN_FEATURE_HPT] == VIR_TRISTATE_SWITCH_ON) {
+if (def->features[VIR_DOMAIN_FEATURE_HPT] != 
VIR_DOMAIN_HPT_RESIZING_NONE) {
 const char *str;
 
 if (!virQEMUCapsGet(qemuCaps, 
QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT)) {
@@ -7276,7 +7276,7 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
 goto cleanup;
 }
 
-str = virDomainHPTResizingTypeToString(def->hpt_resizing);
+str =