Re: [Qemu-devel] [RFC PATCH 1/6] machine: add a deprecated_reason property

[Eduardo Habkost <ehabkost@redhat.com>]

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
> 

-- 
Eduardo