| Message ID | 20171108022828.7242-6-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | generic way to deprecate machines | expand |
On 08/11/17 13:28, Philippe Mathieu-Daudé wrote: > Both are deprecated since 2.9.0 May be so but I could not find where this happened in git history: [vpl1 qemu]$ git log -n1 --grep 'pc-0.10' f6d5a0bad2 2 years, 5 months ago Eduardo Habkost piix: Define PC_COMPAT_0_10 Having some pointers in the commit log would be helpful imho, like [2/6]. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/i386/pc_piix.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index f79d5cb694..c462b702ff 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -941,6 +941,7 @@ static void pc_i440fx_0_11_machine_options(MachineClass *m) > pc_i440fx_0_12_machine_options(m); > m->hw_version = "0.11"; > SET_MACHINE_COMPAT(m, PC_COMPAT_0_11); > + m->deprecated_reason = "Too old"; > } > > DEFINE_I440FX_MACHINE(v0_11, "pc-0.11", pc_compat_0_13, > @@ -976,6 +977,7 @@ static void pc_i440fx_0_10_machine_options(MachineClass *m) > pc_i440fx_0_11_machine_options(m); > m->hw_version = "0.10"; > SET_MACHINE_COMPAT(m, PC_COMPAT_0_10); > + m->deprecated_reason = "Too old"; > } > > DEFINE_I440FX_MACHINE(v0_10, "pc-0.10", pc_compat_0_13, >
Hi Alexey, On 11/07/2017 11:48 PM, Alexey Kardashevskiy wrote: > On 08/11/17 13:28, Philippe Mathieu-Daudé wrote: >> Both are deprecated since 2.9.0 > > May be so but I could not find where this happened in git history: > > [vpl1 qemu]$ git log -n1 --grep 'pc-0.10' > f6d5a0bad2 2 years, 5 months ago Eduardo Habkost piix: Define PC_COMPAT_0_10 > > Having some pointers in the commit log would be helpful imho, like [2/6]. Ok! If there is positive feedback on this series I'll respin trying to be more verbose. I took "2.9.0" from the wiki history [1], which is the formal way to go as I understand: Before an "interface" can be removed, there needs to be a clear transition plan for doing so. This means it should be added to this wiki page, and that there have to be at least two public releases of QEMU where the interface still works, but issues a warning when used. Two public releases are required so that the users have at least one development cycle to complain if they think that there is a good reason that the interface should not be removed. The warning should say that the interface is deprecated / going away soon, with a pointer to documentation or similar information about its replacement syntax. [1] https://wiki.qemu.org/index.php?title=Features/LegacyRemoval&action=history >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> hw/i386/pc_piix.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >> index f79d5cb694..c462b702ff 100644 >> --- a/hw/i386/pc_piix.c >> +++ b/hw/i386/pc_piix.c >> @@ -941,6 +941,7 @@ static void pc_i440fx_0_11_machine_options(MachineClass *m) >> pc_i440fx_0_12_machine_options(m); >> m->hw_version = "0.11"; >> SET_MACHINE_COMPAT(m, PC_COMPAT_0_11); >> + m->deprecated_reason = "Too old"; >> } >> >> DEFINE_I440FX_MACHINE(v0_11, "pc-0.11", pc_compat_0_13, >> @@ -976,6 +977,7 @@ static void pc_i440fx_0_10_machine_options(MachineClass *m) >> pc_i440fx_0_11_machine_options(m); >> m->hw_version = "0.10"; >> SET_MACHINE_COMPAT(m, PC_COMPAT_0_10); >> + m->deprecated_reason = "Too old"; >> } >> >> DEFINE_I440FX_MACHINE(v0_10, "pc-0.10", pc_compat_0_13, >>
On Wed, Nov 08, 2017 at 12:28:00AM -0300, Philippe Mathieu-Daudé wrote: > Hi Alexey, > > On 11/07/2017 11:48 PM, Alexey Kardashevskiy wrote: > > On 08/11/17 13:28, Philippe Mathieu-Daudé wrote: > >> Both are deprecated since 2.9.0 > > > > May be so but I could not find where this happened in git history: > > > > [vpl1 qemu]$ git log -n1 --grep 'pc-0.10' > > f6d5a0bad2 2 years, 5 months ago Eduardo Habkost piix: Define PC_COMPAT_0_10 > > > > Having some pointers in the commit log would be helpful imho, like [2/6]. > > Ok! If there is positive feedback on this series I'll respin trying to > be more verbose. > > I took "2.9.0" from the wiki history [1], which is the formal way to go > as I understand: No, the wiki page content was just an initial jumping off point for discussions. The formal list of what's deprecated is kept in the qemu-doc.texi file in the "Deprecated features" appendix. We really ought to remove most of the content from that wiki page and just point to the formal docs, to avoid continued confusion like this. > > Before an "interface" can be removed, there needs to be a clear > transition plan for doing so. This means it should be added to this wiki > page, and that there have to be at least two public releases of QEMU > where the interface still works, but issues a warning when used. Two > public releases are required so that the users have at least one > development cycle to complain if they think that there is a good reason > that the interface should not be removed. The warning should say that > the interface is deprecated / going away soon, with a pointer to > documentation or similar information about its replacement syntax. > > [1] > https://wiki.qemu.org/index.php?title=Features/LegacyRemoval&action=history > > >> > >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > >> --- > >> hw/i386/pc_piix.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > >> index f79d5cb694..c462b702ff 100644 > >> --- a/hw/i386/pc_piix.c > >> +++ b/hw/i386/pc_piix.c > >> @@ -941,6 +941,7 @@ static void pc_i440fx_0_11_machine_options(MachineClass *m) > >> pc_i440fx_0_12_machine_options(m); > >> m->hw_version = "0.11"; > >> SET_MACHINE_COMPAT(m, PC_COMPAT_0_11); > >> + m->deprecated_reason = "Too old"; > >> } > >> > >> DEFINE_I440FX_MACHINE(v0_11, "pc-0.11", pc_compat_0_13, > >> @@ -976,6 +977,7 @@ static void pc_i440fx_0_10_machine_options(MachineClass *m) > >> pc_i440fx_0_11_machine_options(m); > >> m->hw_version = "0.10"; > >> SET_MACHINE_COMPAT(m, PC_COMPAT_0_10); > >> + m->deprecated_reason = "Too old"; > >> } > >> > >> DEFINE_I440FX_MACHINE(v0_10, "pc-0.10", pc_compat_0_13, > >> > > Regards, Daniel
On Tue, Nov 07, 2017 at 11:28:27PM -0300, Philippe Mathieu-Daudé wrote: > Both are deprecated since 2.9.0 > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/i386/pc_piix.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index f79d5cb694..c462b702ff 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -941,6 +941,7 @@ static void pc_i440fx_0_11_machine_options(MachineClass *m) > pc_i440fx_0_12_machine_options(m); > m->hw_version = "0.11"; > SET_MACHINE_COMPAT(m, PC_COMPAT_0_11); > + m->deprecated_reason = "Too old"; > } > > DEFINE_I440FX_MACHINE(v0_11, "pc-0.11", pc_compat_0_13, > @@ -976,6 +977,7 @@ static void pc_i440fx_0_10_machine_options(MachineClass *m) > pc_i440fx_0_11_machine_options(m); > m->hw_version = "0.10"; > SET_MACHINE_COMPAT(m, PC_COMPAT_0_10); > + m->deprecated_reason = "Too old"; > } Any deprecation message should give users useful information about what they should do instead to avoid the problem. More generally though, what would removing these two machines types allow us to actually achieve ? Seems quite arbitrary to pick 0.10 and 0.11 but leave the other 20+ machine types. Regards, Daniel
On 08.11.2017 10:16, Daniel P. Berrange wrote: > On Tue, Nov 07, 2017 at 11:28:27PM -0300, Philippe Mathieu-Daudé wrote: >> Both are deprecated since 2.9.0 >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> hw/i386/pc_piix.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >> index f79d5cb694..c462b702ff 100644 >> --- a/hw/i386/pc_piix.c >> +++ b/hw/i386/pc_piix.c >> @@ -941,6 +941,7 @@ static void pc_i440fx_0_11_machine_options(MachineClass *m) >> pc_i440fx_0_12_machine_options(m); >> m->hw_version = "0.11"; >> SET_MACHINE_COMPAT(m, PC_COMPAT_0_11); >> + m->deprecated_reason = "Too old"; >> } >> >> DEFINE_I440FX_MACHINE(v0_11, "pc-0.11", pc_compat_0_13, >> @@ -976,6 +977,7 @@ static void pc_i440fx_0_10_machine_options(MachineClass *m) >> pc_i440fx_0_11_machine_options(m); >> m->hw_version = "0.10"; >> SET_MACHINE_COMPAT(m, PC_COMPAT_0_10); >> + m->deprecated_reason = "Too old"; >> } > > Any deprecation message should give users useful information about what > they should do instead to avoid the problem. +1 > More generally though, what would removing these two machines types allow > us to actually achieve ? Seems quite arbitrary to pick 0.10 and 0.11 but > leave the other 20+ machine types. We've discussed this a couple of months ago on the mailing list: The basic idea is that the oldest machine type that is still in use is 0.12 (as a base for the RHEL 6 machines), so we should avoid to get rid of that code yet. But 0.10 and 0.11 very likely do not have any users anymore, so it should be OK to deprecate them. Thomas
On 08.11.2017 10:13, Daniel P. Berrange wrote: > On Wed, Nov 08, 2017 at 12:28:00AM -0300, Philippe Mathieu-Daudé wrote: >> Hi Alexey, >> >> On 11/07/2017 11:48 PM, Alexey Kardashevskiy wrote: >>> On 08/11/17 13:28, Philippe Mathieu-Daudé wrote: >>>> Both are deprecated since 2.9.0 >>> >>> May be so but I could not find where this happened in git history: >>> >>> [vpl1 qemu]$ git log -n1 --grep 'pc-0.10' >>> f6d5a0bad2 2 years, 5 months ago Eduardo Habkost piix: Define PC_COMPAT_0_10 >>> >>> Having some pointers in the commit log would be helpful imho, like [2/6]. >> >> Ok! If there is positive feedback on this series I'll respin trying to >> be more verbose. >> >> I took "2.9.0" from the wiki history [1], which is the formal way to go >> as I understand: > > No, the wiki page content was just an initial jumping off point for > discussions. The formal list of what's deprecated is kept in the > qemu-doc.texi file in the "Deprecated features" appendix. > > We really ought to remove most of the content from that wiki page > and just point to the formal docs, to avoid continued confusion > like this. I've changed the wording on the wiki page and added a pointer to the qemu-doc there, so it's hopefully now clearer that the qemu-doc is the mandatory list of deprecated features / interfaces. Thomas
On 08.11.2017 03:28, Philippe Mathieu-Daudé wrote: > Both are deprecated since 2.9.0 > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/i386/pc_piix.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index f79d5cb694..c462b702ff 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -941,6 +941,7 @@ static void pc_i440fx_0_11_machine_options(MachineClass *m) > pc_i440fx_0_12_machine_options(m); > m->hw_version = "0.11"; > SET_MACHINE_COMPAT(m, PC_COMPAT_0_11); > + m->deprecated_reason = "Too old"; As Daniel already mentioned it, I'd also suggest to use a more helpful message instead (e.g. "Use a newer version of pc-i440fx instead" ?). > } > > DEFINE_I440FX_MACHINE(v0_11, "pc-0.11", pc_compat_0_13, > @@ -976,6 +977,7 @@ static void pc_i440fx_0_10_machine_options(MachineClass *m) > pc_i440fx_0_11_machine_options(m); > m->hw_version = "0.10"; > SET_MACHINE_COMPAT(m, PC_COMPAT_0_10); > + m->deprecated_reason = "Too old"; I think you don't need the second hunk here since the function is calling pc_i440fx_0_11_machine_options(), too. Thomas
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index f79d5cb694..c462b702ff 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -941,6 +941,7 @@ static void pc_i440fx_0_11_machine_options(MachineClass *m) pc_i440fx_0_12_machine_options(m); m->hw_version = "0.11"; SET_MACHINE_COMPAT(m, PC_COMPAT_0_11); + m->deprecated_reason = "Too old"; } DEFINE_I440FX_MACHINE(v0_11, "pc-0.11", pc_compat_0_13, @@ -976,6 +977,7 @@ static void pc_i440fx_0_10_machine_options(MachineClass *m) pc_i440fx_0_11_machine_options(m); m->hw_version = "0.10"; SET_MACHINE_COMPAT(m, PC_COMPAT_0_10); + m->deprecated_reason = "Too old"; } DEFINE_I440FX_MACHINE(v0_10, "pc-0.10", pc_compat_0_13,
Both are deprecated since 2.9.0 Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/i386/pc_piix.c | 2 ++ 1 file changed, 2 insertions(+)