Re: [libvirt] [PATCH 09/11] conf: Improve IOAPIC feature handling

[Andrea Bolognani]

On Tue, 2018-02-13 at 09:34 -0500, John Ferlan wrote:
> On 02/13/2018 08:46 AM, Andrea Bolognani wrote:
> > [...] Should I
> > send out a v2, or should I just go ahead and push the series given
> > the minor nature of the adjustments?
> 
> I see no need for a v2 since the adjustments were essentially minor.
> Consider some of those comments a replacement for you (or I) being able
> to walk across the big pond in order to toss random thoughts or ideas
> that spring to mind when I'm reading patches. Many times it's just what
> I'm thinking as I'm reading. If it gives you an aha moment to describe
> or do something different, then great. If it's utter nonesense, then no
> big deal either ;-).

Cool, I had assumed as much but wanted to make sure ;)

I've now pushed the series, thanks for your review!

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 09/11] conf: Improve IOAPIC feature handling

[John Ferlan]

On 02/13/2018 08:46 AM, Andrea Bolognani wrote:
> On Tue, 2018-02-13 at 07:22 -0500, John Ferlan wrote:
>>> Another approach, which didn't make it to the list for some reason,
>>> was to end the comment with
>>>
>>>   [...] and so on. See virDomainDefFeaturesCheckABIStability() for
>>>   more details.
>>>
>>> That seems like a better way to handle the ever-changing nature of
>>> libvirt than a comment, don't you think?
>>
>> That's fine... I guess since you started listing them I figured adding
>> another in the next patch was "natural".
>>
>> How about this (or something close to it):
>>
>> "Most {hyperv_|kvm_|cpu_}feature options utilize a virTristateSwitch to
>> handle support. A few assign specific data values to the option. See
>> virDomainDefFeaturesCheckABIStability for details."
> 
> I've changed it to match your suggestion.
> 
> I've also addressed all other review comments as described in my
> previous replies. Do you have any objections to those? Should I
> send out a v2, or should I just go ahead and push the series given
> the minor nature of the adjustments?
> 

I see no need for a v2 since the adjustments were essentially minor.
Consider some of those comments a replacement for you (or I) being able
to walk across the big pond in order to toss random thoughts or ideas
that spring to mind when I'm reading patches. Many times it's just what
I'm thinking as I'm reading. If it gives you an aha moment to describe
or do something different, then great. If it's utter nonesense, then no
big deal either ;-).

John

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

Re: [libvirt] [PATCH 09/11] conf: Improve IOAPIC feature handling

[Andrea Bolognani]

On Tue, 2018-02-13 at 07:22 -0500, John Ferlan wrote:
> > Another approach, which didn't make it to the list for some reason,
> > was to end the comment with
> > 
> >   [...] and so on. See virDomainDefFeaturesCheckABIStability() for
> >   more details.
> > 
> > That seems like a better way to handle the ever-changing nature of
> > libvirt than a comment, don't you think?
> 
> That's fine... I guess since you started listing them I figured adding
> another in the next patch was "natural".
> 
> How about this (or something close to it):
> 
> "Most {hyperv_|kvm_|cpu_}feature options utilize a virTristateSwitch to
> handle support. A few assign specific data values to the option. See
> virDomainDefFeaturesCheckABIStability for details."

I've changed it to match your suggestion.

I've also addressed all other review comments as described in my
previous replies. Do you have any objections to those? Should I
send out a v2, or should I just go ahead and push the series given
the minor nature of the adjustments?

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 09/11] conf: Improve IOAPIC feature handling

[John Ferlan]

