Re: [Qemu-devel] [PATCH 8/9] target-i386: Use "-" instead of "_" on all feature names

[Igor Mammedov]

On Fri, 27 May 2016 17:32:34 -0300
Eduardo Habkost <address@hidden> wrote:

> On Tue, May 24, 2016 at 03:22:27PM +0200, Igor Mammedov wrote:
> > On Tue, 24 May 2016 09:34:05 -0300
> > Eduardo Habkost <address@hidden> wrote:
> >   
> > > On Tue, May 24, 2016 at 02:17:03PM +0200, Igor Mammedov wrote:  
> > > > On Fri,  6 May 2016 15:11:31 -0300
> > > > Eduardo Habkost <address@hidden> wrote:  
> [...]
> > > > > -/* Convert all '_' in a feature string option name to '-', to make 
> > > > > feature
> > > > > - * name conform to QOM property naming rule, which uses '-' instead 
> > > > > of '_'.
> > > > > +/* Convert all '_' in a feature string option name to '-', to keep 
> > > > > compatibility
> > > > > + * with old feature names that used "_" instead of "-".
> > > > >   */
> > > > >  static inline void feat2prop(char *s)
> > > > >  {
> > > > > @@ -1925,8 +1925,10 @@ static void x86_cpu_parse_featurestr(CPUState 
> > > > > *cs, char *features,
> > > > >      while (featurestr) {
> > > > >          char *val;    
> > > > I'd place a single feat2prop() here
> > > > and delete it from other call sites in this function.    
> > > 
> > > A previous version of this patch had it. But it would change the
> > > property value too, not just the property name (breaking stuff
> > > like "model-id=some_string").
> > >  
> > it's bug in feat2prop(), which probably should be fixed there,
> > so it would do what comment above it says. Or as alternative:  
> 
> The comment above it doesn't say anything about stopping at a '='
> delimiter. I always expected it to just replace "_" with "-" in a
> null-terminated string.
> 
> (I am not completely against making it stop at '=', but I believe
> your suggestion below sounds better).
> 
> > 
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index ca2a893..e46e4c3 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1941,14 +1941,16 @@ static void x86_cpu_parse_featurestr(CPUState *cs, 
> > char *features
> >      featurestr = features ? strtok(features, ",") : NULL;
> >  
> >      while (featurestr) {
> > -        char *val;
> > +        char *val = strchr(featurestr, '=');
> > +        if (val) {
> > +            *val = 0; val++;
> > +        }
> > +        feat2prop(featurestr);  
> 
> This would make "+feature=FOO" treated as a valid option, and it
> isn't. It would keep the existing behavior if we did this:
> 
> -          if (featurestr[0] == '+') {
> +          if (featurestr[0] == '+' && !val) {
>                add_flagname_to_bitmaps(featurestr + 1, plus_features, 
> &local_err);
> -          } else if (featurestr[0] == '-') {
> +          if (featurestr[0] == '+' && !val) {
>                add_flagname_to_bitmaps(featurestr + 1, minus_features, 
> &local_err);
> 
> In either case, I prefer to get this optimization reviewed as a
> separate patch. Can you send it as a follow-up?
sure

> 
> > -        } else if ((val = strchr(featurestr, '='))) {
> > -            *val = 0; val++;
> > -            feat2prop(featurestr);
> > +        } else if (val) {
> >              if (!strcmp(featurestr, "xlevel")) {
> >                  char *err;
> >                  char num[32];
> > @@ -2000,7 +2002,6 @@ static void x86_cpu_parse_featurestr(CPUState *cs, 
> > char *features,
> >                  object_property_parse(OBJECT(cpu), val, featurestr, 
> > &local_err);
> >              }
> >          } else {
> > -            feat2prop(featurestr);
> >              object_property_parse(OBJECT(cpu), "on", featurestr, 
> > &local_err);
> >          }
> >          if (local_err) {
> > 
> >   
>