[PATCH 0/2] char: Deprecate backend aliases

[PATCH 0/2] char: Deprecate backend aliases

[Kevin Wolf]

These aliases only work the command line, but not in QMP. Command line
QAPIfication involves writing some compatibility glue for them, which
I'm doing, but I think it's desirable to unify accepted values of both
paths. So deprecate the aliases so that we can drop the compatibility
glue later.

In the deprecation documentation I assumed that this is for 6.0, but if
we want to include it in 5.2 still, this can be changed, of course.

Kevin Wolf (2):
  char: Skip CLI aliases in query-chardev-backends
  char: Deprecate backend aliases 'tty' and 'parport'

 docs/system/deprecated.rst |  6 ++++++
 chardev/char.c             | 32 ++++++++++++++++++++++++--------
 2 files changed, 30 insertions(+), 8 deletions(-)

-- 
2.28.0

[PATCH 1/2] char: Skip CLI aliases in query-chardev-backends

[Kevin Wolf]

The aliases "tty" and "parport" are only valid on the command line, QMP
commands like chardev-add don't know them. query-chardev-backends should
describe QMP and therefore not include them in the list of available
backends.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 chardev/char.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index aa4282164a..c406e61db6 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -547,7 +547,7 @@ static const struct ChardevAlias {
 };
 
 typedef struct ChadevClassFE {
-    void (*fn)(const char *name, void *opaque);
+    void (*fn)(const char *name, bool is_cli_alias, void *opaque);
     void *opaque;
 } ChadevClassFE;
 
@@ -561,11 +561,13 @@ chardev_class_foreach(ObjectClass *klass, void *opaque)
         return;
     }
 
-    fe->fn(object_class_get_name(klass) + 8, fe->opaque);
+    fe->fn(object_class_get_name(klass) + 8, false, fe->opaque);
 }
 
 static void
-chardev_name_foreach(void (*fn)(const char *name, void *opaque), void *opaque)
+chardev_name_foreach(void (*fn)(const char *name, bool is_cli_alias,
+                                void *opaque),
+                     void *opaque)
 {
     ChadevClassFE fe = { .fn = fn, .opaque = opaque };
     int i;
@@ -573,12 +575,12 @@ chardev_name_foreach(void (*fn)(const char *name, void *opaque), void *opaque)
     object_class_foreach(chardev_class_foreach, TYPE_CHARDEV, false, &fe);
 
     for (i = 0; i < (int)ARRAY_SIZE(chardev_alias_table); i++) {
-        fn(chardev_alias_table[i].alias, opaque);
+        fn(chardev_alias_table[i].alias, true, opaque);
     }
 }
 
 static void
-help_string_append(const char *name, void *opaque)
+help_string_append(const char *name, bool is_cli_alias, void *opaque)
 {
     GString *str = opaque;
 
@@ -800,11 +802,16 @@ ChardevInfoList *qmp_query_chardev(Error **errp)
 }
 
 static void
-qmp_prepend_backend(const char *name, void *opaque)
+qmp_prepend_backend(const char *name, bool is_cli_alias, void *opaque)
 {
     ChardevBackendInfoList **list = opaque;
-    ChardevBackendInfoList *info = g_malloc0(sizeof(*info));
+    ChardevBackendInfoList *info;
 
+    if (is_cli_alias) {
+        return;
+    }
+
+    info = g_malloc0(sizeof(*info));
     info->value = g_malloc0(sizeof(*info->value));
     info->value->name = g_strdup(name);
     info->next = *list;
-- 
2.28.0

Re: [PATCH 1/2] char: Skip CLI aliases in query-chardev-backends

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> The aliases "tty" and "parport" are only valid on the command line, QMP
> commands like chardev-add don't know them. query-chardev-backends should
> describe QMP and therefore not include them in the list of available
> backends.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

I'd call that a bug.

In the light of PATCH 2, I propose to put that one first (with the
help_string_append() hunk dropped), then remove the aliases from CLI
help, too, like this:


diff --git a/chardev/char.c b/chardev/char.c
index aa4282164a..8b6e78a122 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -568,13 +568,8 @@ static void
 chardev_name_foreach(void (*fn)(const char *name, void *opaque), void *opaque)
 {
     ChadevClassFE fe = { .fn = fn, .opaque = opaque };
-    int i;
 
     object_class_foreach(chardev_class_foreach, TYPE_CHARDEV, false, &fe);
-
-    for (i = 0; i < (int)ARRAY_SIZE(chardev_alias_table); i++) {
-        fn(chardev_alias_table[i].alias, opaque);
-    }
 }
 
 static void

Re: [PATCH 1/2] char: Skip CLI aliases in query-chardev-backends

[Kevin Wolf]

Am 12.11.2020 um 09:22 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > The aliases "tty" and "parport" are only valid on the command line, QMP
> > commands like chardev-add don't know them. query-chardev-backends should
> > describe QMP and therefore not include them in the list of available
> > backends.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> I'd call that a bug.

Which is why I'm fixing it, separate from the deprecation.

> In the light of PATCH 2, I propose to put that one first (with the
> help_string_append() hunk dropped), then remove the aliases from CLI
> help, too, like this: [...]

Going one step back without thinking in solutions immediately, what
you're suggesting is that deprecated options should become undocumented
instead of just annotated as deprecated?

Kevin

Re: [PATCH 1/2] char: Skip CLI aliases in query-chardev-backends

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 12.11.2020 um 09:22 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > The aliases "tty" and "parport" are only valid on the command line, QMP
>> > commands like chardev-add don't know them. query-chardev-backends should
>> > describe QMP and therefore not include them in the list of available
>> > backends.
>> >
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> 
>> I'd call that a bug.
>
> Which is why I'm fixing it, separate from the deprecation.

Thank you :)