On 02/13/2018 04:40 AM, Andrea Bolognani wrote:
> On Mon, 2018-02-12 at 16:59 -0500, John Ferlan wrote:
>>> @@ -19224,14 +19224,13 @@ virDomainDefParseXML(xmlDocPtr xml,
>>>  tmp = virXMLPropString(nodes[i], "driver");
>>>  if (tmp) {
>>>  int value = virDomainIOAPICTypeFromString(tmp);
>>> -if (value < 0) {
>>> +if (value < 0 || value == VIR_DOMAIN_IOAPIC_NONE) {
>>
>> "none" wouldn't be legal XML according to "iopic" in domaincommon.rng. I
>> think this is where things get tricky - I'm fine with the changes as is,
>> but that interaction between domain_conf xml parsing and the rng schema
>> gets me wondering about whether that "none" value should be in the rng.
>> Perhaps there's another opinion on this that may want to pipe in now...
> 
> "none" should definitely *not* be in the schema, since it's not
> a valid value. The problem here is that schema compliance is not
> enforced by libvirt: when using virsh, you are given the option
> to simply ignore schema validation failures and go ahead with
> defining/modifying the guest, so the only way to actually make
> sure invalid values don't make it into the virDomainDef is to do
> something like the above.
> 
> I mean, it's not like accepting "none" in the parser would be a
> big issue - it would just be ignored anyway. But by adding an
> explicit check we can report a better error in the very unlikely
> scenario where
> 
>   
> 
> has been specified in the guest configuration, so I think it's
> a good idea to have it there given how little effort it requires.
> 
>>> @@ -2352,9 +2353,10 @@ struct _virDomainDef {
>>>  
>>>  virDomainOSDef os;
>>>  char *emulator;
>>> -/* These three options are of type virTristateSwitch,
>>> - * except VIR_DOMAIN_FEATURE_CAPABILITIES that is of type
>>> - * virDomainCapabilitiesPolicy */
>>> +/* Most of the values in {kvm_,hyperv_,}features are of type
>>
>> {kvm_|hyperv_|caps_}features
>>
>>> + * virTristateSwitch, but there are exceptions: for example,
>>> + * VIR_DOMAIN_FEATURE_CAPABILITIES is of type 
>>> virDomainCapabilitiesPolicy,
>>
>> s/,//
>>
>>> + * VIR_DOMAIN_FEATURE_IOAPIC is of type virDomainIOAPIC and so on */
>>
>> s/and so on/./
> 
> I see you ask for the comment to be updated in the next commit when
> _HPT is changed as well, but it wasn't really my intention to list
> all types there - we both know how that kind of comment can get out
> of sync with reality very quickly :)
> 
> Another approach, which didn't make it to the list for some reason,
> was to end the comment with
> 
>   [...] and so on. See virDomainDefFeaturesCheckABIStability() for
>   more details.
> 
> That seems like a better way to handle the ever-changing nature of
> libvirt than a comment, don't you think?
> 

That's fine... I guess since you started listing them I figured adding
another in the next patch was "natural".

How about this (or something close to it):

"Most {hyperv_|kvm_|cpu_}feature options utilize a virTristateSwitch to
handle support. A few assign specific data values to the option. See
virDomainDefFeaturesCheckABIStability for details."

John

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

Re: [libvirt] [PATCH 09/11] conf: Improve IOAPIC feature handling

[Andrea Bolognani]

On Mon, 2018-02-12 at 16:59 -0500, John Ferlan wrote:
> > @@ -19224,14 +19224,13 @@ virDomainDefParseXML(xmlDocPtr xml,
> >  tmp = virXMLPropString(nodes[i], "driver");
> >  if (tmp) {
> >  int value = virDomainIOAPICTypeFromString(tmp);
> > -if (value < 0) {
> > +if (value < 0 || value == VIR_DOMAIN_IOAPIC_NONE) {
> 
> "none" wouldn't be legal XML according to "iopic" in domaincommon.rng. I
> think this is where things get tricky - I'm fine with the changes as is,
> but that interaction between domain_conf xml parsing and the rng schema
> gets me wondering about whether that "none" value should be in the rng.
> Perhaps there's another opinion on this that may want to pipe in now...

"none" should definitely *not* be in the schema, since it's not
a valid value. The problem here is that schema compliance is not
enforced by libvirt: when using virsh, you are given the option
to simply ignore schema validation failures and go ahead with
defining/modifying the guest, so the only way to actually make
sure invalid values don't make it into the virDomainDef is to do
something like the above.

I mean, it's not like accepting "none" in the parser would be a
big issue - it would just be ignored anyway. But by adding an
explicit check we can report a better error in the very unlikely
scenario where

  

has been specified in the guest configuration, so I think it's
a good idea to have it there given how little effort it requires.

> > @@ -2352,9 +2353,10 @@ struct _virDomainDef {
> >  
> >  virDomainOSDef os;
> >  char *emulator;
> > -/* These three options are of type virTristateSwitch,
> > - * except VIR_DOMAIN_FEATURE_CAPABILITIES that is of type
> > - * virDomainCapabilitiesPolicy */
> > +/* Most of the values in {kvm_,hyperv_,}features are of type
> 
> {kvm_|hyperv_|caps_}features
> 
> > + * virTristateSwitch, but there are exceptions: for example,
> > + * VIR_DOMAIN_FEATURE_CAPABILITIES is of type 
> > virDomainCapabilitiesPolicy,
> 
> s/,//
> 
> > + * VIR_DOMAIN_FEATURE_IOAPIC is of type virDomainIOAPIC and so on */
> 
> s/and so on/./

I see you ask for the comment to be updated in the next commit when
_HPT is changed as well, but it wasn't really my intention to list
all types there - we both know how that kind of comment can get out
of sync with reality very quickly :)

Another approach, which didn't make it to the list for some reason,
was to end the comment with

  [...] and so on. See virDomainDefFeaturesCheckABIStability() for
  more details.

That seems like a better way to handle the ever-changing nature of
libvirt than a comment, don't you think?

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 09/11] conf: Improve IOAPIC feature handling

[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 driver should be used, store both of them in
> a single place.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/conf/domain_conf.c  | 29 +
>  src/conf/domain_conf.h  | 11 ++-
>  src/qemu/qemu_command.c |  5 +++--
>  src/qemu/qemu_domain.c  |  2 +-
>  4 files changed, 23 insertions(+), 24 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 170c56665..19884ec13 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -900,6 +900,7 @@ VIR_ENUM_IMPL(virDomainLoader,
>  
>  VIR_ENUM_IMPL(virDomainIOAPIC,
>VIR_DOMAIN_IOAPIC_LAST,
> +  "none",
>"qemu",
>"kvm")
>  
> @@ -5913,8 +5914,7 @@ virDomainDefValidateInternal(const virDomainDef *def)
>  
>  if (def->iommu &&
>  def->iommu->intremap == VIR_TRISTATE_SWITCH_ON &&
> -(def->features[VIR_DOMAIN_FEATURE_IOAPIC] != VIR_TRISTATE_SWITCH_ON 
> ||
> - def->ioapic != VIR_DOMAIN_IOAPIC_QEMU)) {
> +def->features[VIR_DOMAIN_FEATURE_IOAPIC] != VIR_DOMAIN_IOAPIC_QEMU) {
>  virReportError(VIR_ERR_XML_ERROR, "%s",
> _("IOMMU interrupt remapping requires split I/O APIC "
>   "(ioapic driver='qemu')"));
> @@ -19224,14 +19224,13 @@ virDomainDefParseXML(xmlDocPtr xml,
>  tmp = virXMLPropString(nodes[i], "driver");
>  if (tmp) {
>  int value = virDomainIOAPICTypeFromString(tmp);
> -if (value < 0) {
> +if (value < 0 || value == VIR_DOMAIN_IOAPIC_NONE) {

"none" wouldn't be legal XML according to "iopic" in domaincommon.rng. I
think this is where things get tricky - I'm fine with the changes as is,
but that interaction between domain_conf xml parsing and the rng schema
gets me wondering about whether that "none" value should be in the rng.
Perhaps there's another opinion on this that may want to pipe in now...

>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("Unknown driver mode: %s"),
> tmp);
>  goto error;
>  }
> -def->ioapic = value;
> -def->features[val] = VIR_TRISTATE_SWITCH_ON;
> +def->features[val] = value;
>  VIR_FREE(tmp);
>  }
>  break;
> @@ -21408,16 +21407,13 @@ 
> virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
>  break;
>  
>  case VIR_DOMAIN_FEATURE_IOAPIC:
> -if (src->features[i] != dst->features[i] ||
> -src->ioapic != dst->ioapic) {
> +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, "driver",
> -   
> virTristateSwitchTypeToString(src->features[i]),
> -   virDomainIOAPICTypeToString(src->ioapic),
> -   
> virTristateSwitchTypeToString(dst->features[i]),
> -   virDomainIOAPICTypeToString(dst->ioapic));
> +   virDomainIOAPICTypeToString(src->features[i]),
> +   
> virDomainIOAPICTypeToString(dst->features[i]));
>  return false;
>  }
>  break;
> @@ -26943,10 +26939,11 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>  break;
>  
>  case VIR_DOMAIN_FEATURE_IOAPIC:
> -if (def->features[i] == VIR_TRISTATE_SWITCH_ON) {
> -virBufferAsprintf(buf, "\n",
> -  
> virDomainIOAPICTypeToString(def->ioapic));
> -}
> +if (def->features[i] == VIR_DOMAIN_IOAPIC_NONE)
> +break;
> +
> +virBufferAsprintf(buf, "\n",
> +  
> virDomainIOAPICTypeToString(def->features[i]));
>  break;
>  
>  case VIR_DOMAIN_FEATURE_HPT:
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 21e004515..20f0efc36 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1861,7 +1861,8 @@ struct _virDomainLoaderDef {
>  void virDomainLoaderDefFree(virDomainLoaderDefPtr loader);
>  
>  typedef enum {
> -VIR_DOMAIN_IOAPIC_QEMU = 0,
> +VIR_DOMAIN_IOAPIC_NONE = 0,
> +VIR_DOMAIN_IOAPIC_QEMU,
>  VIR_DOMAIN_IOAPIC_KVM,
>  
>  VIR_DOMAIN_IOAPIC_LAST
> @@ -2352,9 +2353,10 @@ 

[libvirt] [PATCH 09/11] conf: Improve IOAPIC feature handling

[Andrea Bolognani]

Instead of storing separately whether the feature is enabled
or not and what driver should be used, store both of them in
a single place.

Signed-off-by: Andrea Bolognani 
---
 src/conf/domain_conf.c  | 29 +
 src/conf/domain_conf.h  | 11 ++-
 src/qemu/qemu_command.c |  5 +++--
 src/qemu/qemu_domain.c  |  2 +-
 4 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 170c56665..19884ec13 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -900,6 +900,7 @@ VIR_ENUM_IMPL(virDomainLoader,
 
 VIR_ENUM_IMPL(virDomainIOAPIC,
   VIR_DOMAIN_IOAPIC_LAST,
+  "none",
   "qemu",
   "kvm")
 
@@ -5913,8 +5914,7 @@ virDomainDefValidateInternal(const virDomainDef *def)
 
 if (def->iommu &&
 def->iommu->intremap == VIR_TRISTATE_SWITCH_ON &&
-(def->features[VIR_DOMAIN_FEATURE_IOAPIC] != VIR_TRISTATE_SWITCH_ON ||
- def->ioapic != VIR_DOMAIN_IOAPIC_QEMU)) {
+def->features[VIR_DOMAIN_FEATURE_IOAPIC] != VIR_DOMAIN_IOAPIC_QEMU) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
_("IOMMU interrupt remapping requires split I/O APIC "
  "(ioapic driver='qemu')"));
@@ -19224,14 +19224,13 @@ virDomainDefParseXML(xmlDocPtr xml,
 tmp = virXMLPropString(nodes[i], "driver");
 if (tmp) {
 int value = virDomainIOAPICTypeFromString(tmp);
-if (value < 0) {
+if (value < 0 || value == VIR_DOMAIN_IOAPIC_NONE) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unknown driver mode: %s"),
tmp);
 goto error;
 }
-def->ioapic = value;
-def->features[val] = VIR_TRISTATE_SWITCH_ON;
+def->features[val] = value;
 VIR_FREE(tmp);
 }
 break;
@@ -21408,16 +21407,13 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr 
src,
 break;
 
 case VIR_DOMAIN_FEATURE_IOAPIC:
-if (src->features[i] != dst->features[i] ||
-src->ioapic != dst->ioapic) {
+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, "driver",
-   virTristateSwitchTypeToString(src->features[i]),
-   virDomainIOAPICTypeToString(src->ioapic),
-   virTristateSwitchTypeToString(dst->features[i]),
-   virDomainIOAPICTypeToString(dst->ioapic));
+   virDomainIOAPICTypeToString(src->features[i]),
+   virDomainIOAPICTypeToString(dst->features[i]));
 return false;
 }
 break;
@@ -26943,10 +26939,11 @@ virDomainDefFormatInternal(virDomainDefPtr def,
 break;
 
 case VIR_DOMAIN_FEATURE_IOAPIC:
-if (def->features[i] == VIR_TRISTATE_SWITCH_ON) {
-virBufferAsprintf(buf, "\n",
-  
virDomainIOAPICTypeToString(def->ioapic));
-}
+if (def->features[i] == VIR_DOMAIN_IOAPIC_NONE)
+break;
+
+virBufferAsprintf(buf, "\n",
+  
virDomainIOAPICTypeToString(def->features[i]));
 break;
 
 case VIR_DOMAIN_FEATURE_HPT:
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 21e004515..20f0efc36 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1861,7 +1861,8 @@ struct _virDomainLoaderDef {
 void virDomainLoaderDefFree(virDomainLoaderDefPtr loader);
 
 typedef enum {
-VIR_DOMAIN_IOAPIC_QEMU = 0,
+VIR_DOMAIN_IOAPIC_NONE = 0,
+VIR_DOMAIN_IOAPIC_QEMU,
 VIR_DOMAIN_IOAPIC_KVM,
 
 VIR_DOMAIN_IOAPIC_LAST
@@ -2352,9 +2353,10 @@ struct _virDomainDef {
 
 virDomainOSDef os;
 char *emulator;
-/* These three options are of type virTristateSwitch,
- * except VIR_DOMAIN_FEATURE_CAPABILITIES that is of type
- * virDomainCapabilitiesPolicy */
+/* Most of the values in {kvm_,hyperv_,}features are of type
+ * virTristateSwitch, but there are exceptions: for example,
+ * VIR_DOMAIN_FEATURE_CAPABILITIES is of type virDomainCapabilitiesPolicy,
+ * VIR_DOMAIN_FEATURE_IOAPIC is of type virDomainIOAPIC and so on */
 int features[VIR_DOMAIN_FEATURE_LAST];
 int apic_eoi;
 int hyperv_features[VIR_DOMAIN_HYPERV_LAST];
@@