Re: [Qemu-devel] [PULL 10/10] target-i386: Print obsolete warnings if +-features are used

[Eduardo Habkost]

On Tue, Jun 14, 2016 at 05:38:42PM -0400, Paolo Bonzini wrote:
> 
> 
> - Original Message -
> > From: "Eduardo Habkost" 
> > To: "Paolo Bonzini" 
> > Cc: "Peter Maydell" , "Andreas Färber" 
> > , qemu-devel@nongnu.org, "Richard
> > Henderson" , "Igor Mammedov" 
> > Sent: Tuesday, June 14, 2016 11:31:03 PM
> > Subject: Re: [PULL 10/10] target-i386: Print obsolete warnings if 
> > +-features are used
> > 
> > On Tue, Jun 14, 2016 at 05:16:40PM -0400, Paolo Bonzini wrote:
> > > - Original Message -
> > > > From: "Eduardo Habkost" 
> > > > To: "Peter Maydell" 
> > > > Cc: "Andreas Färber" , qemu-devel@nongnu.org, "Richard
> > > > Henderson" , "Paolo
> > > > Bonzini" , "Igor Mammedov" 
> > > > Sent: Tuesday, June 14, 2016 10:59:08 PM
> > > > Subject: [PULL 10/10] target-i386: Print obsolete warnings if +-features
> > > > are used
> > > > 
> > > > From: Igor Mammedov 
> > > > 
> > > > Signed-off-by: Igor Mammedov 
> > > > [ehabkost: Changed to use error_report()]
> > > > Signed-off-by: Eduardo Habkost 
> > > > ---
> > > >  target-i386/cpu.c | 6 ++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > index 3665fec..baa3783 100644
> > > > --- a/target-i386/cpu.c
> > > > +++ b/target-i386/cpu.c
> > > > @@ -1980,9 +1980,15 @@ static void x86_cpu_parse_featurestr(CPUState 
> > > > *cs,
> > > > char *features,
> > > >  /* Compatibility syntax: */
> > > >  if (featurestr[0] == '+') {
> > > >  add_flagname_to_bitmaps(featurestr + 1, plus_features,
> > > >  _err);
> > > > +error_report(
> > > > +"'+%s' is obsolete and will be removed in future, use
> > > > '%s=on'",
> > > > +featurestr + 1, featurestr + 1);
> > > >  continue;
> > > >  } else if (featurestr[0] == '-') {
> > > >  add_flagname_to_bitmaps(featurestr + 1, minus_features,
> > > >  _err);
> > > > +error_report(
> > > > +"'-%s' is obsolete and will be removed in future, use
> > > > '%s=off'",
> > > > +featurestr + 1, featurestr + 1);
> > > >  continue;
> > > >  }
> > > 
> > > I still disagree with this change.
> > 
> > I've just removed the patch from the x86-pull-request tag, while
> > we sort this out.
> > 
> > Do you suggest supporting the "[+-]feature" syntax forever?
> 
> I suggest supporting it, but removing the awful interaction with 
> "feature=on/off"
> as soon as possible.  This shouldn't block this pull request, of course.

I plan to fix the awful ordering semantics. First with a warning
for 1 or 2 releases (only when the weird semantics is really
triggered), then +feature/-feature could be directly translated
to feature=on/feature=off.

> 
> I just believe it's not practical to remove the feature.  For example
> kvm-unit-tests can be used with new kernel and old QEMU, so I don't think
> it will move away from [+-]feature very soon.
> 
> Regarding libvirt, is "feature=on/off" introspectable?  That would also be
> a problem for libvirt to support both old and new QEMU.

Good point. Removing the feature would require dozens of extra
compatibility code to libvirt and kvm-unit-tests just to save 6
lines of code in QEMU. You convinced me.

-- 
Eduardo

Re: [Qemu-devel] [PULL 10/10] target-i386: Print obsolete warnings if +-features are used

[Paolo Bonzini]

- Original Message -
> From: "Eduardo Habkost" 
> To: "Paolo Bonzini" 
> Cc: "Peter Maydell" , "Andreas Färber" 
> , qemu-devel@nongnu.org, "Richard
> Henderson" , "Igor Mammedov" 
> Sent: Tuesday, June 14, 2016 11:31:03 PM
> Subject: Re: [PULL 10/10] target-i386: Print obsolete warnings if +-features 
> are used
> 
> On Tue, Jun 14, 2016 at 05:16:40PM -0400, Paolo Bonzini wrote:
> > - Original Message -
> > > From: "Eduardo Habkost" 
> > > To: "Peter Maydell" 
> > > Cc: "Andreas Färber" , qemu-devel@nongnu.org, "Richard
> > > Henderson" , "Paolo
> > > Bonzini" , "Igor Mammedov" 
> > > Sent: Tuesday, June 14, 2016 10:59:08 PM
> > > Subject: [PULL 10/10] target-i386: Print obsolete warnings if +-features
> > > are used
> > > 
> > > From: Igor Mammedov 
> > > 
> > > Signed-off-by: Igor Mammedov 
> > > [ehabkost: Changed to use error_report()]
> > > Signed-off-by: Eduardo Habkost 
> > > ---
> > >  target-i386/cpu.c | 6 ++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index 3665fec..baa3783 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -1980,9 +1980,15 @@ static void x86_cpu_parse_featurestr(CPUState *cs,
> > > char *features,
> > >  /* Compatibility syntax: */
> > >  if (featurestr[0] == '+') {
> > >  add_flagname_to_bitmaps(featurestr + 1, plus_features,
> > >  _err);
> > > +error_report(
> > > +"'+%s' is obsolete and will be removed in future, use
> > > '%s=on'",
> > > +featurestr + 1, featurestr + 1);
> > >  continue;
> > >  } else if (featurestr[0] == '-') {
> > >  add_flagname_to_bitmaps(featurestr + 1, minus_features,
> > >  _err);
> > > +error_report(
> > > +"'-%s' is obsolete and will be removed in future, use
> > > '%s=off'",
> > > +featurestr + 1, featurestr + 1);
> > >  continue;
> > >  }
> > 
> > I still disagree with this change.
> 
> I've just removed the patch from the x86-pull-request tag, while
> we sort this out.
> 
> Do you suggest supporting the "[+-]feature" syntax forever?

I suggest supporting it, but removing the awful interaction with 
"feature=on/off"
as soon as possible.  This shouldn't block this pull request, of course.

I just believe it's not practical to remove the feature.  For example
kvm-unit-tests can be used with new kernel and old QEMU, so I don't think
it will move away from [+-]feature very soon.

Regarding libvirt, is "feature=on/off" introspectable?  That would also be
a problem for libvirt to support both old and new QEMU.

Paolo

Re: [Qemu-devel] [PULL 10/10] target-i386: Print obsolete warnings if +-features are used

[Eduardo Habkost]

On Tue, Jun 14, 2016 at 05:16:40PM -0400, Paolo Bonzini wrote:
> - Original Message -
> > From: "Eduardo Habkost" 
> > To: "Peter Maydell" 
> > Cc: "Andreas Färber" , qemu-devel@nongnu.org, "Richard 
> > Henderson" , "Paolo
> > Bonzini" , "Igor Mammedov" 
> > Sent: Tuesday, June 14, 2016 10:59:08 PM
> > Subject: [PULL 10/10] target-i386: Print obsolete warnings if +-features 
> > are used
> > 
> > From: Igor Mammedov 
> > 
> > Signed-off-by: Igor Mammedov 
> > [ehabkost: Changed to use error_report()]
> > Signed-off-by: Eduardo Habkost 
> > ---
> >  target-i386/cpu.c | 6 ++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 3665fec..baa3783 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1980,9 +1980,15 @@ static void x86_cpu_parse_featurestr(CPUState *cs,
> > char *features,
> >  /* Compatibility syntax: */
> >  if (featurestr[0] == '+') {
> >  add_flagname_to_bitmaps(featurestr + 1, plus_features,
> >  _err);
> > +error_report(
> > +"'+%s' is obsolete and will be removed in future, use
> > '%s=on'",
> > +featurestr + 1, featurestr + 1);
> >  continue;
> >  } else if (featurestr[0] == '-') {
> >  add_flagname_to_bitmaps(featurestr + 1, minus_features,
> >  _err);
> > +error_report(
> > +"'-%s' is obsolete and will be removed in future, use
> > '%s=off'",
> > +featurestr + 1, featurestr + 1);
> >  continue;
> >  }
> 
> I still disagree with this change.

I've just removed the patch from the x86-pull-request tag, while
we sort this out.

Do you suggest supporting the "[+-]feature" syntax forever? If
that's really what you prefer, I won't complain too loudly.  It's
only a few extra lines of code, after all.

But if you have something else in mind, please clarify what you
suggest.

-- 
Eduardo

Re: [Qemu-devel] [PULL 10/10] target-i386: Print obsolete warnings if +-features are used

[Paolo Bonzini]

- Original Message -
> From: "Eduardo Habkost" 
> To: "Peter Maydell" 
> Cc: "Andreas Färber" , qemu-devel@nongnu.org, "Richard 
> Henderson" , "Paolo
> Bonzini" , "Igor Mammedov" 
> Sent: Tuesday, June 14, 2016 10:59:08 PM
> Subject: [PULL 10/10] target-i386: Print obsolete warnings if +-features are 
> used
> 
> From: Igor Mammedov 
> 
> Signed-off-by: Igor Mammedov 
> [ehabkost: Changed to use error_report()]
> Signed-off-by: Eduardo Habkost 
> ---
>  target-i386/cpu.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 3665fec..baa3783 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1980,9 +1980,15 @@ static void x86_cpu_parse_featurestr(CPUState *cs,
> char *features,
>  /* Compatibility syntax: */
>  if (featurestr[0] == '+') {
>  add_flagname_to_bitmaps(featurestr + 1, plus_features,
>  _err);
> +error_report(
> +"'+%s' is obsolete and will be removed in future, use
> '%s=on'",
> +featurestr + 1, featurestr + 1);
>  continue;
>  } else if (featurestr[0] == '-') {
>  add_flagname_to_bitmaps(featurestr + 1, minus_features,
>  _err);
> +error_report(
> +"'-%s' is obsolete and will be removed in future, use
> '%s=off'",
> +featurestr + 1, featurestr + 1);
>  continue;
>  }

I still disagree with this change.

Paolo