[PATCH 0/4] xfstests: fix compiler warnings with fsx/fsstress

[PATCH 0/4] xfstests: fix compiler warnings with fsx/fsstress

[Darrick J. Wong]

Hi all,

Fix all the compiler warnings emitted when building fsstress and fsx.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=fix-fsx-and-fsstress-warnings
---
 ltp/fsstress.c |  102 ++++++++++++++++++++++++++------------------------------
 ltp/fsx.c      |    5 +--
 2 files changed, 50 insertions(+), 57 deletions(-)

[PATCH 1/4] fsx: fix strncpy usage error

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

We shouldn't feed sizeof() to strncpy as the string length.  Just use
snprintf, which at least doesn't have the zero termination problems.

In file included from /usr/include/string.h:495,
                 from ../src/global.h:73,
                 from fsx.c:16:
In function 'strncpy',
    inlined from 'main' at fsx.c:2944:5:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: warning:
'__builtin_strncpy' specified bound 4096 equals destination size
[-Wstringop-truncation]
  106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function 'strncpy',
    inlined from 'main' at fsx.c:2914:4:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: warning:
'__builtin_strncpy' specified bound 1024 equals destination size
[-Wstringop-truncation]
  106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 ltp/fsx.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)


diff --git a/ltp/fsx.c b/ltp/fsx.c
index 0abd7de1..cd0bae55 100644
--- a/ltp/fsx.c
+++ b/ltp/fsx.c
@@ -2769,8 +2769,7 @@ main(int argc, char **argv)
 			randomoplen = 0;
 			break;
 		case 'P':
-			strncpy(dname, optarg, sizeof(dname));
-			strcat(dname, "/");
+			snprintf(dname, sizeof(dname), "%s/", optarg);
 			dirpath = strlen(dname);
 			break;
                 case 'R':
@@ -2799,7 +2798,7 @@ main(int argc, char **argv)
 			break;
 		case 255:  /* --record-ops */
 			if (optarg)
-				strncpy(opsfile, optarg, sizeof(opsfile));
+				snprintf(opsfile, sizeof(opsfile), "%s", optarg);
 			recordops = opsfile;
 			break;
 		case 256:  /* --replay-ops */

Re: [PATCH 1/4] fsx: fix strncpy usage error

[Christoph Hellwig]

On Tue, Nov 10, 2020 at 04:44:43PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> We shouldn't feed sizeof() to strncpy as the string length.  Just use
> snprintf, which at least doesn't have the zero termination problems.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/4] fsstress: stop using attr_set

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

attr_set is deprecated, so replace it with lsetxattr.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 ltp/fsstress.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)


diff --git a/ltp/fsstress.c b/ltp/fsstress.c
index 1238fcf5..41b31060 100644
--- a/ltp/fsstress.c
+++ b/ltp/fsstress.c
@@ -395,7 +395,7 @@ void	add_to_flist(int, int, int, int);
 void	append_pathname(pathname_t *, char *);
 int	attr_list_path(pathname_t *, char *, const int, int, attrlist_cursor_t *);
 int	attr_remove_path(pathname_t *, const char *, int);
-int	attr_set_path(pathname_t *, const char *, const char *, const int, int);
+int	attr_set_path(pathname_t *, const char *, const char *, const int);
 void	check_cwd(void);
 void	cleanup_flist(void);
 int	creat_path(pathname_t *, mode_t);
