[LTP] [PATCH 0/3] Cleanup tst_ioctl.c, libswap.c

[LTP] [PATCH 0/3] Cleanup tst_ioctl.c, libswap.c

[Petr Vorel]

Hi,

this is somehow related to #1091 [1], but independent on it.

I also wonder if we should move tst_fibmap() out of lib/tst_ioctl.c
(it's the only function there) somewhere. include/tst_fs.h is used
several files, I wonder if there should be lib/tst_fs.c.

Maybe not all C files which use include/tst_fs.h should be merged in it,
but some of them contain only single function (e.g. lib/tst_fs_has_free.c).
Is there any benefit to have that separate? Size? In the end we link libltp.a
to every test binary (unfortunately). I also don't think that for
readability purposes of the sources we need 5 C files in lib/ which use
tst_test.h.

Kind regards,
Petr

[1] https://github.com/linux-test-project/ltp/issues/1091

Petr Vorel (3):
  tst_ioctl: Cleanup the code
  tst_fs.h: Improve tst_fibmap() doc
  libltpswap: TCONF on EPERM

 include/tst_fs.h          |  5 ++++-
 lib/tst_ioctl.c           | 18 ++++--------------
 libs/libltpswap/libswap.c | 10 +++++++---
 3 files changed, 15 insertions(+), 18 deletions(-)

-- 
2.42.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 1/3] tst_ioctl: Cleanup the code

[Petr Vorel]

From: Petr Vorel <petr.vorel@gmail.com>

* Use SAFE_{OPEN,CLOSE}() (simplification), -1 return code is removed.
* Cleanup unused headers.
* Include tst_fs.h, to keep signature checked during compilation.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 lib/tst_ioctl.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/lib/tst_ioctl.c b/lib/tst_ioctl.c
index 364220bcd..34a056cf4 100644
--- a/lib/tst_ioctl.c
+++ b/lib/tst_ioctl.c
@@ -1,37 +1,27 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 
-#include <errno.h>
-#include <stdio.h>
-#include <stdlib.h>
 #include <sys/ioctl.h>
 #include <linux/fs.h>
 
 #define TST_NO_DEFAULT_MAIN
 
 #include "tst_test.h"
+#include "tst_fs.h"
 
 int tst_fibmap(const char *filename)
 {
-	/* test if FIBMAP ioctl is supported */
 	int fd, block = 0;
 
-	fd = open(filename, O_RDWR | O_CREAT, 0666);
-	if (fd < 0) {
-		tst_res(TWARN | TERRNO,
-			 "open(%s, O_RDWR | O_CREAT, 0666) failed", filename);
-		return -1;
-	}
+	fd = SAFE_OPEN(filename, O_RDWR | O_CREAT, 0666);
 
 	if (ioctl(fd, FIBMAP, &block)) {
 		tst_res(TINFO | TERRNO, "FIBMAP ioctl is NOT supported");
 		close(fd);
 		return 1;
 	}
+
 	tst_res(TINFO, "FIBMAP ioctl is supported");
+	SAFE_CLOSE(fd);
 
-	if (close(fd)) {
-		tst_res(TWARN | TERRNO, "close(fd) failed");
-		return -1;
-	}
 	return 0;
 }
-- 
2.42.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/3] tst_ioctl: Cleanup the code

[Cyril Hrubis]

Hi!
>  
>  	if (ioctl(fd, FIBMAP, &block)) {
>  		tst_res(TINFO | TERRNO, "FIBMAP ioctl is NOT supported");
>  		close(fd);

This should be SAFE_CLOSE() as well, since we are doing the conversion.

Other than that:

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/3] tst_ioctl: Cleanup the code

[Petr Vorel]

Hi Cyril,

> Hi!

> >  	if (ioctl(fd, FIBMAP, &block)) {
> >  		tst_res(TINFO | TERRNO, "FIBMAP ioctl is NOT supported");
> >  		close(fd);

> This should be SAFE_CLOSE() as well, since we are doing the conversion.

Good point, thanks. I added missing license and merged.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 2/3] tst_fs.h: Improve tst_fibmap() doc

[Petr Vorel]

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 include/tst_fs.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/tst_fs.h b/include/tst_fs.h
index 769fac1e5..3dd0d7524 100644
--- a/include/tst_fs.h
+++ b/include/tst_fs.h
@@ -209,7 +209,10 @@ int tst_fs_in_skiplist(const char *fs_type, const char *const *skiplist);
 void tst_fill_fs(const char *path, int verbose, enum tst_fill_access_pattern pattern);
 
 /*
- * test if FIBMAP ioctl is supported
+ * Check if FIBMAP ioctl is supported.
+ * Tests which use tst_fibmap() should use .needs_root = 1 to avoid EPERM.
+ *
+ * @return 0: FIBMAP is supported, 1: FIBMAP is *not* supported.
  */
 int tst_fibmap(const char *filename);
 
-- 
2.42.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/3] tst_fs.h: Improve tst_fibmap() doc

[Cyril Hrubis]

