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

Commit Message

Kevin Wolf Nov. 11, 2020, 1:08 p.m. UTC

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 <[e-mail]>
---
 chardev/char.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

Comments

Markus Armbruster Nov. 12, 2020, 8:22 a.m. UTC | #1

Kevin Wolf <[e-mail]> 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 <[e-mail]>

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

Kevin Wolf Nov. 12, 2020, 10:58 a.m. UTC | #2

Am 12.11.2020 um 09:22 hat Markus Armbruster geschrieben:
> Kevin Wolf <[e-mail]> 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 <[e-mail]>
> 
> 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

Markus Armbruster Nov. 13, 2020, 8:01 a.m. UTC | #3

Kevin Wolf <[e-mail]> writes:

> Am 12.11.2020 um 09:22 hat Markus Armbruster geschrieben:
>> Kevin Wolf <[e-mail]> 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 <[e-mail]>
>> 
>> 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

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;