@@ -909,19 +909,19 @@ attr_remove_path(pathname_t *name, const char *attrname, int flags)
 
 int
 attr_set_path(pathname_t *name, const char *attrname, const char *attrvalue,
-	      const int valuelength, int flags)
+	      const int valuelength)
 {
 	char		buf[NAME_MAX + 1];
 	pathname_t	newname;
 	int		rval;
 
-	rval = attr_set(name->path, attrname, attrvalue, valuelength, flags);
+	rval = lsetxattr(name->path, attrname, attrvalue, valuelength, 0);
 	if (rval >= 0 || errno != ENAMETOOLONG)
 		return rval;
 	separate_pathname(name, buf, &newname);
 	if (chdir(buf) == 0) {
-		rval = attr_set_path(&newname, attrname, attrvalue, valuelength,
-			flags);
+		rval = attr_set_path(&newname, attrname, attrvalue,
+				     valuelength);
 		assert(chdir("..") == 0);
 	}
 	free_pathname(&newname);
@@ -2392,7 +2392,7 @@ attr_set_f(int opno, long r)
 		len = 1;
 	aval = malloc(len);
 	memset(aval, nameseq & 0xff, len);
-	e = attr_set_path(&f, aname, aval, len, ATTR_DONTFOLLOW) < 0 ?
+	e = attr_set_path(&f, aname, aval, len) < 0 ?
 		errno : 0;
 	check_cwd();
 	free(aval);

Re: [PATCH 2/4] fsstress: stop using attr_set

[Christoph Hellwig]

On Tue, Nov 10, 2020 at 04:44:49PM -0800, Darrick J. Wong wrote:
>  	aval = malloc(len);
>  	memset(aval, nameseq & 0xff, len);
> -	e = attr_set_path(&f, aname, aval, len, ATTR_DONTFOLLOW) < 0 ?
> +	e = attr_set_path(&f, aname, aval, len) < 0 ?
>  		errno : 0;

Nipick, but I'd spell this out in a normal if while you touch it:

	if (attr_set_path(&f, aname, aval, len) < 0)
		e = errno;
	else	
		e = 0;

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 3/4] fsstress: remove attr_remove

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

attr_remove is deprecated, so replace it with lremovexattr.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 ltp/fsstress.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)


diff --git a/ltp/fsstress.c b/ltp/fsstress.c
index 41b31060..ad42cb65 100644
--- a/ltp/fsstress.c
+++ b/ltp/fsstress.c
@@ -394,7 +394,7 @@ struct print_string	flag_str = {0};
 void	add_to_flist(int, int, int, int);
 void	append_pathname(pathname_t *, char *);
 int	attr_list_path(pathname_t *, char *, const int, int, attrlist_cursor_t *);
-int	attr_remove_path(pathname_t *, const char *, int);
+int	attr_remove_path(pathname_t *, const char *);
 int	attr_set_path(pathname_t *, const char *, const char *, const int);
 void	check_cwd(void);
 void	cleanup_flist(void);
@@ -889,18 +889,18 @@ attr_list_path(pathname_t *name,
 }
 
 int
-attr_remove_path(pathname_t *name, const char *attrname, int flags)
+attr_remove_path(pathname_t *name, const char *attrname)
 {
 	char		buf[NAME_MAX + 1];
 	pathname_t	newname;
 	int		rval;
 
-	rval = attr_remove(name->path, attrname, flags);
+	rval = lremovexattr(name->path, attrname);
 	if (rval >= 0 || errno != ENAMETOOLONG)
 		return rval;
 	separate_pathname(name, buf, &newname);
 	if (chdir(buf) == 0) {
-		rval = attr_remove_path(&newname, attrname, flags);
+		rval = attr_remove_path(&newname, attrname);
 		assert(chdir("..") == 0);
 	}
 	free_pathname(&newname);
@@ -2362,7 +2362,7 @@ attr_remove_f(int opno, long r)
 		free_pathname(&f);
 		return;
 	}
