| Message ID | 20211101155559.3988492-1-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fsstress: improve error message on check_cwd() error | expand |
On Mon, Nov 01, 2021 at 08:55:59AM -0700, Luis Chamberlain wrote: > I ran into an error with generic/083 with xfs due to check_cwd() but > why it failed is not clear because there are two types of > failures: > > o stat64() failed (likely -ENOMEM is my guess) > o the inode actually changed > > Throw a bone out to developers so that in case en error does happen > they know which rabbit hole to go down on. <cough> word choice on those last three words... > > Cc: Anthony Iliopoulos <ailiopoulos@suse.de> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > --- > ltp/fsstress.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/ltp/fsstress.c b/ltp/fsstress.c > index 90ae432e..a576afea 100644 > --- a/ltp/fsstress.c > +++ b/ltp/fsstress.c > @@ -9,6 +9,7 @@ > #include <sys/uio.h> > #include <stddef.h> > #include <stdbool.h> > +#include <string.h> > #include "global.h" > > #ifdef HAVE_BTRFSUTIL_H > @@ -943,9 +944,21 @@ check_cwd(void) > { > #ifdef DEBUG > struct stat64 statbuf; > + int ret; > + > + ret = stat64(".", &statbuf); > + if (ret !=0) { Nit: space between '!=' and '0'. > + fprintf(stderr, "fsstress: check_cwd stat64 failed with: %d (%s)\n", > + ret, strerror(ret)); ret is set to -1 on error, according to the manpage; to get the real error you'd have to call strerror(errno) as the last arg, or be lazy and: perror("fsstress check_cwd stat64"); > + goto out; > + } > > - if (stat64(".", &statbuf) == 0 && statbuf.st_ino == top_ino) > + if (statbuf.st_ino == top_ino) > return; > + > + fprintf(stderr, "fsstress: check_cwd statbuf.st_ino (%lu) != top_ino (%lu)\n", > + statbuf.st_ino, top_ino); This might want some explicit casting, since this can be defined as anything between unsigned long to uint64_t, at least according to the glibc headers on my system. --D > +out: > assert(chdir(homedir) == 0); > fprintf(stderr, "fsstress: check_cwd failure\n"); > abort(); > -- > 2.33.0 >
On Wed, Nov 03, 2021 at 09:24:34AM -0700, Darrick J. Wong wrote: > On Mon, Nov 01, 2021 at 08:55:59AM -0700, Luis Chamberlain wrote: > > I ran into an error with generic/083 with xfs due to check_cwd() but > > why it failed is not clear because there are two types of > > failures: > > > > o stat64() failed (likely -ENOMEM is my guess) > > o the inode actually changed > > > > Throw a bone out to developers so that in case en error does happen > > they know which rabbit hole to go down on. > > <cough> word choice on those last three words... Will fix thanks. > > > > Cc: Anthony Iliopoulos <ailiopoulos@suse.de> > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > > --- > > ltp/fsstress.c | 15 ++++++++++++++- > > 1 file changed, 14 insertions(+), 1 deletion(-) > > > > diff --git a/ltp/fsstress.c b/ltp/fsstress.c > > index 90ae432e..a576afea 100644 > > --- a/ltp/fsstress.c > > +++ b/ltp/fsstress.c > > @@ -9,6 +9,7 @@ > > #include <sys/uio.h> > > #include <stddef.h> > > #include <stdbool.h> > > +#include <string.h> > > #include "global.h" > > > > #ifdef HAVE_BTRFSUTIL_H > > @@ -943,9 +944,21 @@ check_cwd(void) > > { > > #ifdef DEBUG > > struct stat64 statbuf; > > + int ret; > > + > > + ret = stat64(".", &statbuf); > > + if (ret !=0) { > > Nit: space between '!=' and '0'. OK. > > + fprintf(stderr, "fsstress: check_cwd stat64 failed with: %d (%s)\n", > > + ret, strerror(ret)); > > ret is set to -1 on error, according to the manpage; to get the real > error you'd have to call strerror(errno) as the last arg, or be lazy > and: I don't want to be lazy here as this is a real issue. > perror("fsstress check_cwd stat64"); > > > + goto out; > > + } > > > > - if (stat64(".", &statbuf) == 0 && statbuf.st_ino == top_ino) > > + if (statbuf.st_ino == top_ino) > > return; > > + > > + fprintf(stderr, "fsstress: check_cwd statbuf.st_ino (%lu) != top_ino (%lu)\n", > > + statbuf.st_ino, top_ino); > > This might want some explicit casting, since this can be defined as > anything between unsigned long to uint64_t, at least according to the > glibc headers on my system. Um, Filipe had suggested something a bit different before. Can you guys decide and let me know your final preference ? :) Luis
On Wed, Nov 03, 2021 at 11:18:22AM -0700, Luis Chamberlain wrote: > On Wed, Nov 03, 2021 at 09:24:34AM -0700, Darrick J. Wong wrote: > > > - if (stat64(".", &statbuf) == 0 && statbuf.st_ino == top_ino) > > > + if (statbuf.st_ino == top_ino) > > > return; > > > + > > > + fprintf(stderr, "fsstress: check_cwd statbuf.st_ino (%lu) != top_ino (%lu)\n", > > > + statbuf.st_ino, top_ino); > > > > This might want some explicit casting, since this can be defined as > > anything between unsigned long to uint64_t, at least according to the > > glibc headers on my system. > > Um, Filipe had suggested something a bit different before. Can you guys > decide and let me know your final preference ? :) Any final preference / recommendation? Luis
On Fri, Nov 05, 2021 at 09:04:21AM -0700, Luis Chamberlain wrote: > On Wed, Nov 03, 2021 at 11:18:22AM -0700, Luis Chamberlain wrote: > > On Wed, Nov 03, 2021 at 09:24:34AM -0700, Darrick J. Wong wrote: > > > > - if (stat64(".", &statbuf) == 0 && statbuf.st_ino == top_ino) > > > > + if (statbuf.st_ino == top_ino) > > > > return; > > > > + > > > > + fprintf(stderr, "fsstress: check_cwd statbuf.st_ino (%lu) != top_ino (%lu)\n", > > > > + statbuf.st_ino, top_ino); > > > > > > This might want some explicit casting, since this can be defined as > > > anything between unsigned long to uint64_t, at least according to the > > > glibc headers on my system. > > > > Um, Filipe had suggested something a bit different before. Can you guys > > decide and let me know your final preference ? :) > > Any final preference / recommendation? Use %llu with explicit casts to (unsigned long long) and it should work fine everywhere. --D > Luis
diff --git a/ltp/fsstress.c b/ltp/fsstress.c index 90ae432e..a576afea 100644 --- a/ltp/fsstress.c +++ b/ltp/fsstress.c @@ -9,6 +9,7 @@ #include <sys/uio.h> #include <stddef.h> #include <stdbool.h> +#include <string.h> #include "global.h" #ifdef HAVE_BTRFSUTIL_H @@ -943,9 +944,21 @@ check_cwd(void) { #ifdef DEBUG struct stat64 statbuf; + int ret; + + ret = stat64(".", &statbuf); + if (ret !=0) { + fprintf(stderr, "fsstress: check_cwd stat64 failed with: %d (%s)\n", + ret, strerror(ret)); + goto out; + } - if (stat64(".", &statbuf) == 0 && statbuf.st_ino == top_ino) + if (statbuf.st_ino == top_ino) return; + + fprintf(stderr, "fsstress: check_cwd statbuf.st_ino (%lu) != top_ino (%lu)\n", + statbuf.st_ino, top_ino); +out: assert(chdir(homedir) == 0); fprintf(stderr, "fsstress: check_cwd failure\n"); abort();
I ran into an error with generic/083 with xfs due to check_cwd() but why it failed is not clear because there are two types of failures: o stat64() failed (likely -ENOMEM is my guess) o the inode actually changed Throw a bone out to developers so that in case en error does happen they know which rabbit hole to go down on. Cc: Anthony Iliopoulos <ailiopoulos@suse.de> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- ltp/fsstress.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)