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

[Markus Armbruster <armbru@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!