All of lore.kernel.org
 help / color / mirror / Atom feed
* [LTP] [PATCH] statvfs01: Convert to new LTP API
@ 2022-11-24 11:42 Avinesh Kumar
  2022-11-25  2:18 ` Li Wang
  0 siblings, 1 reply; 15+ messages in thread
From: Avinesh Kumar @ 2022-11-24 11:42 UTC (permalink / raw)
  To: ltp

Also I've removed the TINFO statements, I'm not sure if only
printing the data in logs is helpful in anyway.

Signed-off-by: Avinesh Kumar <akumar@suse.de>
---
 testcases/kernel/syscalls/statvfs/statvfs01.c | 96 +++++--------------
 1 file changed, 22 insertions(+), 74 deletions(-)

diff --git a/testcases/kernel/syscalls/statvfs/statvfs01.c b/testcases/kernel/syscalls/statvfs/statvfs01.c
index e3b356c93..89ca4e960 100644
--- a/testcases/kernel/syscalls/statvfs/statvfs01.c
+++ b/testcases/kernel/syscalls/statvfs/statvfs01.c
@@ -1,92 +1,40 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Copyright (c) Wipro Technologies Ltd, 2005.  All Rights Reserved.
  *    AUTHOR: Prashant P Yendigeri <prashant.yendigeri@wipro.com>
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of version 2 of the GNU General Public License as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it would be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
- *
- * You should have received a copy of the GNU General Public License along
- * with this program; if not, write the Free Software Foundation, Inc.,
- * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
- *
+ * Copyright (c) 2022 SUSE LLC Avinesh Kumar <avinesh.kumar@suse.com>
  */
-/*
- *    DESCRIPTION
- *      This is a Phase I test for the statvfs(2) system call.
- *      It is intended to provide a limited exposure of the system call.
- *	This call behaves similar to statfs.
+
+/*\
+ * [Description]
+ *
+ * Verify that statvfs() executes successfully for all
+ * available filesystems.
  */
 
-#include <stdio.h>
-#include <unistd.h>
-#include <errno.h>
 #include <sys/statvfs.h>
-#include <stdint.h>
-
-#include "test.h"
-
-#define TEST_PATH "/"
+#include "tst_test.h"
 
-static void setup(void);
-static void cleanup(void);
+#define MNT_POINT "mntpoint"
+#define TEST_PATH MNT_POINT"/testfile"
 
-char *TCID = "statvfs01";
-int TST_TOTAL = 1;
-
-int main(int ac, char **av)
+static void run(void)
 {
 	struct statvfs buf;
-	int lc;
-
-	tst_parse_opts(ac, av, NULL, NULL);
 
-	setup();
-
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-
-		tst_count = 0;
-
-		TEST(statvfs(TEST_PATH, &buf));
-
-		if (TEST_RETURN == -1) {
-			tst_resm(TFAIL | TTERRNO, "statvfs(%s, ...) failed",
-				 TEST_PATH);
-		} else {
-			tst_resm(TPASS, "statvfs(%s, ...) passed", TEST_PATH);
-		}
-
-	}
-
-	tst_resm(TINFO, "This call is similar to statfs");
-	tst_resm(TINFO, "Extracting info about the '%s' file system",
-		 TEST_PATH);
-	tst_resm(TINFO, "file system block size = %lu bytes", buf.f_bsize);
-	tst_resm(TINFO, "file system fragment size = %lu bytes", buf.f_frsize);
-	tst_resm(TINFO, "file system free blocks = %ju",
-		 (uintmax_t) buf.f_bfree);
-	tst_resm(TINFO, "file system total inodes = %ju",
-		 (uintmax_t) buf.f_files);
-	tst_resm(TINFO, "file system free inodes = %ju",
-		 (uintmax_t) buf.f_ffree);
-	tst_resm(TINFO, "file system id = %lu", buf.f_fsid);
-	tst_resm(TINFO, "file system max filename length = %lu", buf.f_namemax);
-
-	cleanup();
-	tst_exit();
+	TST_EXP_PASS(statvfs(TEST_PATH, &buf));
 }
 
 static void setup(void)
 {
-	tst_sig(NOFORK, DEF_HANDLER, cleanup);
-
-	TEST_PAUSE;
+	SAFE_TOUCH(TEST_PATH, 0666, NULL);
 }
 
-static void cleanup(void)
-{
-}
+static struct tst_test test = {
+	.test_all = run,
+	.setup = setup,
+	.needs_root = 1,
+	.mount_device = 1,
+	.mntpoint = MNT_POINT,
+	.all_filesystems = 1
+};
-- 
2.38.1


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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [LTP] [PATCH] statvfs01: Convert to new LTP API
  2022-11-24 11:42 [LTP] [PATCH] statvfs01: Convert to new LTP API Avinesh Kumar
@ 2022-11-25  2:18 ` Li Wang
  2022-11-29 10:58   ` Richard Palethorpe
  0 siblings, 1 reply; 15+ messages in thread
From: Li Wang @ 2022-11-25  2:18 UTC (permalink / raw)
  To: Avinesh Kumar; +Cc: ltp


[-- Attachment #1.1: Type: text/plain, Size: 5595 bytes --]

On Thu, Nov 24, 2022 at 7:42 PM Avinesh Kumar <akumar@suse.de> wrote:

> Also I've removed the TINFO statements, I'm not sure if only
> printing the data in logs is helpful in anyway.
>

Removing the printing is OK, but I am just wondering that
can we find a way to check if the returned value in 'buf' is
indeed correct?

As you can see the ‘struct statvfs‘ includes many fs key
values that we need to trust when using them.

struct statvfs {
        unsigned long  f_bsize;    /* Filesystem block size */
        unsigned long  f_frsize;   /* Fragment size */
        fsblkcnt_t     f_blocks;   /* Size of fs in f_frsize units */
        fsblkcnt_t     f_bfree;    /* Number of free blocks */
        fsblkcnt_t     f_bavail;   /* Number of free blocks for
                                             unprivileged users */
        fsfilcnt_t     f_files;    /* Number of inodes */
        fsfilcnt_t     f_ffree;    /* Number of free inodes */
        fsfilcnt_t     f_favail;   /* Number of free inodes for
                                             unprivileged users */
        unsigned long  f_fsid;     /* Filesystem ID */
        unsigned long  f_flag;     /* Mount flags */
        unsigned long  f_namemax;  /* Maximum filename length */
 };


>
> Signed-off-by: Avinesh Kumar <akumar@suse.de>
> ---
>  testcases/kernel/syscalls/statvfs/statvfs01.c | 96 +++++--------------
>  1 file changed, 22 insertions(+), 74 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/statvfs/statvfs01.c
> b/testcases/kernel/syscalls/statvfs/statvfs01.c
> index e3b356c93..89ca4e960 100644
> --- a/testcases/kernel/syscalls/statvfs/statvfs01.c
> +++ b/testcases/kernel/syscalls/statvfs/statvfs01.c
> @@ -1,92 +1,40 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * Copyright (c) Wipro Technologies Ltd, 2005.  All Rights Reserved.
>   *    AUTHOR: Prashant P Yendigeri <prashant.yendigeri@wipro.com>
> - *
> - * This program is free software; you can redistribute it and/or modify it
> - * under the terms of version 2 of the GNU General Public License as
> - * published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it would be useful, but
> - * WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> - *
> - * You should have received a copy of the GNU General Public License along
> - * with this program; if not, write the Free Software Foundation, Inc.,
> - * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> - *
> + * Copyright (c) 2022 SUSE LLC Avinesh Kumar <avinesh.kumar@suse.com>
>   */
> -/*
> - *    DESCRIPTION
> - *      This is a Phase I test for the statvfs(2) system call.
> - *      It is intended to provide a limited exposure of the system call.
> - *     This call behaves similar to statfs.
> +
> +/*\
> + * [Description]
> + *
> + * Verify that statvfs() executes successfully for all
> + * available filesystems.
>   */
>
> -#include <stdio.h>
> -#include <unistd.h>
> -#include <errno.h>
>  #include <sys/statvfs.h>
> -#include <stdint.h>
> -
> -#include "test.h"
> -
> -#define TEST_PATH "/"
> +#include "tst_test.h"
>
> -static void setup(void);
> -static void cleanup(void);
> +#define MNT_POINT "mntpoint"
> +#define TEST_PATH MNT_POINT"/testfile"
>
> -char *TCID = "statvfs01";
> -int TST_TOTAL = 1;
> -
> -int main(int ac, char **av)
> +static void run(void)
>  {
>         struct statvfs buf;
> -       int lc;
> -
> -       tst_parse_opts(ac, av, NULL, NULL);
>
> -       setup();
> -
> -       for (lc = 0; TEST_LOOPING(lc); lc++) {
> -
> -               tst_count = 0;
> -
> -               TEST(statvfs(TEST_PATH, &buf));
> -
> -               if (TEST_RETURN == -1) {
> -                       tst_resm(TFAIL | TTERRNO, "statvfs(%s, ...)
> failed",
> -                                TEST_PATH);
> -               } else {
> -                       tst_resm(TPASS, "statvfs(%s, ...) passed",
> TEST_PATH);
> -               }
> -
> -       }
> -
> -       tst_resm(TINFO, "This call is similar to statfs");
> -       tst_resm(TINFO, "Extracting info about the '%s' file system",
> -                TEST_PATH);
> -       tst_resm(TINFO, "file system block size = %lu bytes", buf.f_bsize);
> -       tst_resm(TINFO, "file system fragment size = %lu bytes",
> buf.f_frsize);
> -       tst_resm(TINFO, "file system free blocks = %ju",
> -                (uintmax_t) buf.f_bfree);
> -       tst_resm(TINFO, "file system total inodes = %ju",
> -                (uintmax_t) buf.f_files);
> -       tst_resm(TINFO, "file system free inodes = %ju",
> -                (uintmax_t) buf.f_ffree);
> -       tst_resm(TINFO, "file system id = %lu", buf.f_fsid);
> -       tst_resm(TINFO, "file system max filename length = %lu",
> buf.f_namemax);
> -
> -       cleanup();
> -       tst_exit();
> +       TST_EXP_PASS(statvfs(TEST_PATH, &buf));
>  }
>
>  static void setup(void)
>  {
> -       tst_sig(NOFORK, DEF_HANDLER, cleanup);
> -
> -       TEST_PAUSE;
> +       SAFE_TOUCH(TEST_PATH, 0666, NULL);
>  }
>
> -static void cleanup(void)
> -{
> -}
> +static struct tst_test test = {
> +       .test_all = run,
> +       .setup = setup,
> +       .needs_root = 1,
> +       .mount_device = 1,
> +       .mntpoint = MNT_POINT,
> +       .all_filesystems = 1
> +};
> --
> 2.38.1
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
>

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 8487 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [LTP] [PATCH] statvfs01: Convert to new LTP API
  2022-11-25  2:18 ` Li Wang
