| Message ID | 20210921175035.1286786-1-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fsstress: improve error message on check_cwd() error | expand |
On Tue, Sep 21, 2021 at 6:51 PM Luis Chamberlain <mcgrof@kernel.org> 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 > > Thow 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(-) > > diff --git a/ltp/fsstress.c b/ltp/fsstress.c > index b4ddf5e2..d2f09901 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)); The error is not in 'ret' but instead in 'errno', so it should print errno and strerror(errno). > + 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 (%ld) != top_ino (%ld)\n", > + (long)statbuf.st_ino, (long)top_ino); ino_t is guaranteed to be an unsigned integer, and can be either 64 or 32 bits wide [1]. Casting to unsigned 64 bits would be better imo. Thanks. https://www.gnu.org/software/libc/manual/html_node/Attribute-Meanings.html > +out: > assert(chdir(homedir) == 0); > fprintf(stderr, "fsstress: check_cwd failure\n"); > abort(); > -- > 2.30.2 >
On Wed, Sep 22, 2021 at 11:15:08AM +0100, Filipe Manana wrote: > On Tue, Sep 21, 2021 at 6:51 PM Luis Chamberlain <mcgrof@kernel.org> 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 > > > > Thow 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(-) > > > > diff --git a/ltp/fsstress.c b/ltp/fsstress.c > > index b4ddf5e2..d2f09901 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)); > > The error is not in 'ret' but instead in 'errno', so it should print > errno and strerror(errno). Good call will fix. > > + 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 (%ld) != top_ino (%ld)\n", > > + (long)statbuf.st_ino, (long)top_ino); > > ino_t is guaranteed to be an unsigned integer, and can be either 64 or > 32 bits wide [1]. > Casting to unsigned 64 bits would be better imo. There are other similar mi-uses in the code, should I fix those to then? Luis
On Wed, Sep 22, 2021 at 9:07 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > On Wed, Sep 22, 2021 at 11:15:08AM +0100, Filipe Manana wrote: > > On Tue, Sep 21, 2021 at 6:51 PM Luis Chamberlain <mcgrof@kernel.org> 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 > > > > > > Thow 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(-) > > > > > > diff --git a/ltp/fsstress.c b/ltp/fsstress.c > > > index b4ddf5e2..d2f09901 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)); > > > > The error is not in 'ret' but instead in 'errno', so it should print > > errno and strerror(errno). > > Good call will fix. > > > > + 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 (%ld) != top_ino (%ld)\n", > > > + (long)statbuf.st_ino, (long)top_ino); > > > > ino_t is guaranteed to be an unsigned integer, and can be either 64 or > > 32 bits wide [1]. > > Casting to unsigned 64 bits would be better imo. > > There are other similar mi-uses in the code, should I fix those to > then? If you want to, as a separate patch. Noticing now that since the code is using struct stat64, using %llu for the printfs without any casts should be fine [1]. Thanks. [1] https://www.mkssoftware.com/docs/man5/struct_stat.5.asp > > Luis >
On Thu, Sep 23, 2021 at 01:37:04PM +0100, Filipe Manana wrote: > On Wed, Sep 22, 2021 at 9:07 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > On Wed, Sep 22, 2021 at 11:15:08AM +0100, Filipe Manana wrote: > > > > + fprintf(stderr, "fsstress: check_cwd statbuf.st_ino (%ld) != top_ino (%ld)\n", > > > > + (long)statbuf.st_ino, (long)top_ino); > > > > > > ino_t is guaranteed to be an unsigned integer, and can be either 64 or > > > 32 bits wide [1]. > > > Casting to unsigned 64 bits would be better imo. > > > > There are other similar mi-uses in the code, should I fix those to > > then? > > If you want to, as a separate patch. > Since the code is using struct stat64, using %llu > for the printfs without any casts should be fine [1]. You mean %lu because with %llu there is a warning. Luis
diff --git a/ltp/fsstress.c b/ltp/fsstress.c index b4ddf5e2..d2f09901 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 (%ld) != top_ino (%ld)\n", + (long)statbuf.st_ino, (long)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 Thow 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(-)