Re: [PATCH 2/2] libblkid: reopen floppy without O_NONBLOCK

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

17.01.2022 14:50, Karel Zak wrote:
> On Mon, Jan 03, 2022 at 09:43:11AM +0100, Karel Zak wrote:
>>> Hi! Are you working on this? If not I can try to make a v2.
>>
>> I had a vacation in the last 14 days; nothing is done. Go ahead if you
>> have time for this task. I'm going to work on something else this
>> week.
> 
> It seems you're busy with something else ;-), so I have implemented it,
> but it's not tested yet.

Oh seems yes ;) Great that you've done it!

> 
> Please, please, can you review and test it with your environment?

Ohh. Will try to remember and restore the environment this week :)

> 
> Thanks!
> 
> See the patch bellow or topic/blkid-floppy branch on github.
> 
>   Karel
> 
> 
>  From 49be1b256ad351cde2e0cf5000c7046e7010cc02 Mon Sep 17 00:00:00 2001
> From: Karel Zak <kzak@redhat.com>
> Date: Mon, 17 Jan 2022 12:37:13 +0100
> Subject: [PATCH] libblkid: reopen floppy without O_NONBLOCK
> 
> Vladimir Sementsov-Ogievskiy wrote:
>> The commit "floppy: reintroduce O_NDELAY fix" was removed from kernel,
>> so we faced the bug described and discussed here:
>> https://bugzilla.suse.com/show_bug.cgi?id=3D1181018
>>
>> Discussion in kernel list on reverting the commit:
>> https://www.spinics.net/lists/stable/msg493061.html
>>
>> In short, I can quote Jiri Kosina's comment:
>>
>>    opening floppy device node with O_NONBLOCK is asking for all kinds
>>    of trouble
>>
>> So opening floppy with O_NONBLOCK in blkid leads to failure of blkid,
>> probable failure of mount and unpleasant error messages in dmesg (see
>> also patch 02 for details).
> 
> Based on patch from Vladimir.
> 
> CC: Jiri Kosina <jkosina@suse.cz>
> Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Karel Zak <kzak@redhat.com>
> ---
>   include/fileutils.h  |  3 +++
>   lib/fileutils.c      | 17 +++++++++++++++++
>   libblkid/src/probe.c | 33 +++++++++++++++++++++++++++++++++
>   3 files changed, 53 insertions(+)
> 
> diff --git a/include/fileutils.h b/include/fileutils.h
> index c36ce6353..8722ed59b 100644
> --- a/include/fileutils.h
> +++ b/include/fileutils.h
> @@ -97,4 +97,7 @@ extern void ul_close_all_fds(unsigned int first, unsigned int last);
>   #define UL_COPY_WRITE_ERROR (-2)
>   int ul_copy_file(int from, int to);
>   
> +
> +extern int ul_reopen(int fd, int flags);
> +
>   #endif /* UTIL_LINUX_FILEUTILS */
> diff --git a/lib/fileutils.c b/lib/fileutils.c
> index 7a8fce26f..5b15d4916 100644
> --- a/lib/fileutils.c
> +++ b/lib/fileutils.c
> @@ -288,3 +288,20 @@ int ul_copy_file(int from, int to)
>   	return copy_file_simple(from, to);
>   #endif
>   }
> +
> +int ul_reopen(int fd, int flags)

"reopen" sounds a bit like we are going to close old fd, but we don't... No better suggestion :(

> +{
> +	ssize_t ssz;
> +	char buf[PATH_MAX];
> +	char fdpath[ sizeof(_PATH_PROC_FDDIR) +  sizeof(stringify_value(INT_MAX)) ];

Extra whitespace after '+'

Also, I think you should add +1 for '/' (you use it below), and +1 for finishing \0.

Or alternatively we may just use PATH_MAX here too and not care.

> +
> +	snprintf(fdpath, sizeof(fdpath), _PATH_PROC_FDDIR "/%d", fd);
> +
> +	ssz = readlink(fdpath, buf, sizeof(buf) - 1);
> +	if (ssz <= 0)
> +		return -EINVAL;

alternatively you may return -errno on ssz < 0, and assert that ssz > 0, I hope 0 can't be returned.

> +
> +	buf[ssz] = '\0';
> +
> +	return open(buf, flags);
> +}
> diff --git a/libblkid/src/probe.c b/libblkid/src/probe.c
> index 6168370e2..d69ff530f 100644
> --- a/libblkid/src/probe.c
> +++ b/libblkid/src/probe.c
> @@ -103,6 +103,9 @@
>   #ifdef HAVE_ERRNO_H
>   #include <errno.h>
>   #endif
> +#ifdef HAVE_LINUX_FD_H
> +#include <linux/fd.h>
> +#endif
>   #include <inttypes.h>
>   #include <stdint.h>
>   #include <stdarg.h>
> @@ -113,6 +116,7 @@
>   #include "sysfs.h"
>   #include "strutils.h"
>   #include "list.h"
> +#include "fileutils.h"
>   
>   /*
>    * All supported chains
> @@ -907,6 +911,35 @@ int blkid_probe_set_device(blkid_probe pr, int fd,
>   	if (fd < 0)
>   		return 1;
>   
> +#ifdef FDGETFDCSTAT
> +	{
> +		/*
> +		 * Re-open without O_NONBLOCK for floppy device.
> +		 *
> +		 * Since kernel commit c7e9d0020361f4308a70cdfd6d5335e273eb8717
> +		 * floppy drive works bad when opened with O_NONBLOCK.
> +		 */
> +		struct floppy_fdc_state  flst;

hmm, extra whitespace?

> +
> +		if (ioctl(fd, FDGETFDCSTAT, &flst) >= 0) {
> +			int flags = fcntl(fd, F_GETFL, 0);
> +
> +			if (flags < 0)
> +				goto err;
> +			if (flags & O_NONBLOCK) {
> +				flags &= ~O_NONBLOCK;
> +
> +				fd = ul_reopen(fd, flags | O_CLOEXEC);
> +				if (fd < 0)
> +					goto err;
> +
> +				pr->flags |= BLKID_FL_PRIVATE_FD;
> +				pr->fd = fd;
> +			}
> +		}
> +	}
> +#endif
> +
>   #if defined(POSIX_FADV_RANDOM) && defined(HAVE_POSIX_FADVISE)
>   	/* Disable read-ahead */
>   	posix_fadvise(fd, 0, 0, POSIX_FADV_RANDOM);
> 


Looks good to me. I hope I'll be able also to test it soon.

-- 
Best regards,
Vladimir