@ 2022-11-29 10:58   ` Richard Palethorpe
  2022-11-30  7:05     ` [LTP] [PATCH v2] " Avinesh Kumar
  2022-11-30  7:20     ` [LTP] [PATCH] " Li Wang
  0 siblings, 2 replies; 15+ messages in thread
From: Richard Palethorpe @ 2022-11-29 10:58 UTC (permalink / raw)
  To: Li Wang; +Cc: ltp

Hello,

Li Wang <liwang@redhat.com> writes:

> On Thu, Nov 24, 2022 at 7:42 PM Avinesh Kumar <akumar@suse.de> wrote:
>
>  Also I've removed the TINFO statements, I'm not sure if only
>  printing the data in logs is helpful in anyway.
>
> Removing the printing is OK, but I am just wondering that
> can we find a way to check if the returned value in 'buf' is
> indeed correct?
>
> As you can see the ‘struct statvfs‘ includes many fs key
> values that we need to trust when using them.
>
> struct statvfs {
>         unsigned long  f_bsize;    /* Filesystem block size */
>         unsigned long  f_frsize;   /* Fragment size */
>         fsblkcnt_t     f_blocks;   /* Size of fs in f_frsize units */
>         fsblkcnt_t     f_bfree;    /* Number of free blocks */
>         fsblkcnt_t     f_bavail;   /* Number of free blocks for
>                                              unprivileged users */
>         fsfilcnt_t     f_files;    /* Number of inodes */
>         fsfilcnt_t     f_ffree;    /* Number of free inodes */
>         fsfilcnt_t     f_favail;   /* Number of free inodes for
>                                              unprivileged users */
>         unsigned long  f_fsid;     /* Filesystem ID */
>         unsigned long  f_flag;     /* Mount flags */
>         unsigned long  f_namemax;  /* Maximum filename length */
>  };

I suppose previously printing the values at least accessed the memory.
Actually validating the values could be a separate patch though.

We (probably) know that maximum filename should be less than 255 chars
(for e.g.), but I think there is a good chance that trying to validate
this will result in false positives and stuff we might want to revert.

-- 
Thank you,
Richard.

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [LTP] [PATCH v2] statvfs01: Convert to new LTP API
  2022-11-29 10:58   ` Richard Palethorpe
