Re: [LTP] [PATCH v5 4/7] Hugetlb: Safe macro for posix_fadvise call

Tarun Sahu
2022-11-25T17:35:25Z

On Nov 24 2022, Cyril Hrubis wrote:
> Hi!
> > include/tst_safe_macros.h | 17 +++++++++++++++++
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/include/tst_safe_macros.h b/include/tst_safe_macros.h
> > index 81c4b0844..4965e44d0 100644
> > --- a/include/tst_safe_macros.h
> > +++ b/include/tst_safe_macros.h
> > @@ -298,6 +298,23 @@ static inline int safe_ftruncate(const char *file, const int lineno,
> > #define SAFE_FTRUNCATE(fd, length) \
> > safe_ftruncate(__FILE__, __LINE__, (fd), (length))
> >
> > +static inline int safe_posix_fadvise(const char *file, const int lineno,
> > + int fd, off_t offset, off_t len, int advice)
> > +{
> > + int rval;
> > +
> > + rval = posix_fadvise(fd, offset, len, advice);
> > +
> > + if (rval)
> > + tst_brk_(file, lineno, TBROK | TERRNO,
> > + "posix_fadvise(%d,%ld,%ld,%d) failed",
> > + fd, (long)offset, (long)len, advice);
>
> I did a closer look at the posix_fadvise() manual and it returns an
> error in case of a failure, so the TERRNO is wrong here since that
> would print whatever was the last error stored in there.
>
> So we either have to do errno = rval; in the if (rval) branch or use
> tst_strerrno() to print the errno ourselves.
>
> Looking at the code we have the safe_pthread.c does use tst_strerrno()
> to print the error, so I would be inclined to do so here as well.
>

posix_fadvise should be inline, and so for the convention it will be part of
tst_safe_macros.h
tst_strerrno is defined in lib/errnos.h

there are only two such header file declared in this way. errnos.h and
signame.h. To be able to use tst_strerrno, it will require some cleanup
(like converting .h to .c and transfer PAIR, STRPAIR macros definition to
header files too from tst_res.c).

As, Even looking at past commits, I am not sure why these two file are
implememnted as .h, and not available as in general. I am currently
thinking of errno = rval way.

Can those file be converted?

> Also LKML coding style prefers curly braces around multiline blocks, so
> I would enclose the multiline tst_brk_() into curly braces, but that is
> very minor.
>
> > + return rval;
> > +}
> > +#define SAFE_POSIX_FADVISE(fd, offset, len, advice) \
> > + safe_posix_fadvise(__FILE__, __LINE__, (fd), (offset), (len), (advice))
> > +
> > static inline int safe_truncate(const char *file, const int lineno,
> > const char *path, off_t length)
> > {
> > --
> > 2.31.1
> >
>
> --
> Cyril Hrubis
> chrubis@suse.cz
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp