| Message ID | 20171108022828.7242-2-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | generic way to deprecate machines | expand |
On 08.11.2017 03:28, Philippe Mathieu-Daudé wrote: > If a machine has the deprecated_reason property set: > - it won't get listed anymore by the "-M help" command, > - a warning will be displayed if the machine is used. > > Example: > $ ppc-softmmu/qemu-system-ppc -M prep > qemu-system-ppc: -M prep: warning: The PowerPC PREP platform machine is deprecated (Obsoleted by the "40p" machine) > [machine starting ...] > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > include/hw/boards.h | 1 + > vl.c | 7 +++++++ > 2 files changed, 8 insertions(+) > > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 191a5b3cd8..4d5c180968 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -182,6 +182,7 @@ struct MachineClass { > const char *default_boot_order; > const char *default_display; > GArray *compat_props; > + const char *deprecated_reason; > const char *hw_version; > ram_addr_t default_ram_size; > const char *default_cpu_type; > diff --git a/vl.c b/vl.c > index ec299099ff..9ed4648af2 100644 > --- a/vl.c > +++ b/vl.c > @@ -2742,6 +2742,10 @@ static gint machine_class_cmp(gconstpointer a, gconstpointer b) > } > if (mc) { > g_slist_free(machines); > + if (mc->deprecated_reason) { > + warn_report("The %s machine is deprecated (%s)", > + mc->desc, mc->deprecated_reason); > + } Just cosmetic, but maybe rather put that into select_machine() instead? > return mc; > } > if (name && !is_help_option(name)) { > @@ -2752,6 +2756,9 @@ static gint machine_class_cmp(gconstpointer a, gconstpointer b) > machines = g_slist_sort(machines, machine_class_cmp); > for (el = machines; el; el = el->next) { > MachineClass *mc = el->data; > + if (mc->deprecated_reason) { > + continue; > + } Not sure, but maybe it would be more user-friendly to still list the machine, but add a "(deprecated)" string at the end? Thomas
On Wed, Nov 08, 2017 at 11:23:53AM +0100, Thomas Huth wrote: > On 08.11.2017 03:28, Philippe Mathieu-Daudé wrote: > > If a machine has the deprecated_reason property set: > > - it won't get listed anymore by the "-M help" command, > > - a warning will be displayed if the machine is used. > > > > Example: > > $ ppc-softmmu/qemu-system-ppc -M prep > > qemu-system-ppc: -M prep: warning: The PowerPC PREP platform machine is deprecated (Obsoleted by the "40p" machine) > > [machine starting ...] > > > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > --- > > include/hw/boards.h | 1 + > > vl.c | 7 +++++++ > > 2 files changed, 8 insertions(+) > > > > diff --git a/include/hw/boards.h b/include/hw/boards.h > > index 191a5b3cd8..4d5c180968 100644 > > --- a/include/hw/boards.h > > +++ b/include/hw/boards.h > > @@ -182,6 +182,7 @@ struct MachineClass { > > const char *default_boot_order; > > const char *default_display; > > GArray *compat_props; > > + const char *deprecated_reason; > > const char *hw_version; > > ram_addr_t default_ram_size; > > const char *default_cpu_type; > > diff --git a/vl.c b/vl.c > > index ec299099ff..9ed4648af2 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -2742,6 +2742,10 @@ static gint machine_class_cmp(gconstpointer a, gconstpointer b) > > } > > if (mc) { > > g_slist_free(machines); > > + if (mc->deprecated_reason) { > > + warn_report("The %s machine is deprecated (%s)", > > + mc->desc, mc->deprecated_reason); > > + } > > Just cosmetic, but maybe rather put that into select_machine() instead? > > > return mc; > > } > > if (name && !is_help_option(name)) { > > @@ -2752,6 +2756,9 @@ static gint machine_class_cmp(gconstpointer a, gconstpointer b) > > machines = g_slist_sort(machines, machine_class_cmp); > > for (el = machines; el; el = el->next) { > > MachineClass *mc = el->data; > > + if (mc->deprecated_reason) { > > + continue; > > + } > > Not sure, but maybe it would be more user-friendly to still list the > machine, but add a "(deprecated)" string at the end? Yeah, as long as it still exists, it should be visible. Regards, Daniel
On Tue, Nov 07, 2017 at 11:28:23PM -0300, Philippe Mathieu-Daudé wrote: > If a machine has the deprecated_reason property set: > - it won't get listed anymore by the "-M help" command, > - a warning will be displayed if the machine is used. > > Example: > $ ppc-softmmu/qemu-system-ppc -M prep > qemu-system-ppc: -M prep: warning: The PowerPC PREP platform machine is deprecated (Obsoleted by the "40p" machine) > [machine starting ...] > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > include/hw/boards.h | 1 + > vl.c | 7 +++++++ > 2 files changed, 8 insertions(+) > > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 191a5b3cd8..4d5c180968 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -182,6 +182,7 @@ struct MachineClass { > const char *default_boot_order; > const char *default_display; > GArray *compat_props; > + const char *deprecated_reason; This series introduces 5 different flavors of deprecation messages: * "Too old" * "Unmaintained" * "The ZCU102 machine has the same features supported" * "Use the \"pc\" machine instead" * "Obsoleted by the \"40p\" machine" Can we clearly document guidelines and examples for values of this field, to help ensure consistency? Examples of questions that could be answered in the field documentation: * Should the message start with an uppercase letter? * Should it really explain _why_ it was deprecated, or is a simple "please use xlnx-zcu102 instead" good enough? * Which of the following is preferred: * "obsoleted by the \"pc\" machine" * "obsoleted by \"pc\"" * "use \"pc\" instead" * "too old, use \"pc\" instead" * "too old; use \"pc\" instead" * <something else?> Minor suggestion: name the field 'deprecation_reason' instead of 'deprecated_reason'. > const char *hw_version; > ram_addr_t default_ram_size; > const char *default_cpu_type; > diff --git a/vl.c b/vl.c > index ec299099ff..9ed4648af2 100644 > --- a/vl.c > +++ b/vl.c > @@ -2742,6 +2742,10 @@ static gint machine_class_cmp(gconstpointer a, gconstpointer b) > } > if (mc) { > g_slist_free(machines); > + if (mc->deprecated_reason) { > + warn_report("The %s machine is deprecated (%s)", > + mc->desc, mc->deprecated_reason); I would use mc->name instead of mc->desc. > + } > return mc; > } > if (name && !is_help_option(name)) { > @@ -2752,6 +2756,9 @@ static gint machine_class_cmp(gconstpointer a, gconstpointer b) > machines = g_slist_sort(machines, machine_class_cmp); > for (el = machines; el; el = el->next) { > MachineClass *mc = el->data; > + if (mc->deprecated_reason) { > + continue; > + } > if (mc->alias) { > printf("%-20s %s (alias of %s)\n", mc->alias, mc->desc, mc->name); > } > -- > 2.15.0 >
Hi Thomas, > This series introduces 5 different flavors of deprecation > messages: > > * "Too old" > * "Unmaintained" > * "The ZCU102 machine has the same features supported" > * "Use the \"pc\" machine instead" > * "Obsoleted by the \"40p\" machine" > > Can we clearly document guidelines and examples for values of > this field, to help ensure consistency? > > Examples of questions that could be answered in the field > documentation: > > * Should the message start with an uppercase letter? > * Should it really explain _why_ it was deprecated, or is a > simple "please use xlnx-zcu102 instead" good enough? > * Which of the following is preferred: > * "obsoleted by the \"pc\" machine" > * "obsoleted by \"pc\"" > * "use \"pc\" instead" > * "too old, use \"pc\" instead" > * "too old; use \"pc\" instead" > * <something else?> Do you have any preference regarding Eduardo's suggestions? I see this pattern: - obsoleted by newer -> hint about replacement - too old, unmaintained (maybe suggest use an older version?) -> hint when removal is scheduled These are also valid for Devices. Now if we want a consistent guideline, I suggest we clearly document the deprecated machines/devices in qemu-doc.texi and extract this information at build time, like trace.h. At least this will force developers to document their deprecations.
On 22.01.2018 03:10, Philippe Mathieu-Daudé wrote: > Hi Thomas, > >> This series introduces 5 different flavors of deprecation >> messages: >> >> * "Too old" >> * "Unmaintained" >> * "The ZCU102 machine has the same features supported" >> * "Use the \"pc\" machine instead" >> * "Obsoleted by the \"40p\" machine" >> >> Can we clearly document guidelines and examples for values of >> this field, to help ensure consistency? >> >> Examples of questions that could be answered in the field >> documentation: >> >> * Should the message start with an uppercase letter? >> * Should it really explain _why_ it was deprecated, or is a >> simple "please use xlnx-zcu102 instead" good enough? >> * Which of the following is preferred: >> * "obsoleted by the \"pc\" machine" >> * "obsoleted by \"pc\"" >> * "use \"pc\" instead" >> * "too old, use \"pc\" instead" >> * "too old; use \"pc\" instead" >> * <something else?> > > Do you have any preference regarding Eduardo's suggestions? > > I see this pattern: > > - obsoleted by newer > -> hint about replacement > - too old, unmaintained (maybe suggest use an older version?) > -> hint when removal is scheduled I don't mind the exact wording, as long as there is an indication for the user what to do next (if possible). > These are also valid for Devices. > > Now if we want a consistent guideline, I suggest we clearly document the > deprecated machines/devices in qemu-doc.texi and extract this > information at build time, like trace.h. While it's a must of course to document deprecations in the qemu-doc, automatic extraction already sounds like over-engineering to me. I don't think that we are going to have as much deprecation entries as trace points, so would an additional automatic mechanism really help a lot here? Thomas
diff --git a/include/hw/boards.h b/include/hw/boards.h index 191a5b3cd8..4d5c180968 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -182,6 +182,7 @@ struct MachineClass { const char *default_boot_order; const char *default_display; GArray *compat_props; + const char *deprecated_reason; const char *hw_version; ram_addr_t default_ram_size; const char *default_cpu_type; diff --git a/vl.c b/vl.c index ec299099ff..9ed4648af2 100644 --- a/vl.c +++ b/vl.c @@ -2742,6 +2742,10 @@ static gint machine_class_cmp(gconstpointer a, gconstpointer b) } if (mc) { g_slist_free(machines); + if (mc->deprecated_reason) { + warn_report("The %s machine is deprecated (%s)", + mc->desc, mc->deprecated_reason); + } return mc; } if (name && !is_help_option(name)) { @@ -2752,6 +2756,9 @@ static gint machine_class_cmp(gconstpointer a, gconstpointer b) machines = g_slist_sort(machines, machine_class_cmp); for (el = machines; el; el = el->next) { MachineClass *mc = el->data; + if (mc->deprecated_reason) { + continue; + } if (mc->alias) { printf("%-20s %s (alias of %s)\n", mc->alias, mc->desc, mc->name); }
If a machine has the deprecated_reason property set: - it won't get listed anymore by the "-M help" command, - a warning will be displayed if the machine is used. Example: $ ppc-softmmu/qemu-system-ppc -M prep qemu-system-ppc: -M prep: warning: The PowerPC PREP platform machine is deprecated (Obsoleted by the "40p" machine) [machine starting ...] Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- include/hw/boards.h | 1 + vl.c | 7 +++++++ 2 files changed, 8 insertions(+)