-	e = attr_remove_path(&f, aname, ATTR_DONTFOLLOW) < 0 ? errno : 0;
+	e = attr_remove_path(&f, aname) < 0 ? errno : 0;
 	check_cwd();
 	if (v)
 		printf("%d/%d: attr_remove %s %s %d\n",

Re: [PATCH 3/4] fsstress: remove attr_remove

[Christoph Hellwig]

On Tue, Nov 10, 2020 at 04:44:56PM -0800, Darrick J. Wong wrote:
> -	e = attr_remove_path(&f, aname, ATTR_DONTFOLLOW) < 0 ? errno : 0;
> +	e = attr_remove_path(&f, aname) < 0 ? errno : 0;
>  	check_cwd();
>  	if (v)
>  		printf("%d/%d: attr_remove %s %s %d\n",

Same comment as for the last one, otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 4/4] fsstress: get rid of attr_list

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

attr_list is deprecated, so just call llistxattr directly.  This is a
bit involved, since attr_remove_f was highly dependent on libattr
structures.  Note that attr_list uses llistxattr internally and
llistxattr is limited to XATTR_LIST_MAX, so this doesn't result in any
loss of functionality.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 ltp/fsstress.c |   80 ++++++++++++++++++++++++++------------------------------
 1 file changed, 37 insertions(+), 43 deletions(-)


diff --git a/ltp/fsstress.c b/ltp/fsstress.c
index ad42cb65..e4940ba4 100644
--- a/ltp/fsstress.c
+++ b/ltp/fsstress.c
@@ -393,7 +393,7 @@ struct print_string	flag_str = {0};
 
 void	add_to_flist(int, int, int, int);
 void	append_pathname(pathname_t *, char *);
-int	attr_list_path(pathname_t *, char *, const int, int, attrlist_cursor_t *);
+int	attr_list_path(pathname_t *, char *, const int);
 int	attr_remove_path(pathname_t *, const char *);
 int	attr_set_path(pathname_t *, const char *, const char *, const int);
 void	check_cwd(void);
@@ -860,28 +860,36 @@ append_pathname(pathname_t *name, char *str)
 	name->len += len;
 }
 