@ 2022-11-30  7:05     ` Avinesh Kumar
  2022-11-30  8:52       ` Petr Vorel
  2022-11-30  7:20     ` [LTP] [PATCH] " Li Wang
  1 sibling, 1 reply; 15+ messages in thread
From: Avinesh Kumar @ 2022-11-30  7:05 UTC (permalink / raw)
  To: ltp

Removed the TINFO statements,
Added a validation of statvfs.f_namemax field by trying to create
files of valid and invalid length names.

Signed-off-by: Avinesh Kumar <akumar@suse.de>
---
 testcases/kernel/syscalls/statvfs/statvfs01.c | 106 ++++++------------
 1 file changed, 34 insertions(+), 72 deletions(-)

diff --git a/testcases/kernel/syscalls/statvfs/statvfs01.c b/testcases/kernel/syscalls/statvfs/statvfs01.c
index e3b356c93..94e17ba1d 100644
--- a/testcases/kernel/syscalls/statvfs/statvfs01.c
+++ b/testcases/kernel/syscalls/statvfs/statvfs01.c
@@ -1,92 +1,54 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Copyright (c) Wipro Technologies Ltd, 2005.  All Rights Reserved.
  *    AUTHOR: Prashant P Yendigeri <prashant.yendigeri@wipro.com>
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of version 2 of the GNU General Public License as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it would be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
- *
- * You should have received a copy of the GNU General Public License along
- * with this program; if not, write the Free Software Foundation, Inc.,
- * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
- *
+ * Copyright (c) 2022 SUSE LLC Avinesh Kumar <avinesh.kumar@suse.com>
  */
-/*
- *    DESCRIPTION
- *      This is a Phase I test for the statvfs(2) system call.
- *      It is intended to provide a limited exposure of the system call.
- *	This call behaves similar to statfs.
+
+/*\
+ * [Description]
+ *
+ * Verify that statvfs() executes successfully for all
+ * available filesystems.
  */
 
-#include <stdio.h>
-#include <unistd.h>
-#include <errno.h>
 #include <sys/statvfs.h>
-#include <stdint.h>
-
-#include "test.h"
-
-#define TEST_PATH "/"
+#include "tst_test.h"
 
-static void setup(void);
-static void cleanup(void);
+#define MNT_POINT "mntpoint"
+#define TEST_PATH MNT_POINT"/testfile"
 
-char *TCID = "statvfs01";
-int TST_TOTAL = 1;
-
-int main(int ac, char **av)
+static void run(void)
 {
 	struct statvfs buf;
-	int lc;
-
-	tst_parse_opts(ac, av, NULL, NULL);
+	char *valid_fname, *invalid_fname;
 
-	setup();
+	TST_EXP_PASS(statvfs(TEST_PATH, &buf));
 
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
+	valid_fname = SAFE_MALLOC(buf.f_namemax - 1);
+	invalid_fname = SAFE_MALLOC(buf.f_namemax + 1);
+	memset(valid_fname, 'a', buf.f_namemax - 1);
+	memset(invalid_fname, 'b', buf.f_namemax + 1);
 
-		tst_count = 0;
-
-		TEST(statvfs(TEST_PATH, &buf));
-
-		if (TEST_RETURN == -1) {
-			tst_resm(TFAIL | TTERRNO, "statvfs(%s, ...) failed",
-				 TEST_PATH);
-		} else {
-			tst_resm(TPASS, "statvfs(%s, ...) passed", TEST_PATH);
-		}
-
-	}
-
-	tst_resm(TINFO, "This call is similar to statfs");
-	tst_resm(TINFO, "Extracting info about the '%s' file system",
-		 TEST_PATH);
-	tst_resm(TINFO, "file system block size = %lu bytes", buf.f_bsize);
-	tst_resm(TINFO, "file system fragment size = %lu bytes", buf.f_frsize);
-	tst_resm(TINFO, "file system free blocks = %ju",
-		 (uintmax_t) buf.f_bfree);
-	tst_resm(TINFO, "file system total inodes = %ju",
-		 (uintmax_t) buf.f_files);
-	tst_resm(TINFO, "file system free inodes = %ju",
-		 (uintmax_t) buf.f_ffree);
-	tst_resm(TINFO, "file system id = %lu", buf.f_fsid);
-	tst_resm(TINFO, "file system max filename length = %lu", buf.f_namemax);
-
-	cleanup();
-	tst_exit();
+	TST_EXP_FD(creat(valid_fname, 0444));
+	TST_EXP_FAIL(creat(invalid_fname, 0444), ENAMETOOLONG);
 }
 
 static void setup(void)
 {
-	tst_sig(NOFORK, DEF_HANDLER, cleanup);
-
-	TEST_PAUSE;
+	SAFE_TOUCH(TEST_PATH, 0666, NULL);
 }
 
-static void cleanup(void)
-{
-}
+static struct tst_test test = {
+	.test_all = run,
+	.setup = setup,
+	.needs_root = 1,
+	.mount_device = 1,
+	.mntpoint = MNT_POINT,
+	.all_filesystems = 1,
+	.skip_filesystems = (const char *const[]) {
+		"vfat",
+		"exfat",
+		NULL
+	}
+};
-- 
2.38.1


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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [LTP] [PATCH] statvfs01: Convert to new LTP API
  2022-11-29 10:58   ` Richard Palethorpe
  2022-11-30  7:05     ` [LTP] [PATCH v2] " Avinesh Kumar
@ 2022-11-30  7:20     ` Li Wang
  2022-12-01  6:00       ` Li Wang
  1 sibling, 1 reply; 15+ messages in thread
From: Li Wang @ 2022-11-30  7:20 UTC (permalink / raw)
  To: rpalethorpe; +Cc: ltp


[-- Attachment #1.1: Type: text/plain, Size: 2188 bytes --]

On Tue, Nov 29, 2022 at 7:09 PM Richard Palethorpe <rpalethorpe@suse.de>
wrote:

> Hello,
>
> Li Wang <liwang@redhat.com> writes:
>
> > On Thu, Nov 24, 2022 at 7:42 PM Avinesh Kumar <akumar@suse.de> wrote:
> >
> >  Also I've removed the TINFO statements, I'm not sure if only
> >  printing the data in logs is helpful in anyway.
> >
> > Removing the printing is OK, but I am just wondering that
> > can we find a way to check if the returned value in 'buf' is
> > indeed correct?
> >
> > As you can see the ‘struct statvfs‘ includes many fs key
> > values that we need to trust when using them.
> >
> > struct statvfs {
> >         unsigned long  f_bsize;    /* Filesystem block size */
> >         unsigned long  f_frsize;   /* Fragment size */
> >         fsblkcnt_t     f_blocks;   /* Size of fs in f_frsize units */
> >         fsblkcnt_t     f_bfree;    /* Number of free blocks */
> >         fsblkcnt_t     f_bavail;   /* Number of free blocks for
> >                                              unprivileged users */
> >         fsfilcnt_t     f_files;    /* Number of inodes */
> >         fsfilcnt_t     f_ffree;    /* Number of free inodes */
> >         fsfilcnt_t     f_favail;   /* Number of free inodes for
> >                                              unprivileged users */
> >         unsigned long  f_fsid;     /* Filesystem ID */
> >         unsigned long  f_flag;     /* Mount flags */
> >         unsigned long  f_namemax;  /* Maximum filename length */
> >  };
>
> I suppose previously printing the values at least accessed the memory.
>

Sounds reasonable.



> Actually validating the values could be a separate patch though.
>

+1 Absolutely.



>
> We (probably) know that maximum filename should be less than 255 chars
> (for e.g.), but I think there is a good chance that trying to validate
> this will result in false positives and stuff we might want to revert.
>

Maybe we can create a concrete size of the device and mount
it with a designated FS (e.g. ext4), then extracting the known FS
data into `struct statvfs` and validating them.

Does this make sense?


-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 3790 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [LTP] [PATCH v2] statvfs01: Convert to new LTP API
  2022-11-30  7:05     ` [LTP] [PATCH v2] " Avinesh Kumar
@ 2022-11-30  8:52       ` Petr Vorel
  2022-11-30  9:50         ` Petr Vorel
  0 siblings, 1 reply; 15+ messages in thread
From: Petr Vorel @ 2022-11-30  8:52 UTC (permalink / raw)
  To: Avinesh Kumar; +Cc: Richard Palethorpe, ltp

Hi Avinesh,

> Removed the TINFO statements,
> Added a validation of statvfs.f_namemax field by trying to create
> files of valid and invalid length names.
Very nice rewrite, thanks for adding this veryfication.
It'd be nice to update this in docparse description.

