Re: [PATCH 1/4] chardev/parallel: Don't close stdin on inappropriate device

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(a)redhat.com&gt; > --- > +++ 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(a)redhat.com&gt;