[PATCH 0/4] char: Minor fixes, and a tighter QAPI schema
Markus Armbruster (4): chardev/parallel: Don't close stdin on inappropriate device tests/unit/test-char: Fix qemu_socket(), make_udp_socket() check qapi/char: Make backend types properly conditional qapi/char: Deprecate backend type "memory" docs/about/deprecated.rst | 8 ++++++++ qapi/char.json | 28 +++++++++++++++++----------- include/qemu/osdep.h | 9 ++++++++- chardev/char-parallel.c | 7 +++++-- tests/unit/test-char.c | 25 +++++++++++++++++++++++-- chardev/meson.build | 4 +--- 6 files changed, 62 insertions(+), 19 deletions(-) -- 2.43.0
The __linux__ version of qemu_chr_open_pp_fd() tries to claim the parport device with a PPCLAIM ioctl(). On success, it stores the file descriptor in the chardev object, and returns success. On failure, it closes the file descriptor, and returns failure. chardev_new() then passes the Chardev to object_unref(). This duly calls char_parallel_finalize(), which closes the file descriptor stored in the chardev object. Since qemu_chr_open_pp_fd() didn't store it, it's still zero, so this closes standard input. Ooopsie. To demonstate, add a unit test. With the bug above unfixed, running this test closes standard input. char_hotswap_test() happens to run next. It opens a socket, duly gets file descriptor 0, and since it tests for success with > 0 instead of >= 0, it fails. The test needs to be conditional exactly like the chardev it tests. Since the condition is rather complicated, steal the solution from the serial chardev: define HAVE_CHARDEV_PARALLEL in qemu/osdep.h. This also permits simplifying chardev/meson.build a bit. The bug fix is easy enough: store the file descriptor, and leave closing it to char_parallel_finalize(). Signed-off-by: Markus Armbruster <armbru@redhat.com> --- include/qemu/osdep.h | 9 ++++++++- chardev/char-parallel.c | 7 +++++-- tests/unit/test-char.c | 21 +++++++++++++++++++++ chardev/meson.build | 4 +--- 4 files changed, 35 insertions(+), 6 deletions(-) diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h index c9692cc314..3b0f3389d3 100644 --- a/include/qemu/osdep.h +++ b/include/qemu/osdep.h @@ -508,11 +508,18 @@ void qemu_anon_ram_free(void *ptr, size_t size); #ifdef _WIN32 #define HAVE_CHARDEV_SERIAL 1 -#elif defined(__linux__) || defined(__sun__) || defined(__FreeBSD__) \ +#define HAVE_CHARDEV_PARALLEL 1 +#else +#if defined(__linux__) || defined(__sun__) || defined(__FreeBSD__) \ || defined(__NetBSD__) || defined(__OpenBSD__) || defined(__DragonFly__) \ || defined(__GLIBC__) || defined(__APPLE__) #define HAVE_CHARDEV_SERIAL 1 #endif +#if defined(__linux__) || defined(__FreeBSD__) \ + || defined(__FreeBSD_kernel__) || defined(__DragonFly__) +#define HAVE_CHARDEV_PARALLEL 1 +#endif +#endif #if defined(__HAIKU__) #define SIGIO SIGPOLL diff --git a/chardev/char-parallel.c b/chardev/char-parallel.c index a5164f975a..78697d7522 100644 --- a/chardev/char-parallel.c +++ b/chardev/char-parallel.c @@ -164,13 +164,13 @@ static void qemu_chr_open_pp_fd(Chardev *chr, { ParallelChardev *drv = PARALLEL_CHARDEV(chr); + drv->fd = fd; + if (ioctl(fd, PPCLAIM) < 0) { error_setg_errno(errp, errno, "not a parallel port"); - close(fd); return; } - drv->fd = fd; drv->mode = IEEE1284_MODE_COMPAT; } #endif /* __linux__ */ @@ -238,6 +238,7 @@ static void qemu_chr_open_pp_fd(Chardev *chr, } #endif +#ifdef HAVE_CHARDEV_PARALLEL static void qmp_chardev_open_parallel(Chardev *chr, ChardevBackend *backend, bool *be_opened, @@ -306,3 +307,5 @@ static void register_types(void) } type_init(register_types); + +#endif /* HAVE_CHARDEV_PARALLEL */ diff --git a/tests/unit/test-char.c b/tests/unit/test-char.c index 649fdf64e1..76946e6f90 100644 --- a/tests/unit/test-char.c +++ b/tests/unit/test-char.c @@ -1203,6 +1203,24 @@ static void char_serial_test(void) } #endif +#if defined(HAVE_CHARDEV_PARALLEL) && !defined(WIN32) +static void char_parallel_test(void) +{ + QemuOpts *opts; + Chardev *chr; + + opts = qemu_opts_create(qemu_find_opts("chardev"), "parallel-id", + 1, &error_abort); + qemu_opt_set(opts, "backend", "parallel", &error_abort); + qemu_opt_set(opts, "path", "/dev/null", &error_abort); + + chr = qemu_chr_new_from_opts(opts, NULL, NULL); + g_assert_null(chr); + + qemu_opts_del(opts); +} +#endif + #ifndef _WIN32 static void char_file_fifo_test(void) { @@ -1544,6 +1562,9 @@ int main(int argc, char **argv) g_test_add_func("/char/udp", char_udp_test); #if defined(HAVE_CHARDEV_SERIAL) && !defined(WIN32) g_test_add_func("/char/serial", char_serial_test); +#endif +#if defined(HAVE_CHARDEV_PARALLEL) && !defined(WIN32) + g_test_add_func("/char/parallel", char_parallel_test); #endif g_test_add_func("/char/hotswap", char_hotswap_test); g_test_add_func("/char/websocket", char_websock_test); diff --git a/chardev/meson.build b/chardev/meson.build index c80337d15f..b39028b7b0 100644 --- a/chardev/meson.build +++ b/chardev/meson.build @@ -21,11 +21,9 @@ if host_os == 'windows' else chardev_ss.add(files( 'char-fd.c', + 'char-parallel.c', 'char-pty.c', ), util) - if host_os in ['linux', 'gnu/kfreebsd', 'freebsd', 'dragonfly'] - chardev_ss.add(files('char-parallel.c')) - endif endif chardev_ss = chardev_ss.apply({}) -- 2.43.0
On Sat, Feb 03, 2024 at 09:02:25AM +0100, Markus Armbruster wrote:
The __linux__ version of qemu_chr_open_pp_fd() tries to claim the parport device with a PPCLAIM ioctl(). On success, it stores the file descriptor in the chardev object, and returns success. On failure, it closes the file descriptor, and returns failure.
chardev_new() then passes the Chardev to object_unref(). This duly calls char_parallel_finalize(), which closes the file descriptor stored in the chardev object. Since qemu_chr_open_pp_fd() didn't store it, it's still zero, so this closes standard input. Ooopsie.
To demonstate, add a unit test. With the bug above unfixed, running this test closes standard input. char_hotswap_test() happens to run next. It opens a socket, duly gets file descriptor 0, and since it tests for success with > 0 instead of >= 0, it fails.
Two bugs for the price of one!
The test needs to be conditional exactly like the chardev it tests. Since the condition is rather complicated, steal the solution from the serial chardev: define HAVE_CHARDEV_PARALLEL in qemu/osdep.h. This also permits simplifying chardev/meson.build a bit.
The bug fix is easy enough: store the file descriptor, and leave closing it to char_parallel_finalize().
Signed-off-by: Markus Armbruster <armbru@redhat.com> ---
+++ b/include/qemu/osdep.h @@ -508,11 +508,18 @@ void qemu_anon_ram_free(void *ptr, size_t size);
#ifdef _WIN32 #define HAVE_CHARDEV_SERIAL 1 -#elif defined(__linux__) || defined(__sun__) || defined(__FreeBSD__) \ +#define HAVE_CHARDEV_PARALLEL 1 +#else +#if defined(__linux__) || defined(__sun__) || defined(__FreeBSD__) \ || defined(__NetBSD__) || defined(__OpenBSD__) || defined(__DragonFly__) \ || defined(__GLIBC__) || defined(__APPLE__) #define HAVE_CHARDEV_SERIAL 1 #endif +#if defined(__linux__) || defined(__FreeBSD__) \ + || defined(__FreeBSD_kernel__) || defined(__DragonFly__) +#define HAVE_CHARDEV_PARALLEL 1 +#endif +#endif
Not for this patch, but I've grown to like a preprocessor style I've seen in other projects to make it easier to read nested #if: #ifdef _WIN32 # define HAVE_CHARDEV_SERIAL 1 # define HAVE_CHARDEV_PARALLEL 1 #else # if defined(__linux__) ... defined(__APPLE__) # define HAVE_CHARDEV_SERIAL 1 # endif # if defined(__linux__) ... defined(__DragonFly__) # define HAVE_CHARDEV_PARALLEL 1 # endif #endif
+++ b/chardev/meson.build @@ -21,11 +21,9 @@ if host_os == 'windows' else chardev_ss.add(files( 'char-fd.c', + 'char-parallel.c', 'char-pty.c',
Indentation looks off. Otherwise, Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Eric Blake <eblake@redhat.com> writes:
On Sat, Feb 03, 2024 at 09:02:25AM +0100, Markus Armbruster wrote:
The __linux__ version of qemu_chr_open_pp_fd() tries to claim the parport device with a PPCLAIM ioctl(). On success, it stores the file descriptor in the chardev object, and returns success. On failure, it closes the file descriptor, and returns failure.
chardev_new() then passes the Chardev to object_unref(). This duly calls char_parallel_finalize(), which closes the file descriptor stored in the chardev object. Since qemu_chr_open_pp_fd() didn't store it, it's still zero, so this closes standard input. Ooopsie.
To demonstate, add a unit test. With the bug above unfixed, running this test closes standard input. char_hotswap_test() happens to run next. It opens a socket, duly gets file descriptor 0, and since it tests for success with > 0 instead of >= 0, it fails.
Two bugs for the price of one!
The test needs to be conditional exactly like the chardev it tests. Since the condition is rather complicated, steal the solution from the serial chardev: define HAVE_CHARDEV_PARALLEL in qemu/osdep.h. This also permits simplifying chardev/meson.build a bit.
The bug fix is easy enough: store the file descriptor, and leave closing it to char_parallel_finalize().
Signed-off-by: Markus Armbruster <armbru@redhat.com> ---
+++ b/include/qemu/osdep.h @@ -508,11 +508,18 @@ void qemu_anon_ram_free(void *ptr, size_t size);
#ifdef _WIN32 #define HAVE_CHARDEV_SERIAL 1 -#elif defined(__linux__) || defined(__sun__) || defined(__FreeBSD__) \ +#define HAVE_CHARDEV_PARALLEL 1 +#else +#if defined(__linux__) || defined(__sun__) || defined(__FreeBSD__) \ || defined(__NetBSD__) || defined(__OpenBSD__) || defined(__DragonFly__) \ || defined(__GLIBC__) || defined(__APPLE__) #define HAVE_CHARDEV_SERIAL 1 #endif +#if defined(__linux__) || defined(__FreeBSD__) \ + || defined(__FreeBSD_kernel__) || defined(__DragonFly__) +#define HAVE_CHARDEV_PARALLEL 1 +#endif +#endif
Not for this patch, but I've grown to like a preprocessor style I've seen in other projects to make it easier to read nested #if:
#ifdef _WIN32 # define HAVE_CHARDEV_SERIAL 1 # define HAVE_CHARDEV_PARALLEL 1 #else # if defined(__linux__) ... defined(__APPLE__) # define HAVE_CHARDEV_SERIAL 1 # endif # if defined(__linux__) ... defined(__DragonFly__) # define HAVE_CHARDEV_PARALLEL 1 # endif #endif
I like this style, too.
+++ b/chardev/meson.build @@ -21,11 +21,9 @@ if host_os == 'windows' else chardev_ss.add(files( 'char-fd.c', + 'char-parallel.c', 'char-pty.c',
Indentation looks off. Otherwise,
Will fix.
Reviewed-by: Eric Blake <eblake@redhat.com>
Thanks!
Markus Armbruster <armbru@redhat.com> writes:
The __linux__ version of qemu_chr_open_pp_fd() tries to claim the parport device with a PPCLAIM ioctl(). On success, it stores the file descriptor in the chardev object, and returns success. On failure, it closes the file descriptor, and returns failure.
chardev_new() then passes the Chardev to object_unref(). This duly calls char_parallel_finalize(), which closes the file descriptor stored in the chardev object. Since qemu_chr_open_pp_fd() didn't store it, it's still zero, so this closes standard input. Ooopsie.
To demonstate, add a unit test. With the bug above unfixed, running this test closes standard input. char_hotswap_test() happens to run next. It opens a socket, duly gets file descriptor 0, and since it tests for success with > 0 instead of >= 0, it fails.
The test needs to be conditional exactly like the chardev it tests. Since the condition is rather complicated, steal the solution from the serial chardev: define HAVE_CHARDEV_PARALLEL in qemu/osdep.h. This also permits simplifying chardev/meson.build a bit.
The bug fix is easy enough: store the file descriptor, and leave closing it to char_parallel_finalize().
Signed-off-by: Markus Armbruster <armbru@redhat.com>
[...]
diff --git a/chardev/char-parallel.c b/chardev/char-parallel.c index a5164f975a..78697d7522 100644 --- a/chardev/char-parallel.c +++ b/chardev/char-parallel.c @@ -164,13 +164,13 @@ static void qemu_chr_open_pp_fd(Chardev *chr, { ParallelChardev *drv = PARALLEL_CHARDEV(chr);
+ drv->fd = fd; + if (ioctl(fd, PPCLAIM) < 0) { error_setg_errno(errp, errno, "not a parallel port"); - close(fd); return; }
- drv->fd = fd; drv->mode = IEEE1284_MODE_COMPAT; } #endif /* __linux__ */ @@ -238,6 +238,7 @@ static void qemu_chr_open_pp_fd(Chardev *chr, } #endif
+#ifdef HAVE_CHARDEV_PARALLEL static void qmp_chardev_open_parallel(Chardev *chr, ChardevBackend *backend, bool *be_opened, @@ -306,3 +307,5 @@ static void register_types(void) }
type_init(register_types); + +#endif /* HAVE_CHARDEV_PARALLEL */ diff --git a/tests/unit/test-char.c b/tests/unit/test-char.c index 649fdf64e1..76946e6f90 100644 --- a/tests/unit/test-char.c +++ b/tests/unit/test-char.c @@ -1203,6 +1203,24 @@ static void char_serial_test(void) } #endif
+#if defined(HAVE_CHARDEV_PARALLEL) && !defined(WIN32) +static void char_parallel_test(void) +{ + QemuOpts *opts; + Chardev *chr; + + opts = qemu_opts_create(qemu_find_opts("chardev"), "parallel-id", + 1, &error_abort); + qemu_opt_set(opts, "backend", "parallel", &error_abort); + qemu_opt_set(opts, "path", "/dev/null", &error_abort); + + chr = qemu_chr_new_from_opts(opts, NULL, NULL); + g_assert_null(chr);
This is wrong. On a Linux host, qemu_chr_new_from_opts() fails, because qemu_chr_open_pp_fd()'s attempt to PPCLAIM fails. On a BSD host, it succeeds. Proposed fixup appended. Marc-André, is respinning the PR with the fixup okay, or would you prefer a v2?
+ + qemu_opts_del(opts); +} +#endif + #ifndef _WIN32 static void char_file_fifo_test(void) { @@ -1544,6 +1562,9 @@ int main(int argc, char **argv) g_test_add_func("/char/udp", char_udp_test); #if defined(HAVE_CHARDEV_SERIAL) && !defined(WIN32) g_test_add_func("/char/serial", char_serial_test); +#endif +#if defined(HAVE_CHARDEV_PARALLEL) && !defined(WIN32) + g_test_add_func("/char/parallel", char_parallel_test); #endif g_test_add_func("/char/hotswap", char_hotswap_test); g_test_add_func("/char/websocket", char_websock_test);
[...] diff --git a/tests/unit/test-char.c b/tests/unit/test-char.c index e3b783c06b..f273ce5226 100644 --- a/tests/unit/test-char.c +++ b/tests/unit/test-char.c @@ -1215,7 +1215,13 @@ static void char_parallel_test(void) qemu_opt_set(opts, "path", "/dev/null", &error_abort); chr = qemu_chr_new_from_opts(opts, NULL, NULL); +#ifdef __linux__ + /* fails to PPCLAIM, see qemu_chr_open_pp_fd() */ g_assert_null(chr); +#else + g_assert_nonnull(chr); + object_unparent(OBJECT(chr)); +#endif qemu_opts_del(opts); }
Hi On Tue, Feb 13, 2024 at 5:58 PM Markus Armbruster <armbru@redhat.com> wrote:
Markus Armbruster <armbru@redhat.com> writes:
The __linux__ version of qemu_chr_open_pp_fd() tries to claim the parport device with a PPCLAIM ioctl(). On success, it stores the file descriptor in the chardev object, and returns success. On failure, it closes the file descriptor, and returns failure.
chardev_new() then passes the Chardev to object_unref(). This duly calls char_parallel_finalize(), which closes the file descriptor stored in the chardev object. Since qemu_chr_open_pp_fd() didn't store it, it's still zero, so this closes standard input. Ooopsie.
To demonstate, add a unit test. With the bug above unfixed, running this test closes standard input. char_hotswap_test() happens to run next. It opens a socket, duly gets file descriptor 0, and since it tests for success with > 0 instead of >= 0, it fails.
The test needs to be conditional exactly like the chardev it tests. Since the condition is rather complicated, steal the solution from the serial chardev: define HAVE_CHARDEV_PARALLEL in qemu/osdep.h. This also permits simplifying chardev/meson.build a bit.
The bug fix is easy enough: store the file descriptor, and leave closing it to char_parallel_finalize().
Signed-off-by: Markus Armbruster <armbru@redhat.com>
[...]
diff --git a/chardev/char-parallel.c b/chardev/char-parallel.c index a5164f975a..78697d7522 100644 --- a/chardev/char-parallel.c +++ b/chardev/char-parallel.c @@ -164,13 +164,13 @@ static void qemu_chr_open_pp_fd(Chardev *chr, { ParallelChardev *drv = PARALLEL_CHARDEV(chr);
+ drv->fd = fd; + if (ioctl(fd, PPCLAIM) < 0) { error_setg_errno(errp, errno, "not a parallel port"); - close(fd); return; }
- drv->fd = fd; drv->mode = IEEE1284_MODE_COMPAT; } #endif /* __linux__ */ @@ -238,6 +238,7 @@ static void qemu_chr_open_pp_fd(Chardev *chr, } #endif
+#ifdef HAVE_CHARDEV_PARALLEL static void qmp_chardev_open_parallel(Chardev *chr, ChardevBackend *backend, bool *be_opened, @@ -306,3 +307,5 @@ static void register_types(void) }
type_init(register_types); + +#endif /* HAVE_CHARDEV_PARALLEL */ diff --git a/tests/unit/test-char.c b/tests/unit/test-char.c index 649fdf64e1..76946e6f90 100644 --- a/tests/unit/test-char.c +++ b/tests/unit/test-char.c @@ -1203,6 +1203,24 @@ static void char_serial_test(void) } #endif
+#if defined(HAVE_CHARDEV_PARALLEL) && !defined(WIN32) +static void char_parallel_test(void) +{ + QemuOpts *opts; + Chardev *chr; + + opts = qemu_opts_create(qemu_find_opts("chardev"), "parallel-id", + 1, &error_abort); + qemu_opt_set(opts, "backend", "parallel", &error_abort); + qemu_opt_set(opts, "path", "/dev/null", &error_abort); + + chr = qemu_chr_new_from_opts(opts, NULL, NULL); + g_assert_null(chr);
This is wrong.
On a Linux host, qemu_chr_new_from_opts() fails, because qemu_chr_open_pp_fd()'s attempt to PPCLAIM fails.
On a BSD host, it succeeds.
Proposed fixup appended. Marc-André, is respinning the PR with the fixup okay, or would you prefer a v2?
I am okay with a new PR. thanks
+ + qemu_opts_del(opts); +} +#endif + #ifndef _WIN32 static void char_file_fifo_test(void) { @@ -1544,6 +1562,9 @@ int main(int argc, char **argv) g_test_add_func("/char/udp", char_udp_test); #if defined(HAVE_CHARDEV_SERIAL) && !defined(WIN32) g_test_add_func("/char/serial", char_serial_test); +#endif +#if defined(HAVE_CHARDEV_PARALLEL) && !defined(WIN32) + g_test_add_func("/char/parallel", char_parallel_test); #endif g_test_add_func("/char/hotswap", char_hotswap_test); g_test_add_func("/char/websocket", char_websock_test);
[...]
diff --git a/tests/unit/test-char.c b/tests/unit/test-char.c index e3b783c06b..f273ce5226 100644 --- a/tests/unit/test-char.c +++ b/tests/unit/test-char.c @@ -1215,7 +1215,13 @@ static void char_parallel_test(void) qemu_opt_set(opts, "path", "/dev/null", &error_abort);
chr = qemu_chr_new_from_opts(opts, NULL, NULL); +#ifdef __linux__ + /* fails to PPCLAIM, see qemu_chr_open_pp_fd() */ g_assert_null(chr); +#else + g_assert_nonnull(chr); + object_unparent(OBJECT(chr)); +#endif
qemu_opts_del(opts); }
qemu_socket() and make_udp_socket() return a file descriptor on success, -1 on failure. The check misinterprets 0 as failure. Fix that. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- tests/unit/test-char.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/test-char.c b/tests/unit/test-char.c index 76946e6f90..e3b783c06b 100644 --- a/tests/unit/test-char.c +++ b/tests/unit/test-char.c @@ -556,7 +556,7 @@ static int make_udp_socket(int *port) socklen_t alen = sizeof(addr); int ret, sock = qemu_socket(PF_INET, SOCK_DGRAM, 0); - g_assert_cmpint(sock, >, 0); + g_assert_cmpint(sock, >=, 0); addr.sin_family = AF_INET ; addr.sin_addr.s_addr = htonl(INADDR_ANY); addr.sin_port = 0; @@ -1401,7 +1401,7 @@ static void char_hotswap_test(void) int port; int sock = make_udp_socket(&port); - g_assert_cmpint(sock, >, 0); + g_assert_cmpint(sock, >=, 0); chr_args = g_strdup_printf("udp:127.0.0.1:%d", port); -- 2.43.0
On Sat, Feb 03, 2024 at 09:02:26AM +0100, Markus Armbruster wrote:
qemu_socket() and make_udp_socket() return a file descriptor on success, -1 on failure. The check misinterprets 0 as failure. Fix that.
Signed-off-by: Markus Armbruster <armbru@redhat.com> --- tests/unit/test-char.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> Might be worth amending the commit message of 1/4 where you called out this bug to mention it will be fixed in the next patch. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Eric Blake <eblake@redhat.com> writes:
On Sat, Feb 03, 2024 at 09:02:26AM +0100, Markus Armbruster wrote:
qemu_socket() and make_udp_socket() return a file descriptor on success, -1 on failure. The check misinterprets 0 as failure. Fix that.
Signed-off-by: Markus Armbruster <armbru@redhat.com> --- tests/unit/test-char.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
Might be worth amending the commit message of 1/4 where you called out this bug to mention it will be fixed in the next patch.
Yes. Thanks!
Character backends are actually QOM types. When a backend's compile-time conditional QOM type is not compiled in, creation fails with "'FOO' is not a valid char driver name". Okay, except introspecting chardev-add with query-qmp-schema doesn't work then: the backend type is there even though the QOM type isn't. A management application can work around this issue by using qom-list-types instead. Fix the issue anyway: add the conditionals to the QAPI schema. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- qapi/char.json | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/qapi/char.json b/qapi/char.json index 6c6ad3b10c..2d74e66746 100644 --- a/qapi/char.json +++ b/qapi/char.json @@ -472,8 +472,8 @@ ## { 'enum': 'ChardevBackendKind', 'data': [ 'file', - 'serial', - 'parallel', + { 'name': 'serial', 'if': 'HAVE_CHARDEV_SERIAL' }, + { 'name': 'parallel', 'if': 'HAVE_CHARDEV_PARALLEL' }, 'pipe', 'socket', 'udp', @@ -482,10 +482,10 @@ 'mux', 'msmouse', 'wctablet', - 'braille', + { 'name': 'braille', 'if': 'CONFIG_BRLAPI' }, 'testdev', 'stdio', - 'console', + { 'name': 'console', 'if': 'CONFIG_WIN32' }, { 'name': 'spicevmc', 'if': 'CONFIG_SPICE' }, { 'name': 'spiceport', 'if': 'CONFIG_SPICE' }, { 'name': 'qemu-vdagent', 'if': 'CONFIG_SPICE_PROTOCOL' }, @@ -614,8 +614,10 @@ 'base': { 'type': 'ChardevBackendKind' }, 'discriminator': 'type', 'data': { 'file': 'ChardevFileWrapper', - 'serial': 'ChardevHostdevWrapper', - 'parallel': 'ChardevHostdevWrapper', + 'serial': { 'type': 'ChardevHostdevWrapper', + 'if': 'HAVE_CHARDEV_SERIAL' }, + 'parallel': { 'type': 'ChardevHostdevWrapper', + 'if': 'HAVE_CHARDEV_PARALLEL' }, 'pipe': 'ChardevHostdevWrapper', 'socket': 'ChardevSocketWrapper', 'udp': 'ChardevUdpWrapper', @@ -624,10 +626,12 @@ 'mux': 'ChardevMuxWrapper', 'msmouse': 'ChardevCommonWrapper', 'wctablet': 'ChardevCommonWrapper', - 'braille': 'ChardevCommonWrapper', + 'braille': { 'type': 'ChardevCommonWrapper', + 'if': 'CONFIG_BRLAPI' }, 'testdev': 'ChardevCommonWrapper', 'stdio': 'ChardevStdioWrapper', - 'console': 'ChardevCommonWrapper', + 'console': { 'type': 'ChardevCommonWrapper', + 'if': 'CONFIG_WIN32' }, 'spicevmc': { 'type': 'ChardevSpiceChannelWrapper', 'if': 'CONFIG_SPICE' }, 'spiceport': { 'type': 'ChardevSpicePortWrapper', -- 2.43.0
On Sat, Feb 03, 2024 at 09:02:27AM +0100, Markus Armbruster wrote:
Character backends are actually QOM types. When a backend's compile-time conditional QOM type is not compiled in, creation fails with "'FOO' is not a valid char driver name". Okay, except introspecting chardev-add with query-qmp-schema doesn't work then: the backend type is there even though the QOM type isn't.
A management application can work around this issue by using qom-list-types instead.
Fix the issue anyway: add the conditionals to the QAPI schema.
Signed-off-by: Markus Armbruster <armbru@redhat.com> --- qapi/char.json | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
It's an alias for "ringbuf" we kept for backward compatibility; see commit 3a1da42eb35 (qapi: Rename ChardevBackend member "memory" to "ringbuf"). Deprecation is long overdue. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- docs/about/deprecated.rst | 8 ++++++++ qapi/char.json | 8 +++++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index d4492b9460..ae1a520c26 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -371,6 +371,14 @@ Specifying the iSCSI password in plain text on the command line using the used instead, to refer to a ``--object secret...`` instance that provides a password via a file, or encrypted. +Character device options +'''''''''''''''''''''''' + +Backend ``memory`` (since 9.0) +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +``memory`` is a deprecated synonym for ``ringbuf``. + CPU device properties ''''''''''''''''''''' diff --git a/qapi/char.json b/qapi/char.json index 2d74e66746..75a7e057f0 100644 --- a/qapi/char.json +++ b/qapi/char.json @@ -468,6 +468,10 @@ # # @memory: Since 1.5 # +# Features: +# +# @deprecated: Member @memory is deprecated. Use @ringbuf instead. +# # Since: 1.4 ## { 'enum': 'ChardevBackendKind', @@ -492,8 +496,7 @@ { 'name': 'dbus', 'if': 'CONFIG_DBUS_DISPLAY' }, 'vc', 'ringbuf', - # next one is just for compatibility - 'memory' ] } + { 'name': 'memory', 'features': [ 'deprecated' ] } ] } ## # @ChardevFileWrapper: @@ -642,7 +645,6 @@ 'if': 'CONFIG_DBUS_DISPLAY' }, 'vc': 'ChardevVCWrapper', 'ringbuf': 'ChardevRingbufWrapper', - # next one is just for compatibility 'memory': 'ChardevRingbufWrapper' } } ## -- 2.43.0
On a Saturday in 2024, Markus Armbruster wrote:
It's an alias for "ringbuf" we kept for backward compatibility; see commit 3a1da42eb35 (qapi: Rename ChardevBackend member "memory" to "ringbuf"). Deprecation is long overdue.
Signed-off-by: Markus Armbruster <armbru@redhat.com> --- docs/about/deprecated.rst | 8 ++++++++ qapi/char.json | 8 +++++--- 2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index d4492b9460..ae1a520c26 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -371,6 +371,14 @@ Specifying the iSCSI password in plain text on the command line using the used instead, to refer to a ``--object secret...`` instance that provides a password via a file, or encrypted.
+Character device options +'''''''''''''''''''''''' + +Backend ``memory`` (since 9.0) +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +``memory`` is a deprecated synonym for ``ringbuf``. +
For libvirt: Reviewed-by: Ján Tomko <jtomko@redhat.com> (We don't support either of those) Jano
On Sat, Feb 03, 2024 at 09:02:28AM +0100, Markus Armbruster wrote:
It's an alias for "ringbuf" we kept for backward compatibility; see commit 3a1da42eb35 (qapi: Rename ChardevBackend member "memory" to "ringbuf"). Deprecation is long overdue.
Signed-off-by: Markus Armbruster <armbru@redhat.com> --- docs/about/deprecated.rst | 8 ++++++++ qapi/char.json | 8 +++++--- 2 files changed, 13 insertions(+), 3 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Hi On Sat, Feb 3, 2024 at 12:02 PM Markus Armbruster <armbru@redhat.com> wrote:
Markus Armbruster (4): chardev/parallel: Don't close stdin on inappropriate device tests/unit/test-char: Fix qemu_socket(), make_udp_socket() check qapi/char: Make backend types properly conditional qapi/char: Deprecate backend type "memory"
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
docs/about/deprecated.rst | 8 ++++++++ qapi/char.json | 28 +++++++++++++++++----------- include/qemu/osdep.h | 9 ++++++++- chardev/char-parallel.c | 7 +++++-- tests/unit/test-char.c | 25 +++++++++++++++++++++++-- chardev/meson.build | 4 +--- 6 files changed, 62 insertions(+), 19 deletions(-)
-- 2.43.0
participants (4)
-
Eric Blake -
Ján Tomko -
Marc-André Lureau -
Markus Armbruster