| Message ID | 1465226212-254093-7-git-send-email-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/06/2016 17:16, Igor Mammedov wrote: > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > target-i386/cpu.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index f791a06..31e5e6f 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -1975,6 +1975,9 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > error_propagate(errp, local_err); > return; > } > + fprintf(stderr, > + "'+%s' is obsolete and will be removed in future, use '%s=on'", > + featurestr + 1, featurestr + 1); Could you detect using +foo together with foo=off, and -foo together with foo=on? Those are the really problematic cases, without them +foo and -foo can become synonyms for =on and =off. Paolo > continue; > } else if (featurestr[0] == '-') { > add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err); > @@ -1982,6 +1985,9 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > error_propagate(errp, local_err); > return; > } > + fprintf(stderr, > + "'-%s' is obsolete and will be removed in future, use '%s=off'", > + featurestr + 1, featurestr + 1); > continue; > } > >
On Tue, 7 Jun 2016 13:44:58 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 06/06/2016 17:16, Igor Mammedov wrote: > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > target-i386/cpu.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index f791a06..31e5e6f 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -1975,6 +1975,9 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > > error_propagate(errp, local_err); > > return; > > } > > + fprintf(stderr, > > + "'+%s' is obsolete and will be removed in future, use '%s=on'", > > + featurestr + 1, featurestr + 1); > > Could you detect using +foo together with foo=off, and -foo together > with foo=on? Those are the really problematic cases, without them +foo > and -foo can become synonyms for =on and =off. That's (legacy)current semantic of -cpu +-foo where it overrides any foo=x, potentially it's possible to track foo=x locally in parser and then compare with +-foo both ways. But all we can do currently is to print warning about such use case. I think Eduardo's suggestion to just warn that +-foo is obsolete for now and drop support for it in several releases is sufficient(good) enough. > Paolo > > > continue; > > } else if (featurestr[0] == '-') { > > add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err); > > @@ -1982,6 +1985,9 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > > error_propagate(errp, local_err); > > return; > > } > > + fprintf(stderr, > > + "'-%s' is obsolete and will be removed in future, use '%s=off'", > > + featurestr + 1, featurestr + 1); > > continue; > > } > > > >
On 07/06/2016 14:32, Igor Mammedov wrote: >> > Could you detect using +foo together with foo=off, and -foo together >> > with foo=on? Those are the really problematic cases, without them +foo >> > and -foo can become synonyms for =on and =off. > That's (legacy)current semantic of -cpu +-foo where it overrides any foo=x, > potentially it's possible to track foo=x locally in parser > and then compare with +-foo both ways. > But all we can do currently is to print warning about such use case. > > I think Eduardo's suggestion to just warn that +-foo is obsolete for now > and drop support for it in several releases is sufficient(good) enough. kvm-unit-tests and libvirt both use it, especially because =on and =off are relatively new I think? It seems like it's really widespread. Paolo
On Tue, 7 Jun 2016 14:36:51 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 07/06/2016 14:32, Igor Mammedov wrote: > >> > Could you detect using +foo together with foo=off, and -foo together > >> > with foo=on? Those are the really problematic cases, without them +foo > >> > and -foo can become synonyms for =on and =off. > > That's (legacy)current semantic of -cpu +-foo where it overrides any foo=x, > > potentially it's possible to track foo=x locally in parser > > and then compare with +-foo both ways. > > But all we can do currently is to print warning about such use case. > > > > I think Eduardo's suggestion to just warn that +-foo is obsolete for now > > and drop support for it in several releases is sufficient(good) enough. > > kvm-unit-tests and libvirt both use it, especially because =on and =off > are relatively new I think? It seems like it's really widespread. Yep, that's why it's not removed now. Looks like libvirt would be able to switch to foo=x syntax, I can take a look at kvm-unit-tests and make it use foo=x too.
On 07/06/2016 14:54, Igor Mammedov wrote: > On Tue, 7 Jun 2016 14:36:51 +0200 > Paolo Bonzini <pbonzini@redhat.com> wrote: > >> On 07/06/2016 14:32, Igor Mammedov wrote: >>>>> Could you detect using +foo together with foo=off, and -foo together >>>>> with foo=on? Those are the really problematic cases, without them +foo >>>>> and -foo can become synonyms for =on and =off. >>> That's (legacy)current semantic of -cpu +-foo where it overrides any foo=x, >>> potentially it's possible to track foo=x locally in parser >>> and then compare with +-foo both ways. >>> But all we can do currently is to print warning about such use case. >>> >>> I think Eduardo's suggestion to just warn that +-foo is obsolete for now >>> and drop support for it in several releases is sufficient(good) enough. >> >> kvm-unit-tests and libvirt both use it, especially because =on and =off >> are relatively new I think? It seems like it's really widespread. > > Yep, that's why it's not removed now. > Looks like libvirt would be able to switch to foo=x syntax, > I can take a look at kvm-unit-tests and make it use foo=x too. And all tutorials, and all scripts. It's really too hard. I'd really prefer to make an incompatible change straight in 2.7 for the case of mixed foo=x and [+-]foo. Paolo
On Tue, 7 Jun 2016 15:00:04 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 07/06/2016 14:54, Igor Mammedov wrote: > > On Tue, 7 Jun 2016 14:36:51 +0200 > > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > >> On 07/06/2016 14:32, Igor Mammedov wrote: > >>>>> Could you detect using +foo together with foo=off, and -foo together > >>>>> with foo=on? Those are the really problematic cases, without them +foo > >>>>> and -foo can become synonyms for =on and =off. > >>> That's (legacy)current semantic of -cpu +-foo where it overrides any foo=x, > >>> potentially it's possible to track foo=x locally in parser > >>> and then compare with +-foo both ways. > >>> But all we can do currently is to print warning about such use case. > >>> > >>> I think Eduardo's suggestion to just warn that +-foo is obsolete for now > >>> and drop support for it in several releases is sufficient(good) enough. > >> > >> kvm-unit-tests and libvirt both use it, especially because =on and =off > >> are relatively new I think? It seems like it's really widespread. > > > > Yep, that's why it's not removed now. > > Looks like libvirt would be able to switch to foo=x syntax, > > I can take a look at kvm-unit-tests and make it use foo=x too. > > And all tutorials, and all scripts. It's really too hard. > > I'd really prefer to make an incompatible change straight in 2.7 for the > case of mixed foo=x and [+-]foo. I've tried to make a bit more extreme incompatible change starting from 2.7 machine type in RFC (i.e. allow only foo=x syntax). But Eduardo prefers to keep current +-foo, just marking it obsolete so that users would have time to adapt before support for legacy +-foo is dropped. Anyways, we can introduce "mixed foo=x and [+-]foo" check on top of this series with warning and probably switch to fail later so not break existing users without giving them time to adapt. > Paolo
On Tue, Jun 07, 2016 at 03:26:58PM +0200, Igor Mammedov wrote: > On Tue, 7 Jun 2016 15:00:04 +0200 > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 07/06/2016 14:54, Igor Mammedov wrote: > > > On Tue, 7 Jun 2016 14:36:51 +0200 > > > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > >> On 07/06/2016 14:32, Igor Mammedov wrote: > > >>>>> Could you detect using +foo together with foo=off, and -foo together > > >>>>> with foo=on? Those are the really problematic cases, without them +foo > > >>>>> and -foo can become synonyms for =on and =off. > > >>> That's (legacy)current semantic of -cpu +-foo where it overrides any foo=x, > > >>> potentially it's possible to track foo=x locally in parser > > >>> and then compare with +-foo both ways. > > >>> But all we can do currently is to print warning about such use case. > > >>> > > >>> I think Eduardo's suggestion to just warn that +-foo is obsolete for now > > >>> and drop support for it in several releases is sufficient(good) enough. > > >> > > >> kvm-unit-tests and libvirt both use it, especially because =on and =off > > >> are relatively new I think? It seems like it's really widespread. > > > > > > Yep, that's why it's not removed now. > > > Looks like libvirt would be able to switch to foo=x syntax, > > > I can take a look at kvm-unit-tests and make it use foo=x too. > > > > And all tutorials, and all scripts. It's really too hard. > > > > I'd really prefer to make an incompatible change straight in 2.7 for the > > case of mixed foo=x and [+-]foo. > I've tried to make a bit more extreme incompatible change starting from 2.7 > machine type in RFC (i.e. allow only foo=x syntax). kvm-unit-tests would be another reason to not disable it unconditionally on pc-2.7, as it uses the default machine-type. > But Eduardo prefers to keep current +-foo, just marking it obsolete > so that users would have time to adapt before support for legacy +-foo > is dropped. > > Anyways, we can introduce "mixed foo=x and [+-]foo" check on top of > this series with warning and probably switch to fail later so not > break existing users without giving them time to adapt. We could make it fail in the case of mixing +foo and foo=x, but we would need to be careful to not break existing valid cases (like "vendor=AuthenticAMD,+foo", or "+foo,pmu=on". The code that handles "foo=x" doesn't know if the property is a feature flag or not. I'm not sure the extra code complexity is worth it. If we plan to to be stuck with the "[+-]foo" code forever (I am not completely against keeping it), why not just let people mix it with the new format? I don't see the point of making the code more convoluted just to prevent people from mixing both. If we don't plan to keep the [+-]foo code forever, then let's start printing a warning as soon as possible. (Note that I am not talking about the weird "-foo,+foo" ordering semantics. That's something else we need to warn about and eventually obsolete.)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index f791a06..31e5e6f 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1975,6 +1975,9 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, error_propagate(errp, local_err); return; } + fprintf(stderr, + "'+%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, &local_err); @@ -1982,6 +1985,9 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, error_propagate(errp, local_err); return; } + fprintf(stderr, + "'-%s' is obsolete and will be removed in future, use '%s=off'", + featurestr + 1, featurestr + 1); continue; }
Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- target-i386/cpu.c | 6 ++++++ 1 file changed, 6 insertions(+)