> +
> +/*\
> + * [Description]
> + *
> + * Verify that statvfs() executes successfully for all
> + * available filesystems.
e.g.:
Verify that statvfs() executes successfully for all
available filesystems. Verify statvfs.f_namemax field
by trying to create files of valid and invalid length names.

I can merge it with this change.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

...
> +static struct tst_test test = {
> +	.test_all = run,
> +	.setup = setup,
> +	.needs_root = 1,
> +	.mount_device = 1,
> +	.mntpoint = MNT_POINT,
> +	.all_filesystems = 1,
> +	.skip_filesystems = (const char *const[]) {
> +		"vfat",
> +		"exfat",
I was looking what's wrong with vfat and exfat.
statvfs.f_namemax returns 1530, which is obviously too long, thus valid_fname
obviously returns ENAMETOOLONG (36). Tested on 6.1.0-rc6-1.g4c01546-default.
I wonder why, isn't that a bug?

Kind regards,
Petr

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [LTP] [PATCH v2] statvfs01: Convert to new LTP API
  2022-11-30  8:52       ` Petr Vorel
@ 2022-11-30  9:50         ` Petr Vorel
  2022-12-01  5:16           ` Li Wang
  2022-12-01  8:51           ` Avinesh Kumar
  0 siblings, 2 replies; 15+ messages in thread
From: Petr Vorel @ 2022-11-30  9:50 UTC (permalink / raw)
  To: Avinesh Kumar, Richard Palethorpe, ltp

Hi all,

...
> > +static struct tst_test test = {
> > +	.test_all = run,
> > +	.setup = setup,
> > +	.needs_root = 1,
> > +	.mount_device = 1,
> > +	.mntpoint = MNT_POINT,
> > +	.all_filesystems = 1,
> > +	.skip_filesystems = (const char *const[]) {
> > +		"vfat",
> > +		"exfat",
> I was looking what's wrong with vfat and exfat.
> statvfs.f_namemax returns 1530, which is obviously too long, thus valid_fname
> obviously returns ENAMETOOLONG (36). Tested on 6.1.0-rc6-1.g4c01546-default.
> I wonder why, isn't that a bug?

To reply myself, both glibc and musl defines:
statvfs->f_namemax = statfs->f_namelen;

TL;DR: 6 * 255 = 1530 due names being in UTF-8:

Therefore looking into kernel sources for statfs->f_namelen:

include/linux/nls.h
#define NLS_MAX_CHARSET_SIZE 6 /* for UTF-8 */

=== exfat ===
exfat/exfat_raw.h
#define EXFAT_MAX_FILE_LEN 255

exfat/super.c
static int exfat_statfs(struct dentry *dentry, struct kstatfs *buf)
{
	...
    /* Unicode utf16 255 characters */
    buf->f_namelen = EXFAT_MAX_FILE_LEN * NLS_MAX_CHARSET_SIZE;

=== vfat ===
include/uapi/linux/msdos_fs.h
#define FAT_LFN_LEN 255     /* maximum long name length */

fat/inode.c
static int fat_statfs(struct dentry *dentry, struct kstatfs *buf)
{
	...
    buf->f_namelen =
        (sbi->options.isvfat ? FAT_LFN_LEN : 12) * NLS_MAX_CHARSET_SIZE;

=> i.e. for vfat without long filename support it'd be 72.

How about
1) don't skip exfat and vfat but just skip creating file with valid name? or

2) Add #define NLS_MAX_CHARSET_SIZE 6 and for vfat and exfat calculate
length as: buf.f_namemax / NLS_MAX_CHARSET_SIZE - 1 ?

Kind regards,
Petr

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [LTP] [PATCH v2] statvfs01: Convert to new LTP API
  2022-11-30  9:50         ` Petr Vorel
@ 2022-12-01  5:16           ` Li Wang
  2022-12-01  9:34             ` Richard Palethorpe
  2022-12-01  8:51           ` Avinesh Kumar
  1 sibling, 1 reply; 15+ messages in thread
From: Li Wang @ 2022-12-01  5:16 UTC (permalink / raw)
  To: Petr Vorel; +Cc: ltp, Richard Palethorpe


