[PATCH 0/2] char: Deprecate backend aliases
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
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
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
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
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.
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
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
participants (3)
-
Kevin Wolf -
Markus Armbruster -
Paolo Bonzini