| Message ID | 20240203080228.2766159-2-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | char: Minor fixes, and a tighter QAPI schema | expand |
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 <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); > } >
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({})
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(-)