[1/1] tst_mkfs: Add -I option to mkfs.vfat

to workaround occasional error on tests with .all_filesystems flag:

tst_test.c:888: TINFO: Formatting /dev/loop0 with vfat opts='' extra opts=''
mkfs.vfat: Partitions or virtual mappings on device '/dev/loop0',
 not making filesystem (use -I to override)
tst_test.c:888: TBROK: mkfs.vfat failed with exit code 1

Tested also on BusyBox v1.33.0, which accept but ignores -I since
beginning (1.14.0).

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Richard also suggested to wipe the device, but not sure how to do it
quickly and don't bring any more dependency.

Kind regards,
Petr

 lib/tst_mkfs.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Hello,

Petr Vorel <pvorel@suse.cz> writes:

> to workaround occasional error on tests with .all_filesystems flag:
>
> tst_test.c:888: TINFO: Formatting /dev/loop0 with vfat opts='' extra opts=''
> mkfs.vfat: Partitions or virtual mappings on device '/dev/loop0',
>  not making filesystem (use -I to override)
> tst_test.c:888: TBROK: mkfs.vfat failed with exit code 1
>
> Tested also on BusyBox v1.33.0, which accept but ignores -I since
> beginning (1.14.0).
>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> Richard also suggested to wipe the device, but not sure how to do it
> quickly and don't bring any more dependency.

I guess that you only need to wipe the headers by writing zeros to the
device. That is unless this is actually refering to a mapping in
/dev/mapper?

