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!