+int
+attr_list_count(char *buffer, int buffersize)
+{
+	char *p = buffer;
+	char *end = buffer + buffersize;
+	int count = 0;
+
+	while (p < end && *p != 0) {
+		count++;
+		p += strlen(p) + 1;
+	}
+
+	return count;
+}
+
 int
 attr_list_path(pathname_t *name,
 	       char *buffer,
-	       const int buffersize,
-	       int flags,
-	       attrlist_cursor_t *cursor)
+	       const int buffersize)
 {
 	char		buf[NAME_MAX + 1];
 	pathname_t	newname;
 	int		rval;
 
-	if (flags != ATTR_DONTFOLLOW) {
-		errno = EINVAL;
-		return -1;
-	}
-
-	rval = attr_list(name->path, buffer, buffersize, flags, cursor);
+	rval = llistxattr(name->path, buffer, buffersize);
 	if (rval >= 0 || errno != ENAMETOOLONG)
 		return rval;
 	separate_pathname(name, buf, &newname);
 	if (chdir(buf) == 0) {
-		rval = attr_list_path(&newname, buffer, buffersize, flags, cursor);
+		rval = attr_list_path(&newname, buffer, buffersize);
 		assert(chdir("..") == 0);
 	}
 	free_pathname(&newname);
@@ -2302,32 +2310,24 @@ aread_f(int opno, long r)
 void
 attr_remove_f(int opno, long r)
 {
-	attrlist_ent_t		*aep;
-	attrlist_t		*alist;
-	char			*aname;
-	char			buf[4096];
-	attrlist_cursor_t	cursor;
+	char			*bufname;
+	char			*bufend;
+	char			*aname = NULL;
+	char			buf[XATTR_LIST_MAX];
 	int			e;
 	int			ent;
 	pathname_t		f;
-	int			total;
+	int			total = 0;
 	int			v;
 	int			which;
 
 	init_pathname(&f);
 	if (!get_fname(FT_ANYm, r, &f, NULL, NULL, &v))
 		append_pathname(&f, ".");
-	total = 0;
-	bzero(&cursor, sizeof(cursor));
-	do {
-		bzero(buf, sizeof(buf));
-		e = attr_list_path(&f, buf, sizeof(buf), ATTR_DONTFOLLOW, &cursor);
-		check_cwd();
-		if (e)
-			break;
-		alist = (attrlist_t *)buf;
-		total += alist->al_count;
-	} while (alist->al_more);
+	e = attr_list_path(&f, buf, sizeof(buf));
+	check_cwd();
+	if (e > 0)
+		total = attr_list_count(buf, e);
 	if (total == 0) {
 		if (v)
 			printf("%d/%d: attr_remove - no attrs for %s\n",
@@ -2335,25 +2335,19 @@ attr_remove_f(int opno, long r)
 		free_pathname(&f);
 		return;
 	}
+
 	which = (int)(random() % total);
-	bzero(&cursor, sizeof(cursor));
+	bufname = buf;
+	bufend = buf + e;
 	ent = 0;
-	aname = NULL;
-	do {
-		bzero(buf, sizeof(buf));
-		e = attr_list_path(&f, buf, sizeof(buf), ATTR_DONTFOLLOW, &cursor);
-		check_cwd();
-		if (e)
-			break;
-		alist = (attrlist_t *)buf;
-		if (which < ent + alist->al_count) {
-			aep = (attrlist_ent_t *)
-				&buf[alist->al_offset[which - ent]];
-			aname = aep->a_name;
+	while (bufname < bufend) {
+		if (which < ent) {
+			aname = bufname;
 			break;
 		}
-		ent += alist->al_count;
-	} while (alist->al_more);
+		ent++;
+		bufname += strlen(bufname) + 1;
+	}
 	if (aname == NULL) {
 		if (v)
 			printf(

Re: [PATCH 4/4] fsstress: get rid of attr_list

[Christoph Hellwig]

Instead of the loop to check if the attr existed can't we just check
for the ENODATA return value from removexattr?

Also I think with this series we can drop the libattr dependency
for xfstests, can't we?

Re: [PATCH 4/4] fsstress: get rid of attr_list

[Darrick J. Wong]

On Sat, Nov 14, 2020 at 10:47:53AM +0000, Christoph Hellwig wrote:
> Instead of the loop to check if the attr existed can't we just check
> for the ENODATA return value from removexattr?

Yes.

> Also I think with this series we can drop the libattr dependency
> for xfstests, can't we?

I'll do that in a subsequent patch.

--D

Re: [PATCH 4/4] fsstress: get rid of attr_list

[Darrick J. Wong]

On Thu, Feb 11, 2021 at 01:18:18PM -0800, Darrick J. Wong wrote:
> On Sat, Nov 14, 2020 at 10:47:53AM +0000, Christoph Hellwig wrote:
> > Instead of the loop to check if the attr existed can't we just check
> > for the ENODATA return value from removexattr?
> 
> Yes.

No -- attr_remove_f reads the list of attribute names from the file and
then picks one at random to lremovexattr().

--D

> 
> > Also I think with this series we can drop the libattr dependency
> > for xfstests, can't we?
> 
> I'll do that in a subsequent patch.
> 
> --D

Re: [PATCH 0/4] xfstests: fix compiler warnings with fsx/fsstress

[Eryu Guan]

On Tue, Nov 10, 2020 at 04:44:37PM -0800, Darrick J. Wong wrote:
> Hi all,
> 
> Fix all the compiler warnings emitted when building fsstress and fsx.

I've taken the first 3 patches, and left the last one for now :)

Thanks,
Eryu

> 
> If you're going to start using this mess, you probably ought to just
> pull from my git trees, which are linked below.
> 
> This is an extraordinary way to destroy everything.  Enjoy!
> Comments and questions are, as always, welcome.
> 
> --D
> 
> fstests git tree:
> https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=fix-fsx-and-fsstress-warnings
> ---
>  ltp/fsstress.c |  102 ++++++++++++++++++++++++++------------------------------
>  ltp/fsx.c      |    5 +--
>  2 files changed, 50 insertions(+), 57 deletions(-)