[-- Attachment #1.1: Type: text/plain, Size: 1983 bytes --]

On Wed, Nov 30, 2022 at 5:50 PM Petr Vorel <pvorel@suse.cz> wrote:

> Hi all,
>
> ...
> > > +static struct tst_test test = {
> > > +   .test_all = run,
> > > +   .setup = setup,
> > > +   .needs_root = 1,
> > > +   .mount_device = 1,
> > > +   .mntpoint = MNT_POINT,
> > > +   .all_filesystems = 1,
> > > +   .skip_filesystems = (const char *const[]) {
> > > +           "vfat",
> > > +           "exfat",
> > I was looking what's wrong with vfat and exfat.
> > statvfs.f_namemax returns 1530, which is obviously too long, thus
> valid_fname
> > obviously returns ENAMETOOLONG (36). Tested on
> 6.1.0-rc6-1.g4c01546-default.
> > I wonder why, isn't that a bug?
>
> To reply myself, both glibc and musl defines:
> statvfs->f_namemax = statfs->f_namelen;
>
> TL;DR: 6 * 255 = 1530 due names being in UTF-8:
>
> Therefore looking into kernel sources for statfs->f_namelen:
>
> include/linux/nls.h
> #define NLS_MAX_CHARSET_SIZE 6 /* for UTF-8 */
>
> === exfat ===
> exfat/exfat_raw.h
> #define EXFAT_MAX_FILE_LEN 255
>
> exfat/super.c
> static int exfat_statfs(struct dentry *dentry, struct kstatfs *buf)
> {
>         ...
>     /* Unicode utf16 255 characters */
>     buf->f_namelen = EXFAT_MAX_FILE_LEN * NLS_MAX_CHARSET_SIZE;
>
> === vfat ===
> include/uapi/linux/msdos_fs.h
> #define FAT_LFN_LEN 255     /* maximum long name length */
>
> fat/inode.c
> static int fat_statfs(struct dentry *dentry, struct kstatfs *buf)
> {
>         ...
>     buf->f_namelen =
>         (sbi->options.isvfat ? FAT_LFN_LEN : 12) * NLS_MAX_CHARSET_SIZE;
>
> => i.e. for vfat without long filename support it'd be 72.
>
> How about
> 1) don't skip exfat and vfat but just skip creating file with valid name?
> or
>

Sure, I think this method is better.


>
> 2) Add #define NLS_MAX_CHARSET_SIZE 6 and for vfat and exfat calculate
> length as: buf.f_namemax / NLS_MAX_CHARSET_SIZE - 1 ?
>
> Kind regards,
> Petr
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
>

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 3151 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [LTP] [PATCH] statvfs01: Convert to new LTP API
  2022-11-30  7:20     ` [LTP] [PATCH] " Li Wang
@ 2022-12-01  6:00       ` Li Wang
  0 siblings, 0 replies; 15+ messages in thread
From: Li Wang @ 2022-12-01  6:00 UTC (permalink / raw)
  To: rpalethorpe; +Cc: ltp


[-- Attachment #1.1: Type: text/plain, Size: 4113 bytes --]

Li Wang <liwang@redhat.com> wrote:


> We (probably) know that maximum filename should be less than 255 chars
>> (for e.g.), but I think there is a good chance that trying to validate
>> this will result in false positives and stuff we might want to revert.
>>
>
> Maybe we can create a concrete size of the device and mount
> it with a designated FS (e.g. ext4), then extracting the known FS
> data into `struct statvfs` and validating them.
>

As some of the data are determined by the FS attribute and
device size. (e.g. block size, total block, fragment size, and
total inodes). We can check them directly by known values.

For those easy change data, e.g. FS free blocks, which
determined by the real system status, maybe just check
that is no large than the total block number.

Based on that I draft something below to validate the fields.
what do you think?


#include <sys/statvfs.h>
#include "tst_test.h"

#define MNT_POINT "mntpoint"
#define TEST_PATH MNT_POINT"/testfile"

static void run(void)
{
        int type;
        struct statfs buf;
        struct statvfs vbuf;

        TST_EXP_PASS_SILENT(statfs(TEST_PATH, &buf));
        TST_EXP_PASS(statvfs(TEST_PATH, &vbuf));

        tst_res(TINFO, "Extracting FS info from the '%s' file", MNT_POINT);
        tst_res(TINFO, "vbuf.f_fsid == %lu", vbuf.f_fsid);

        long fs_type = tst_fs_type(TEST_PATH);

        switch (fs_type) {
        case TST_EXT2_OLD_MAGIC:
        case TST_EXT234_MAGIC:
                TST_EXP_EQ_LI(vbuf.f_bsize, 1024);
                TST_EXP_EQ_LI(vbuf.f_frsize, 1024);
                ttype = (vbuf.f_bfree <= buf.f_blocks) ? TPASS : TFAIL;
                tst_res(ttype, "vbuf.f_bfree == %ju", (uintmax_t)
vbuf.f_bfree);
                TST_EXP_EQ_LI(vbuf.f_files, 76912);
                ttype = (vbuf.f_ffree <= vbuf.f_files) ? TPASS : TFAIL;
                tst_res(ttype, "vbuf.f_ffree == %ju", (uintmax_t)
vbuf.f_ffree);
                TST_EXP_EQ_LI(vbuf.f_flag, 4096);
                TST_EXP_EQ_LI(vbuf.f_namemax, 255);
                break;
        case TST_XFS_MAGIC:
                TST_EXP_EQ_LI(vbuf.f_bsize, 4096);
                TST_EXP_EQ_LI(vbuf.f_frsize, 4096);
                ttype = (vbuf.f_bfree <= buf.f_blocks) ? TPASS : TFAIL;
                tst_res(ttype, "vbuf.f_bfree == %ju", (uintmax_t)
vbuf.f_bfree);
                TST_EXP_EQ_LI(vbuf.f_files, 153600);
                ttype = (vbuf.f_ffree <= vbuf.f_files) ? TPASS : TFAIL;
                tst_res(ttype, "vbuf.f_ffree == %ju", (uintmax_t)
vbuf.f_ffree);
                TST_EXP_EQ_LI(vbuf.f_flag, 4096);
                TST_EXP_EQ_LI(vbuf.f_namemax, 255);
                break;
        case TST_BTRFS_MAGIC:
                TST_EXP_EQ_LI(vbuf.f_bsize, 4096);
                TST_EXP_EQ_LI(vbuf.f_frsize, 4096);
                ttype = (vbuf.f_bfree <= buf.f_blocks) ? TPASS : TFAIL;
                tst_res(ttype, "vbuf.f_bfree == %ju", (uintmax_t)
vbuf.f_bfree);
                TST_EXP_EQ_LI(vbuf.f_files, 0);
                ttype = (vbuf.f_ffree <= vbuf.f_files) ? TPASS : TFAIL;
                tst_res(ttype, "vbuf.f_ffree == %ju", (uintmax_t)
vbuf.f_ffree);
                TST_EXP_EQ_LI(vbuf.f_flag, 4096);
                TST_EXP_EQ_LI(vbuf.f_namemax, 255);
                break;
        default:
                tst_res(TINFO, "vbuf.f_bsize == %lu bytes", vbuf.f_bsize);
                tst_res(TINFO, "vbuf.f_frsize == %lu bytes", vbuf.f_frsize);
                tst_res(TINFO, "vbuf.f_bfree == %ju", (uintmax_t)
vbuf.f_bfree);
                tst_res(TINFO, "vbuf.f_files == %ju", (uintmax_t)
vbuf.f_files);
                tst_res(TINFO, "vbuf.f_ffree == %ju", (uintmax_t)
vbuf.f_ffree);
                tst_res(TINFO, "vbuf.f_namemax == %lu", vbuf.f_namemax);
                break;
    }
}

static void setup(void)
{
        SAFE_TOUCH(TEST_PATH, 0666, NULL);
}

static struct tst_test test = {
        .test_all = run,
        .setup = setup,
        .needs_root = 1,
        .mount_device = 1,
        .mntpoint = MNT_POINT,
        .dev_min_size = 300,
        .all_filesystems = 1
};


-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 8403 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [LTP] [PATCH v2] statvfs01: Convert to new LTP API
  2022-11-30  9:50         ` Petr Vorel
  2022-12-01  5:16           ` Li Wang
@ 2022-12-01  8:51           ` Avinesh Kumar
  2022-12-01  9:17             ` Li Wang
  1 sibling, 1 reply; 15+ messages in thread
From: Avinesh Kumar @ 2022-12-01  8:51 UTC (permalink / raw)
  To: Petr Vorel; +Cc: Li Wang, Richard Palethorpe, ltp

Hi Petr,

On Wednesday, November 30, 2022 3:20:37 PM IST Petr Vorel wrote:
> Hi all,
> 
> ...
> > > +static struct tst_test test = {
> > > +	.test_all = run,
> > > +	.setup = setup,
> > > +	.needs_root = 1,
> > > +	.mount_device = 1,
> > > +	.mntpoint = MNT_POINT,
> > > +	.all_filesystems = 1,
> > > +	.skip_filesystems = (const char *const[]) {
> > > +		"vfat",
> > > +		"exfat",
> > I was looking what's wrong with vfat and exfat.
> > statvfs.f_namemax returns 1530, which is obviously too long, thus valid_fname
> > obviously returns ENAMETOOLONG (36). Tested on 6.1.0-rc6-1.g4c01546-default.
> > I wonder why, isn't that a bug?
> 
> To reply myself, both glibc and musl defines:
> statvfs->f_namemax = statfs->f_namelen;
> 
> TL;DR: 6 * 255 = 1530 due names being in UTF-8:
> 
> Therefore looking into kernel sources for statfs->f_namelen:
> 
> include/linux/nls.h
> #define NLS_MAX_CHARSET_SIZE 6 /* for UTF-8 */
> 
> === exfat ===
> exfat/exfat_raw.h
> #define EXFAT_MAX_FILE_LEN 255
> 
> exfat/super.c
> static int exfat_statfs(struct dentry *dentry, struct kstatfs *buf)
> {
> 	...
>     /* Unicode utf16 255 characters */
>     buf->f_namelen = EXFAT_MAX_FILE_LEN * NLS_MAX_CHARSET_SIZE;
> 
> === vfat ===
> include/uapi/linux/msdos_fs.h
> #define FAT_LFN_LEN 255     /* maximum long name length */
> 
> fat/inode.c
> static int fat_statfs(struct dentry *dentry, struct kstatfs *buf)
> {
> 	...
>     buf->f_namelen =
>         (sbi->options.isvfat ? FAT_LFN_LEN : 12) * NLS_MAX_CHARSET_SIZE;
> 
> => i.e. for vfat without long filename support it'd be 72.
> 
> How about
> 1) don't skip exfat and vfat but just skip creating file with valid name? or
> 
> 2) Add #define NLS_MAX_CHARSET_SIZE 6 and for vfat and exfat calculate
> length as: buf.f_namemax / NLS_MAX_CHARSET_SIZE - 1 ?
> 
> Kind regards,
> Petr
> 

Thank you for the review and research on vfat, exfat scenarios.
I have adopted the option 1 for now and sent a v3 of this patch.


Regards,
Avinesh





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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [LTP] [PATCH v2] statvfs01: Convert to new LTP API
  2022-12-01  8:51           ` Avinesh Kumar
@ 2022-12-01  9:17             ` Li Wang
  2022-12-01 10:45               ` Petr Vorel
  0 siblings, 1 reply; 15+ messages in thread
From: Li Wang @ 2022-12-01  9:17 UTC (permalink / raw)
  To: Avinesh Kumar, Petr Vorel; +Cc: Richard Palethorpe, ltp


[-- Attachment #1.1: Type: text/plain, Size: 1471 bytes --]

Avinesh Kumar <akumar@suse.de> wrote:

> How about
> > 1) don't skip exfat and vfat but just skip creating file with valid
> name? or
> >
> > 2) Add #define NLS_MAX_CHARSET_SIZE 6 and for vfat and exfat calculate
> > length as: buf.f_namemax / NLS_MAX_CHARSET_SIZE - 1 ?
>


> Thank you for the review and research on vfat, exfat scenarios.
> I have adopted the option 1 for now and sent a v3 of this patch.
>

I thought option_1 meant to skip creating a valide-file when
detecting on "vfat,exfat" FS, but not skip for others.

Or probably I misunderstood Petr's words.

Anyway, don't hurry to send V3 until we get an agreement :).

--- a/testcases/kernel/syscalls/statvfs/statvfs01.c
+++ b/testcases/kernel/syscalls/statvfs/statvfs01.c
@@ -30,7 +30,10 @@ static void run(void)
        memset(valid_fname, 'a', buf.f_namemax - 1);
        memset(invalid_fname, 'b', buf.f_namemax + 1);

-       TST_EXP_FD(creat(valid_fname, 0444));
+       long fs_type = tst_fs_type(TEST_PATH);
+       if  (fs_type != TST_VFAT_MAGIC && fs_type != TST_EXFAT_MAGIC)
+               TST_EXP_FD(creat(valid_fname, 0444));
+
        TST_EXP_FAIL(creat(invalid_fname, 0444), ENAMETOOLONG);
 }

@@ -46,9 +49,4 @@ static struct tst_test test = {
        .mount_device = 1,
        .mntpoint = MNT_POINT,
        .all_filesystems = 1,
-       .skip_filesystems = (const char *const[]) {
-               "vfat",
-               "exfat",
-               NULL
-       }
 };


-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 2843 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [LTP] [PATCH v2] statvfs01: Convert to new LTP API
  2022-12-01  5:16           ` Li Wang
@ 2022-12-01  9:34             ` Richard Palethorpe
  2022-12-02  9:20               ` Petr Vorel
  0 siblings, 1 reply; 15+ messages in thread
From: Richard Palethorpe @ 2022-12-01  9:34 UTC (permalink / raw)
  To: Li Wang; +Cc: ltp

Hello,

Li Wang <liwang@redhat.com> writes:

> On Wed, Nov 30, 2022 at 5:50 PM Petr Vorel <pvorel@suse.cz> wrote:
>
>> Hi all,
>>
>> ...
>> > > +static struct tst_test test = {
>> > > +   .test_all = run,
>> > > +   .setup = setup,
>> > > +   .needs_root = 1,
>> > > +   .mount_device = 1,
>> > > +   .mntpoint = MNT_POINT,
>> > > +   .all_filesystems = 1,
>> > > +   .skip_filesystems = (const char *const[]) {
>> > > +           "vfat",
>> > > +           "exfat",
>> > I was looking what's wrong with vfat and exfat.
>> > statvfs.f_namemax returns 1530, which is obviously too long, thus
>> valid_fname
>> > obviously returns ENAMETOOLONG (36). Tested on
>> 6.1.0-rc6-1.g4c01546-default.
>> > I wonder why, isn't that a bug?

This is the kind of issue which made me think it should be a separate
patch. Because maybe it is a bug.

>>
>> To reply myself, both glibc and musl defines:
>> statvfs->f_namemax = statfs->f_namelen;
>>
>> TL;DR: 6 * 255 = 1530 due names being in UTF-8:
>>
>> Therefore looking into kernel sources for statfs->f_namelen:
>>
>> include/linux/nls.h
>> #define NLS_MAX_CHARSET_SIZE 6 /* for UTF-8 */
>>
>> === exfat ===
>> exfat/exfat_raw.h
>> #define EXFAT_MAX_FILE_LEN 255
>>
>> exfat/super.c
>> static int exfat_statfs(struct dentry *dentry, struct kstatfs *buf)
>> {
>>         ...
>>     /* Unicode utf16 255 characters */
>>     buf->f_namelen = EXFAT_MAX_FILE_LEN * NLS_MAX_CHARSET_SIZE;
>>
>> === vfat ===
>> include/uapi/linux/msdos_fs.h
>> #define FAT_LFN_LEN 255     /* maximum long name length */
>>
>> fat/inode.c
>> static int fat_statfs(struct dentry *dentry, struct kstatfs *buf)
>> {
>>         ...
>>     buf->f_namelen =
>>         (sbi->options.isvfat ? FAT_LFN_LEN : 12) * NLS_MAX_CHARSET_SIZE;
>>
>> => i.e. for vfat without long filename support it'd be 72.
>>
>> How about
>> 1) don't skip exfat and vfat but just skip creating file with valid name?
>> or
>>
>
> Sure, I think this method is better.

Is it supposed to return the length in bytes or unicode 'characters'? If
it's the later then things get really complicated so I guess it's bytes.

However BTRFS also supports unicode (and bigger file names in theory)
and just reports 255. If you look at the BTRFS code comments, it says
that they limited it to 255 because other things might break.

So will creating a file with > 255 chars ever work, even if we use
UTF-16 symbols?

In the meantime could we just read the data into a guarded buffer and
check it's not all zero's or all one's (for e.g.)?

>
>
>>
>> 2) Add #define NLS_MAX_CHARSET_SIZE 6 and for vfat and exfat calculate
>> length as: buf.f_namemax / NLS_MAX_CHARSET_SIZE - 1 ?
>>
>> Kind regards,
>> Petr
>>
>> --
>> Mailing list info: https://lists.linux.it/listinfo/ltp
>>
>>


-- 
Thank you,
Richard.

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [LTP] [PATCH v2] statvfs01: Convert to new LTP API
  2022-12-01  9:17             ` Li Wang
@ 2022-12-01 10:45               ` Petr Vorel
  2022-12-01 11:04                 ` Avinesh Kumar
  0 siblings, 1 reply; 15+ messages in thread
From: Petr Vorel @ 2022-12-01 10:45 UTC (permalink / raw)
  To: Li Wang; +Cc: ltp, Richard Palethorpe

Hi all,

> Avinesh Kumar <akumar@suse.de> wrote:

> > How about
> > > 1) don't skip exfat and vfat but just skip creating file with valid
> > name? or

> > > 2) Add #define NLS_MAX_CHARSET_SIZE 6 and for vfat and exfat calculate
> > > length as: buf.f_namemax / NLS_MAX_CHARSET_SIZE - 1 ?



> > Thank you for the review and research on vfat, exfat scenarios.
> > I have adopted the option 1 for now and sent a v3 of this patch.


> I thought option_1 meant to skip creating a valide-file when
> detecting on "vfat,exfat" FS, but not skip for others.

> Or probably I misunderstood Petr's words.
No, you understood me well. I wanted this to be in v3,
but it's not there :).

Kind regards,
Petr

> Anyway, don't hurry to send V3 until we get an agreement :).

> --- a/testcases/kernel/syscalls/statvfs/statvfs01.c
> +++ b/testcases/kernel/syscalls/statvfs/statvfs01.c
> @@ -30,7 +30,10 @@ static void run(void)
>         memset(valid_fname, 'a', buf.f_namemax - 1);
>         memset(invalid_fname, 'b', buf.f_namemax + 1);

> -       TST_EXP_FD(creat(valid_fname, 0444));
> +       long fs_type = tst_fs_type(TEST_PATH);
> +       if  (fs_type != TST_VFAT_MAGIC && fs_type != TST_EXFAT_MAGIC)
> +               TST_EXP_FD(creat(valid_fname, 0444));
> +
>         TST_EXP_FAIL(creat(invalid_fname, 0444), ENAMETOOLONG);
>  }

> @@ -46,9 +49,4 @@ static struct tst_test test = {
>         .mount_device = 1,
>         .mntpoint = MNT_POINT,
>         .all_filesystems = 1,
> -       .skip_filesystems = (const char *const[]) {
> -               "vfat",
> -               "exfat",
> -               NULL
> -       }
>  };

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [LTP] [PATCH v2] statvfs01: Convert to new LTP API
  2022-12-01 10:45               ` Petr Vorel
@ 2022-12-01 11:04                 ` Avinesh Kumar
  0 siblings, 0 replies; 15+ messages in thread
From: Avinesh Kumar @ 2022-12-01 11:04 UTC (permalink / raw)
  To: Li Wang, Petr Vorel; +Cc: Li Wang, Richard Palethorpe, ltp

On Thursday, December 1, 2022 4:15:46 PM IST Petr Vorel wrote:
> Hi all,
> 
> > Avinesh Kumar <akumar@suse.de> wrote:
> 
> > > How about
> > > > 1) don't skip exfat and vfat but just skip creating file with valid
> > > name? or
> 
> > > > 2) Add #define NLS_MAX_CHARSET_SIZE 6 and for vfat and exfat calculate
> > > > length as: buf.f_namemax / NLS_MAX_CHARSET_SIZE - 1 ?
> 
> 
> 
> > > Thank you for the review and research on vfat, exfat scenarios.
> > > I have adopted the option 1 for now and sent a v3 of this patch.
> 
> 
> > I thought option_1 meant to skip creating a valide-file when
> > detecting on "vfat,exfat" FS, but not skip for others.
> 
> > Or probably I misunderstood Petr's words.
> No, you understood me well. I wanted this to be in v3,
> but it's not there :).
> 
> Kind regards,
> Petr
> 
> > Anyway, don't hurry to send V3 until we get an agreement :).

Hi Petr, Li,

Sorry, I misunderstood and had removed the valid filename creation altogether,
I am correcting the patch in v4, as suggested by Li.

Thanks,
Avinesh

> 
> > --- a/testcases/kernel/syscalls/statvfs/statvfs01.c
> > +++ b/testcases/kernel/syscalls/statvfs/statvfs01.c
> > @@ -30,7 +30,10 @@ static void run(void)
> >         memset(valid_fname, 'a', buf.f_namemax - 1);
> >         memset(invalid_fname, 'b', buf.f_namemax + 1);
> 
> > -       TST_EXP_FD(creat(valid_fname, 0444));
> > +       long fs_type = tst_fs_type(TEST_PATH);
> > +       if  (fs_type != TST_VFAT_MAGIC && fs_type != TST_EXFAT_MAGIC)
> > +               TST_EXP_FD(creat(valid_fname, 0444));
> > +
> >         TST_EXP_FAIL(creat(invalid_fname, 0444), ENAMETOOLONG);
> >  }
> 
> > @@ -46,9 +49,4 @@ static struct tst_test test = {
> >         .mount_device = 1,
> >         .mntpoint = MNT_POINT,
> >         .all_filesystems = 1,
> > -       .skip_filesystems = (const char *const[]) {
> > -               "vfat",
> > -               "exfat",
> > -               NULL
> > -       }
> >  };
> 





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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [LTP] [PATCH v2] statvfs01: Convert to new LTP API
  2022-12-01  9:34             ` Richard Palethorpe
@ 2022-12-02  9:20               ` Petr Vorel
  0 siblings, 0 replies; 15+ messages in thread
From: Petr Vorel @ 2022-12-02  9:20 UTC (permalink / raw)
  To: Richard Palethorpe; +Cc: ltp

Hi all,

...
> >> To reply myself, both glibc and musl defines:
> >> statvfs->f_namemax = statfs->f_namelen;

> >> TL;DR: 6 * 255 = 1530 due names being in UTF-8:

> >> Therefore looking into kernel sources for statfs->f_namelen:

> >> include/linux/nls.h
> >> #define NLS_MAX_CHARSET_SIZE 6 /* for UTF-8 */

> >> === exfat ===
> >> exfat/exfat_raw.h
> >> #define EXFAT_MAX_FILE_LEN 255

> >> exfat/super.c
> >> static int exfat_statfs(struct dentry *dentry, struct kstatfs *buf)
> >> {
> >>         ...
> >>     /* Unicode utf16 255 characters */
> >>     buf->f_namelen = EXFAT_MAX_FILE_LEN * NLS_MAX_CHARSET_SIZE;

> >> === vfat ===
> >> include/uapi/linux/msdos_fs.h
> >> #define FAT_LFN_LEN 255     /* maximum long name length */

> >> fat/inode.c
> >> static int fat_statfs(struct dentry *dentry, struct kstatfs *buf)
> >> {
> >>         ...
> >>     buf->f_namelen =
> >>         (sbi->options.isvfat ? FAT_LFN_LEN : 12) * NLS_MAX_CHARSET_SIZE;

> >> => i.e. for vfat without long filename support it'd be 72.

> >> How about
> >> 1) don't skip exfat and vfat but just skip creating file with valid name?
> >> or


> > Sure, I think this method is better.

> Is it supposed to return the length in bytes or unicode 'characters'? If
> it's the later then things get really complicated so I guess it's bytes.
Yes.

TL;DR: I'd test vfat and exfat for valid filename with (f_namelen / 6) + 1.

> However BTRFS also supports unicode (and bigger file names in theory)
> and just reports 255. If you look at the BTRFS code comments, it says
> that they limited it to 255 because other things might break.

> So will creating a file with > 255 chars ever work, even if we use
> UTF-16 symbols?

Wrote some testing code (see below), vfat and exfat behaves differently:

$ gcc -Wall statvfs-test-ms.c -o statvfs-test-ms && ./statvfs-test-ms /tmp/vfat/
=== filesystem: vfat ===

file: test_ϴ_12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678
strlen: 256, mblen: 255
buf.f_namelen: 1530

file: test_ϴ_12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678_
strlen: 257, mblen: 256
creat() failed: File name too long

file: ϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴ
strlen: 510, mblen: 255
buf.f_namelen: 1530

Also NTFS over fuse behaves the same (allows 256 length), but (probably due
fuse) f_namelen is only 255:

=== filesystem: fuse ===

file: test_ϴ_12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678
strlen: 256, mblen: 255
buf.f_namelen: 255

file: test_ϴ_12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678_
strlen: 257, mblen: 256
creat() failed: File name too long

file: ϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴ
strlen: 510, mblen: 255
buf.f_namelen: 255


NOTE: they both allow strlen 256 as max character multibyte length is in that
case just 255 (not counting terminating null byte \0 ?).

When testing on normal linux filesystems (btrfs, ext4, xfs, tmpfs, ...) they all
allow only strlen 255 and obviously does not work with multibyte (output on shortened
filenames):

=== filesystem: btrfs ===

file: test_ϴ_1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567
strlen: 255, mblen: 254
buf.f_namelen: 255

file: test_ϴ_1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567_
strlen: 256, mblen: 255
creat() failed: File name too long

file: ϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴ
strlen: 508, mblen: 254
creat() failed: File name too long

Kind regards,
Petr

> In the meantime could we just read the data into a guarded buffer and
> check it's not all zero's or all one's (for e.g.)?




> >> 2) Add #define NLS_MAX_CHARSET_SIZE 6 and for vfat and exfat calculate
> >> length as: buf.f_namemax / NLS_MAX_CHARSET_SIZE - 1 ?

> >> Kind regards,
> >> Petr

#include <fcntl.h>
#include <locale.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/statfs.h>
#include <unistd.h>

/* test requires C.utf8 locale */
#define LOCALE "C.utf8"

/* here for vfat, exfat, which allow 256, Linux filesystems (btrfs, ext4, tmpfs xfs, ...) allow only 255 */
#define MAX_FILENAME_PATH "test_ϴ_" \
"1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890" \
"1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890" \
"123456789012345678901234567890123456789012345678"

#define TOO_LONG_FILENAME MAX_FILENAME_PATH "_"

/* here for vfat, exfat, which allow 256, Linux filesystems (btrfs, ext4, tmpfs xfs, ...) allow only 255 */
#define MAX_FILENAME_PATH_UTF16 \
"ϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴ" \
"ϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴ" \
"ϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴϴ"

/* man 2 statfs or kernel-source/include/linux/magic.h */
#define TST_BTRFS_MAGIC    0x9123683E
#define TST_NFS_MAGIC      0x6969
#define TST_RAMFS_MAGIC    0x858458f6
#define TST_TMPFS_MAGIC    0x01021994
#define TST_V9FS_MAGIC     0x01021997
#define TST_XFS_MAGIC      0x58465342
#define TST_EXT2_OLD_MAGIC 0xEF51
/* ext2, ext3, ext4 have the same magic number */
#define TST_EXT234_MAGIC   0xEF53
#define TST_MINIX_MAGIC    0x137F
#define TST_MINIX_MAGIC2   0x138F
#define TST_MINIX2_MAGIC   0x2468
#define TST_MINIX2_MAGIC2  0x2478
#define TST_MINIX3_MAGIC   0x4D5A
#define TST_UDF_MAGIC      0x15013346
#define TST_SYSV2_MAGIC    0x012FF7B6
#define TST_SYSV4_MAGIC    0x012FF7B5
#define TST_UFS_MAGIC      0x00011954
#define TST_UFS2_MAGIC     0x19540119
#define TST_F2FS_MAGIC     0xF2F52010
#define TST_NILFS_MAGIC    0x3434
#define TST_EXOFS_MAGIC    0x5DF5
#define TST_OVERLAYFS_MAGIC 0x794c7630
#define TST_FUSE_MAGIC     0x65735546
#define TST_VFAT_MAGIC     0x4d44 /* AKA MSDOS */
#define TST_EXFAT_MAGIC    0x2011BAB0UL

long tst_fs_type(const char *path)
{
	struct statfs sbuf;

	if (statfs(path, &sbuf)) {
		perror("statfs");
		exit(EXIT_FAILURE);
		return 0;
	}

	return sbuf.f_type;
}

const char *tst_fs_type_name(long f_type)
{
	switch (f_type) {
	case TST_TMPFS_MAGIC:
		return "tmpfs";
	case TST_NFS_MAGIC:
		return "nfs";
	case TST_V9FS_MAGIC:
		return "9p";
	case TST_RAMFS_MAGIC:
		return "ramfs";
	case TST_BTRFS_MAGIC:
		return "btrfs";
	case TST_XFS_MAGIC:
		return "xfs";
	case TST_EXT2_OLD_MAGIC:
		return "ext2";
	case TST_EXT234_MAGIC:
		return "ext2/ext3/ext4";
	case TST_MINIX_MAGIC:
	case TST_MINIX_MAGIC2:
	case TST_MINIX2_MAGIC:
	case TST_MINIX2_MAGIC2:
	case TST_MINIX3_MAGIC:
		return "minix";
	case TST_UDF_MAGIC:
		return "udf";
	case TST_SYSV2_MAGIC:
	case TST_SYSV4_MAGIC:
		return "sysv";
	case TST_UFS_MAGIC:
	case TST_UFS2_MAGIC:
		return "ufs";
	case TST_F2FS_MAGIC:
		return "f2fs";
	case TST_NILFS_MAGIC:
		return "nilfs";
	case TST_EXOFS_MAGIC:
		return "exofs";
	case TST_OVERLAYFS_MAGIC:
		return "overlayfs";
	case TST_FUSE_MAGIC:
		return "fuse";
	case TST_VFAT_MAGIC:
		return "vfat";
	case TST_EXFAT_MAGIC:
		return "exfat";
	default:
		return "unknown";
	}
}

int creat_file_print_len(char *str)
{
	int fd;
	struct statfs buf;

	printf("\nfile: %s\nstrlen: %zu, mblen: %zu\n", str,
		   strlen(str), mbstowcs(NULL, str, 0));

	if ((fd = creat(str, 0666)) < 0) {
		perror("creat() failed");
		return 1;
	}

	close(fd);

	if (statfs(str, &buf) < 0) {
		perror("statfs() failed");
		return 1;
	}

	printf("buf.f_namelen: %zu\n", buf.f_namelen);

	return 0;
}

int main(int argc, char *argv[]) {

	int ret = 0;

	if (argc < 2) {
		fprintf(stderr, "Usage: %s DIR\n", argv[0]);
		exit(EXIT_FAILURE);
	}

	if (chdir(argv[1]) > 0) {
		perror("chdir() failed");
		exit(EXIT_FAILURE);
	}
	printf("=== filesystem: %s ===\n", tst_fs_type_name(tst_fs_type(".")));

	if (setlocale(LC_ALL, LOCALE) == NULL) {
		perror("setlocale");
		exit(EXIT_FAILURE);
	}

	ret |= creat_file_print_len(MAX_FILENAME_PATH);
	ret |= creat_file_print_len(TOO_LONG_FILENAME);
	ret |= creat_file_print_len(MAX_FILENAME_PATH_UTF16);

	return ret;
}

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2022-12-02  9:20 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-24 11:42 [LTP] [PATCH] statvfs01: Convert to new LTP API Avinesh Kumar
2022-11-25  2:18 ` Li Wang
2022-11-29 10:58   ` Richard Palethorpe
2022-11-30  7:05     ` [LTP] [PATCH v2] " Avinesh Kumar
2022-11-30  8:52       ` Petr Vorel
2022-11-30  9:50         ` Petr Vorel
2022-12-01  5:16           ` Li Wang
2022-12-01  9:34             ` Richard Palethorpe
2022-12-02  9:20               ` Petr Vorel
2022-12-01  8:51           ` Avinesh Kumar
2022-12-01  9:17             ` Li Wang
2022-12-01 10:45               ` Petr Vorel
2022-12-01 11:04                 ` Avinesh Kumar
2022-11-30  7:20     ` [LTP] [PATCH] " Li Wang
2022-12-01  6:00       ` Li Wang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.