[v2] treat host models as case-insensitive strings

[libvirt] [PATCH v2] treat host models as case-insensitive strings

Posted by Scott Garfinkle 7 years, 4 months ago

Qemu now allows case-insensitive specification of CPU models. This fixes the
resulting problems on (at least) POWER arch machines.

Patch V2: Change only the internal interface. This solves the actual problem at
hand of reporting unsupported models now that qemu allows case-insensitive
strings (e.g. "Power8" instead of "POWER8").

Signed-off-by: Scott Garfinkle <scottgar@linux.vnet.ibm.com>
---
 src/conf/domain_capabilities.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index e7323a8..f7d9be5 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -271,7 +271,7 @@ virDomainCapsCPUModelsGet(virDomainCapsCPUModelsPtr cpuModels,
         return NULL;
 
     for (i = 0; i < cpuModels->nmodels; i++) {
-        if (STREQ(cpuModels->models[i].name, name))
+        if (STRCASEEQ(cpuModels->models[i].name, name))
             return cpuModels->models + i;
     }
 
-- 
1.8.3.1

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

Re: [libvirt] [PATCH v2] treat host models as case-insensitive strings

Posted by John Ferlan 7 years, 4 months ago

On 12/26/2017 02:55 PM, Scott Garfinkle wrote:
> Qemu now allows case-insensitive specification of CPU models. This fixes the
> resulting problems on (at least) POWER arch machines.

Would have been great to reference which qemu commit number, but there's
probably way too many.  Perhaps best to note it's as of QEMU 2.11.  My
quick search turns up "03c9141d75" and "4a12c699d", is that about right?
Although there's quite a few other ones too

> 
> Patch V2: Change only the internal interface. This solves the actual problem at
> hand of reporting unsupported models now that qemu allows case-insensitive
> strings (e.g. "Power8" instead of "POWER8").

This hunk would typically go under the "---" below to give a hint to the
reviewer about what changed.

Hopefully no one someday decides POWER8 is more "powerful" than Power8
;-) (couldn't resist).

> 
> Signed-off-by: Scott Garfinkle <scottgar@linux.vnet.ibm.com>
> ---
>  src/conf/domain_capabilities.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

This seems like a reasonable way to approach the problem.  I can fix up
the comment message a bit and will push later... Just want to make sure
that no one else comes up with a latent concern...  The only thing that
springs to my mind is migration possibly.

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

> diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
> index e7323a8..f7d9be5 100644
> --- a/src/conf/domain_capabilities.c
> +++ b/src/conf/domain_capabilities.c
> @@ -271,7 +271,7 @@ virDomainCapsCPUModelsGet(virDomainCapsCPUModelsPtr cpuModels,
>          return NULL;
>  
>      for (i = 0; i < cpuModels->nmodels; i++) {
> -        if (STREQ(cpuModels->models[i].name, name))
> +        if (STRCASEEQ(cpuModels->models[i].name, name))
>              return cpuModels->models + i;
>      }
>  
> 

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

Re: [libvirt] [PATCH v2] treat host models as case-insensitive strings

Posted by Scott Garfinkle 7 years, 4 months ago

On Thu, 2018-01-11 at 11:19 -0500, John Ferlan wrote:
> 
> On 12/26/2017 02:55 PM, Scott Garfinkle wrote:
> > Qemu now allows case-insensitive specification of CPU models. This
> > fixes the resulting problems on (at least) POWER arch machines.
> 
> Would have been great to reference which qemu commit number, but
> there's probably way too many.  Perhaps best to note it's as of QEMU
> 2.11.  My quick search turns up "03c9141d75" and "4a12c699d", is that
> about right?
Thanks, next time I'll try to be more specific. Yes Qemu 2.11. The full
patch series is at
   https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04651.html

> > 
> > Patch V2: Change only the internal interface. This solves the
> > actual problem at
> > hand of reporting unsupported models now that qemu allows case-
> > insensitive
> > strings (e.g. "Power8" instead of "POWER8").
> 
> This hunk would typically go under the "---" below to give a hint to
> the reviewer about what changed.
ack

> Hopefully no one someday decides POWER8 is more "powerful" than
> Power8 ;-) (couldn't resist).
groan  :-)

> > 
> > Signed-off-by: Scott Garfinkle <scottgar@linux.vnet.ibm.com>
> > ---
> >  src/conf/domain_capabilities.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> 
> This seems like a reasonable way to approach the problem.  I can fix
> up the comment message a bit and will push later... Just want to make
> sure that no one else comes up with a latent concern...  The only
> thing that springs to my mind is migration possibly.
It seems to me that the in the worst we might be more permissive with
the patch than without; I am hopeful for the sake of sanity that nobody
actually has two model names disambiguated only by case!

> Reviewed-by: John Ferlan <jferlan@redhat.com>
> 
> John
> 
> > diff --git a/src/conf/domain_capabilities.c
> > b/src/conf/domain_capabilities.c
> > index e7323a8..f7d9be5 100644
> > --- a/src/conf/domain_capabilities.c
> > +++ b/src/conf/domain_capabilities.c
> > @@ -271,7 +271,7 @@
> > virDomainCapsCPUModelsGet(virDomainCapsCPUModelsPtr cpuModels,
> >          return NULL;
> >  
> >      for (i = 0; i < cpuModels->nmodels; i++) {
> > -        if (STREQ(cpuModels->models[i].name, name))
> > +        if (STRCASEEQ(cpuModels->models[i].name, name))
> >              return cpuModels->models + i;
> >      }
> >  
> > 
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

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