>
> Kind regards,
> Petr
>
>  lib/tst_mkfs.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/lib/tst_mkfs.c b/lib/tst_mkfs.c
> index 736324f04..2d335a3ad 100644
> --- a/lib/tst_mkfs.c
> +++ b/lib/tst_mkfs.c
> @@ -52,6 +52,16 @@ void tst_mkfs_(const char *file, const int lineno, void (cleanup_fn)(void),
>  
>  	snprintf(mkfs, sizeof(mkfs), "mkfs.%s", fs_type);
>  
> +	/*
> +	 * Workaround a problem:
> +	 * mkfs.vfat: Partitions or virtual mappings on device '/dev/loop0', not
> +	 * making filesystem (use -I to override)
> +	 */
> +	if (!strcmp(fs_type, "vfat")) {
> +		argv[pos] = "-I";
> +		strcat(fs_opts_str, argv[pos++]);

Doesn't it need a space after -I? Might be better to put this after
fs_opts and then you can check i > 0.

> +	}
> +
>  	if (fs_opts) {
>  		for (i = 0; fs_opts[i]; i++) {
>  			argv[pos++] = fs_opts[i];

On 30. 03. 21 12:00, Richard Palethorpe wrote:
> Hello,
> 
> Petr Vorel <pvorel@suse.cz> writes:
> 
>> to workaround occasional error on tests with .all_filesystems flag:
>>
>> tst_test.c:888: TINFO: Formatting /dev/loop0 with vfat opts='' extra opts=''
>> mkfs.vfat: Partitions or virtual mappings on device '/dev/loop0',
>>  not making filesystem (use -I to override)
>> tst_test.c:888: TBROK: mkfs.vfat failed with exit code 1
>>
>> Tested also on BusyBox v1.33.0, which accept but ignores -I since
>> beginning (1.14.0).
>>
>> Signed-off-by: Petr Vorel <pvorel@suse.cz>
>> ---
>> Richard also suggested to wipe the device, but not sure how to do it
>> quickly and don't bring any more dependency.
> 
> I guess that you only need to wipe the headers by writing zeros to the
> device. That is unless this is actually refering to a mapping in
> /dev/mapper?

We already call tst_clear_device(dev) in tst_mkfs_() but mkfs.vfat keeps
randomly failing with loop devices anyway.

Hi,

...
> > +	/*
> > +	 * Workaround a problem:
> > +	 * mkfs.vfat: Partitions or virtual mappings on device '/dev/loop0', not
> > +	 * making filesystem (use -I to override)
> > +	 */
> > +	if (!strcmp(fs_type, "vfat")) {
> > +		argv[pos] = "-I";
> > +		strcat(fs_opts_str, argv[pos++]);

> Doesn't it need a space after -I?
No, it's char *const argv[] passed to execvp() in tst_cmd_fds_(), which does the
separation. Adding spaces on the contrary causes failure (equivalent of
mkfs.vfat " -I " ...).

> Might be better to put this after fs_opts and then you can check i > 0.
Have I overlooked introduced regression? It works, only it needs to add " "
space for formatting output, but functionality works, adding tested file.

Kind regards,
Petr

Testing file:

#include "tst_test.h"
#define MOUNT_POINT "mount_ext"

static void do_test(void)
{
	tst_res(TPASS, "here");
}

static struct tst_test test = {
	.test_all = do_test,
	.mount_device = 1,
	.mntpoint = MOUNT_POINT,
	.needs_root = 1,
	.dev_fs_type = "vfat",
	.dev_fs_opts = (const char *const []){"-v", NULL},
};

Needed fix:

--- lib/tst_mkfs.c
+++ lib/tst_mkfs.c
@@ -60,6 +60,7 @@ void tst_mkfs_(const char *file, const int lineno, void (cleanup_fn)(void),
 	if (!strcmp(fs_type, "vfat")) {
 		argv[pos] = "-I";
 		strcat(fs_opts_str, argv[pos++]);
+		strcat(fs_opts_str, " ");
 	}
 
 	if (fs_opts) {

Hello,

Petr Vorel <pvorel@suse.cz> writes:

> Hi,
>
> ...
>> > +	/*
>> > +	 * Workaround a problem:
>> > +	 * mkfs.vfat: Partitions or virtual mappings on device '/dev/loop0', not
>> > +	 * making filesystem (use -I to override)
>> > +	 */
>> > +	if (!strcmp(fs_type, "vfat")) {
>> > +		argv[pos] = "-I";
>> > +		strcat(fs_opts_str, argv[pos++]);
>
>> Doesn't it need a space after -I?
> No, it's char *const argv[] passed to execvp() in tst_cmd_fds_(), which does the
> separation. Adding spaces on the contrary causes failure (equivalent of
> mkfs.vfat " -I " ...).
>
>> Might be better to put this after fs_opts and then you can check i > 0.
> Have I overlooked introduced regression? It works, only it needs to add " "
> space for formatting output, but functionality works, adding tested
> file.

Ah, well given Martin's comment as well, this LGTM.

>
> Kind regards,
> Petr
>
> Testing file:
>
> #include "tst_test.h"
> #define MOUNT_POINT "mount_ext"
>
> static void do_test(void)
> {
> 	tst_res(TPASS, "here");
> }
>
> static struct tst_test test = {
> 	.test_all = do_test,
> 	.mount_device = 1,
> 	.mntpoint = MOUNT_POINT,
> 	.needs_root = 1,
> 	.dev_fs_type = "vfat",
> 	.dev_fs_opts = (const char *const []){"-v", NULL},
> };
>
> Needed fix:
>
> --- lib/tst_mkfs.c
> +++ lib/tst_mkfs.c
> @@ -60,6 +60,7 @@ void tst_mkfs_(const char *file, const int lineno, void (cleanup_fn)(void),
>  	if (!strcmp(fs_type, "vfat")) {
>  		argv[pos] = "-I";
>  		strcat(fs_opts_str, argv[pos++]);
> +		strcat(fs_opts_str, " ");
>  	}
>  
>  	if (fs_opts) {

Hi,

instead of using -I I tried Martin Doucha's suggestion to rescan partition table
with ioctl BLKRRPART. But I get EINVAL. Not sure if O_RDONLY is a proper flag,
but I also tried to open with args used for manipulating loop (open(path,
O_CREAT|O_WRONLY|O_TRUNC, S_IRUSR|S_IWUSR)) and a same issue. Happens on recent
kernel and very old kernel 3.16.

I wonder if I have wrong flags or BLKRRPART is just not supported on loop
devices. Looking into kernel sources it's this part blkdev_reread_part():

	if (!disk_part_scan_enabled(bdev->bd_disk) || bdev_is_partition(bdev))
		return -EINVAL;
Kind regards,
Petr

diff --git lib/tst_mkfs.c lib/tst_mkfs.c
index 736324f04..06fafcd18 100644
--- lib/tst_mkfs.c
+++ lib/tst_mkfs.c
@@ -15,6 +15,10 @@
  * along with this program. If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <sys/ioctl.h>
+#include <sys/mount.h>
+#include <fcntl.h>
+
 #include "test.h"
 #include "ltp_priv.h"
 #include "tst_mkfs.h"
@@ -26,7 +30,7 @@ void tst_mkfs_(const char *file, const int lineno, void (cleanup_fn)(void),
 	       const char *dev, const char *fs_type,
 	       const char *const fs_opts[], const char *const extra_opts[])
 {
-	int i, pos = 1, ret;
+	int fd, i, pos = 1, ret;
 	char mkfs[64];
 	const char *argv[OPTS_MAX] = {mkfs};
 	char fs_opts_str[1024] = "";
@@ -93,6 +97,13 @@ void tst_mkfs_(const char *file, const int lineno, void (cleanup_fn)(void),
 			"tst_clear_device() failed");
 	}
 
+	/* force kernel to reread partition table */
+	fd = open(dev, O_RDONLY);
+	tst_resm_(file, lineno, TINFO, "dev: %s, fd: %d", dev, fd);
+	ret = ioctl(fd, BLKRRPART);
+	tst_resm_(file, lineno, TINFO, "rc: %d", ret);
+	close(fd);
+
 	tst_resm_(file, lineno, TINFO,
 		"Formatting %s with %s opts='%s' extra opts='%s'",
 		dev, fs_type, fs_opts_str, extra_opts_str);