Hi!
>  /*
> - * test if FIBMAP ioctl is supported
> + * Check if FIBMAP ioctl is supported.
> + * Tests which use tst_fibmap() should use .needs_root = 1 to avoid EPERM.
               ^
	       Maybe just:

	       Tests nees to set .needs_root = 1 in order to avoid EPERM.

> + * @return 0: FIBMAP is supported, 1: FIBMAP is *not* supported.

Otherwise it looks good.

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/3] tst_fs.h: Improve tst_fibmap() doc

[Petr Vorel]

> Hi!
> >  /*
> > - * test if FIBMAP ioctl is supported
> > + * Check if FIBMAP ioctl is supported.
> > + * Tests which use tst_fibmap() should use .needs_root = 1 to avoid EPERM.
>                ^
> 	       Maybe just:

> 	       Tests nees to set .needs_root = 1 in order to avoid EPERM.
+1 (with "needs")

Thanks, merged!

Kind regards,
Petr

> > + * @return 0: FIBMAP is supported, 1: FIBMAP is *not* supported.

> Otherwise it looks good.

> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 3/3] libltpswap: TCONF on EPERM

[Petr Vorel]

Although tests, which use tst_fibmap() should use .needs_root = 1
to avoid EPERM, let's add a sanity check in case it's not used.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 libs/libltpswap/libswap.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c
index a4427736f..4ef24ef5c 100644
--- a/libs/libltpswap/libswap.c
+++ b/libs/libltpswap/libswap.c
@@ -53,10 +53,14 @@ void is_swap_supported(const char *filename)
 
 	TEST(tst_syscall(__NR_swapon, filename, 0));
 	if (TST_RET == -1) {
-		if (fibmap == 1 && errno == EINVAL)
-			tst_brk(TCONF, "Swapfile on %s not implemented", fstype);
-		else
+		if (fibmap == 1) {
+			if (errno == EINVAL)
+				tst_brk(TCONF, "Swapfile on %s not implemented", fstype);
+			if (errno == EPERM)
+				tst_brk(TCONF, "No permission for swapon() on %s", fstype);
+		} else {
 			tst_brk(TFAIL | TTERRNO, "swapon on %s failed", fstype);
+		}
 	}
 
 	TEST(tst_syscall(__NR_swapoff, filename, 0));
-- 
2.42.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 3/3] libltpswap: TCONF on EPERM

[Cyril Hrubis]

Hi!
> +		if (fibmap == 1) {
> +			if (errno == EINVAL)
> +				tst_brk(TCONF, "Swapfile on %s not implemented", fstype);
> +			if (errno == EPERM)
> +				tst_brk(TCONF, "No permission for swapon() on %s", fstype);

I'm not sure if we actually need to print the filesystem type in the
case that we do not have a permission to do swapon.

Also this kind of works because the fibmap ioctl() fails as well,
however I guess that this should really be:

	if (errno == EPERM)
		tst_brk(TCONF, "Permission denied for swapon()");
	else if (fibmap == 1 && errno == EINVAL)
		tst_brk(TCONF, "Swapfile on %s not implemented", fstype);
	else
		tst_brk(TFAIL | TTERNO, ...);

> +		} else {
>  			tst_brk(TFAIL | TTERRNO, "swapon on %s failed", fstype);
> +		}
>  	}
>  
>  	TEST(tst_syscall(__NR_swapoff, filename, 0));
> -- 
> 2.42.0
> 

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 3/3] libltpswap: TCONF on EPERM

[Petr Vorel]

Hi Cyril,

> Hi!
> > +		if (fibmap == 1) {
> > +			if (errno == EINVAL)
> > +				tst_brk(TCONF, "Swapfile on %s not implemented", fstype);
> > +			if (errno == EPERM)
> > +				tst_brk(TCONF, "No permission for swapon() on %s", fstype);

> I'm not sure if we actually need to print the filesystem type in the
> case that we do not have a permission to do swapon.

> Also this kind of works because the fibmap ioctl() fails as well,
> however I guess that this should really be:

> 	if (errno == EPERM)
> 		tst_brk(TCONF, "Permission denied for swapon()");
> 	else if (fibmap == 1 && errno == EINVAL)
> 		tst_brk(TCONF, "Swapfile on %s not implemented", fstype);
> 	else
> 		tst_brk(TFAIL | TTERNO, ...);

Ah, that's actually better, thanks!
Tomorrow, I'll modify it and merge with your RBT.

Kind regards,
Petr

> > +		} else {
> >  			tst_brk(TFAIL | TTERRNO, "swapon on %s failed", fstype);
> > +		}
> >  	}

> >  	TEST(tst_syscall(__NR_swapoff, filename, 0));
> > -- 
> > 2.42.0

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 3/3] libltpswap: TCONF on EPERM

[Petr Vorel]

Hi Cyril,

commit merged, thanks!

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 0/3] Cleanup tst_ioctl.c, libswap.c

[Marius Kittler]

LGTM

Reviewed-by: Marius Kittler <mkittler@suse.de>




-- 
Mailing list info: https://lists.linux.it/listinfo/ltp