>> In the light of PATCH 2, I propose to put that one first (with the
>> help_string_append() hunk dropped), then remove the aliases from CLI
>> help, too, like this: [...]
>
> Going one step back without thinking in solutions immediately, what
> you're suggesting is that deprecated options should become undocumented
> instead of just annotated as deprecated?

"Should become" is open to debate.  "Have become" is a fact:

    3478eae990 "qemu-options: Deprecate -nodefconfig", 2017-10-09
    80f52a6694 "Deprecate -M command line options", 2011-07-23

and more.

I don't see the why *help* output should mention deprecated options.
It's distracting.  Tell me how to use this thing, not how I could use
it, but really shouldn't.

Mentioning them in the reference manual may have value.

Covering them in deprecated.rst is of course mandatory.

[PATCH 2/2] char: Deprecate backend aliases 'tty' and 'parport'

[Kevin Wolf]

QAPI doesn't know the aliases 'tty' and 'parport' and there is no
reason to prefer them to the real names of the backends 'serial' and
'parallel'.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 docs/system/deprecated.rst |  6 ++++++
 chardev/char.c             | 11 ++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index bbaae0d97c..7e313eae4f 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -81,6 +81,12 @@ error in the future.
 The ``-realtime mlock=on|off`` argument has been replaced by the
 ``-overcommit mem-lock=on|off`` argument.
 
+``-chardev`` backend aliases ``tty`` and ``parport`` (since 6.0)
+''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+``tty`` and ``parport`` are aliases that will be removed. Instead, the
+actual backend names ``serial`` and ``parallel`` should be used.
+
 RISC-V ``-bios`` (since 5.1)
 ''''''''''''''''''''''''''''
 
diff --git a/chardev/char.c b/chardev/char.c
index c406e61db6..f9e297185d 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -534,9 +534,10 @@ static const ChardevClass *char_get_class(const char *driver, Error **errp)
     return cc;
 }
 
-static const struct ChardevAlias {
+static struct ChardevAlias {
     const char *typename;
     const char *alias;
+    bool deprecation_warning_printed;
 } chardev_alias_table[] = {
 #ifdef HAVE_CHARDEV_PARPORT
     { "parallel", "parport" },
@@ -585,6 +586,9 @@ help_string_append(const char *name, bool is_cli_alias, void *opaque)
     GString *str = opaque;
 
     g_string_append_printf(str, "\n  %s", name);
+    if (is_cli_alias) {
+        g_string_append(str, " (deprecated)");
+    }
 }
 
 static const char *chardev_alias_translate(const char *name)
@@ -592,6 +596,11 @@ static const char *chardev_alias_translate(const char *name)
     int i;
     for (i = 0; i < (int)ARRAY_SIZE(chardev_alias_table); i++) {
         if (g_strcmp0(chardev_alias_table[i].alias, name) == 0) {
+            if (!chardev_alias_table[i].deprecation_warning_printed) {
+                warn_report("The alias '%s' is deprecated, use '%s' instead",
+                            name, chardev_alias_table[i].typename);
+                chardev_alias_table[i].deprecation_warning_printed = true;
+            }
             return chardev_alias_table[i].typename;
         }
     }
-- 
2.28.0

Re: [PATCH 0/2] char: Deprecate backend aliases

[Paolo Bonzini]

On 11/11/20 14:08, Kevin Wolf wrote:
> These aliases only work the command line, but not in QMP. Command line
> QAPIfication involves writing some compatibility glue for them, which
> I'm doing, but I think it's desirable to unify accepted values of both
> paths. So deprecate the aliases so that we can drop the compatibility
> glue later.
> 
> In the deprecation documentation I assumed that this is for 6.0, but if
> we want to include it in 5.2 still, this can be changed, of course.
> 
> Kevin Wolf (2):
>    char: Skip CLI aliases in query-chardev-backends
>    char: Deprecate backend aliases 'tty' and 'parport'
> 
>   docs/system/deprecated.rst |  6 ++++++
>   chardev/char.c             | 32 ++++++++++++++++++++++++--------
>   2 files changed, 30 insertions(+), 8 deletions(-)
> 

Even though I disagree with QAPIfying -chardev, this one is obviously a 
good thing.

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo