[LTP] [RFC] Dependency hell on static inline forced by off_t + _GNU_SOURCE
Edward Liaw
edliaw@google.com
Wed Apr 24 22:49:54 CEST 2024
Hi Petr, sorry for the late reply,
On Fri, Apr 12, 2024 at 6:19 AM Petr Vorel <pvorel@suse.cz> wrote:
>
> Hi Jan, all,
>
> [ Cc Fabrice: fixed LTP issues on musl and uClibc-ng, Edward: bionic/AOSP ]
>
> > On Fri, Apr 12, 2024 at 1:46 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> > > Hi there,
>
> > > I have following function:
>
> > > #include "tst_fs.h"
> > > #include "lapi/fallocate.h"
>
> > > #define SAFE_FALLOCATE(fd, mode, offset, len) \
> > > safe_access(__FILE__, __LINE__, (path), (mode), (offset), (len), #mode)
>
> > ^^ you seem to have some mismatch here, macro name vs function name
> > and parameters
>
> Thanks!
>
> > > static inline int safe_fallocate(const char *file, const int lineno,
> > > int fd, int mode, off_t offset, off_t len, const char *smode)
> > > {
> > > int rval;
>
> > > rval = fallocate(fd, mode, offset, len);
>
> > > if (rval == -1) {
> > > if (tst_fs_type_(NULL, ".") == TST_NFS_MAGIC && (errno == EOPNOTSUPP ||
> > > errno == ENOSYS)) {
> > > tst_brk_(file, lineno, TCONF | TERRNO,
> > > "fallocate(%d, %s, %ld, %ld) unsupported",
> > > fd, smode, (long)offset, (long)len);
> > > }
> > > tst_brk_(file, lineno, TBROK | TERRNO,
> > > "fallocate(%d, %s, %ld, %ld) failed",
> > > fd, smode, (long)offset, (long)len);
> > > } else if (rval < 0) {
> > > tst_brk_(file, lineno, TBROK | TERRNO,
> > > "Invalid fallocate(%d, %s, %ld, %ld) return value %d",
> > > fd, smode, (long)offset, (long)len, rval);
> > > }
>
> > > return rval;
> > > }
>
> > > I have no idea where to put it.
> > > 1) fallocate() requires '#define _GNU_SOURCE'
>
> > Could we just provide a declaration for the func in lapi/? Or do we
> > risk not matching glibc func prototype in some environments?
>
> Thanks! I did not see this simply solution. It sounds a bit risky to me, but
> there should not be any diference. I wonder what others think.
>
> As it would allow keep using include/lapi/fallocate.h also by legacy tests, I'm
> slightly for this approach. Also keeping fallocate.h just with fallback
> definitions would keep it clean.
>
> Actually it's hard to get all #define _GNU_SOURCE on all places (it'd be easier
> to really compile everything with -D_GNU_SOURCE), thus regardless where we put
> the function having function prototype in include/lapi/fallocate.h would help to
> avoid _GNU_SOURCE hell. But I wonder what will be a fix if there is really some
> clash.
I'm not that experienced with C, and don't really understand the
implications of this; will compiling everything with -D_GNU_SOURCE
cause problems somewhere?
>
> > > 2) fallocate() off_t parameter requires to be in a header (see 9120d8a22 and
> > > 3f571da28).
> > > 3) Use of tst_fs_type_(NULL, ".") and TBROK etc requires tst_test.h.
>
> > I'm not sure I see the issue with 3). The tests that include
> > tst_safe_macros and tst_safe_macros_inline
> > should be already using new API.
>
> It's also used by various library sources (lib/*.c).
>
> > I tried only quickly the idea with extra declaration:
>
> Thanks a lot for this POC!
>
> > diff --git a/include/lapi/fallocate.h b/include/lapi/fallocate.h
> > index fc246bcfc168..2f5ea907d465 100644
> > --- a/include/lapi/fallocate.h
> > +++ b/include/lapi/fallocate.h
> > @@ -49,6 +49,8 @@ static inline long fallocate(int fd, int mode,
Hmm, I'm not clear on why it is defined to return long here, and int otherwise.
> > loff_t offset, loff_t len)
> > (off_t) len));
> > # endif
> > }
> > +#else
> > +int fallocate(int fd, int mode, off_t offset, off_t len);
>
> @Fabrice, @Edward do you see anything dangerous on this? It looks safe to me.
>
> > #endif
>
> FYI bionic, musl, uclibc-ng
>
> === bionic ===
>
> libc/include/fcntl.h
>
> /**
> * [fallocate(2)](http://man7.org/linux/man-pages/man2/fallocate.2.html)
> * is a Linux-specific extension of posix_fallocate().
> *
> * Valid flags are `FALLOC_FL_KEEP_SIZE`, `FALLOC_FL_PUNCH_HOLE`,
> * `FALLOC_FL_NO_HIDE_STALE`, `FALLOC_FL_COLLAPSE_RANGE`,
> * `FALLOC_FL_ZERO_RANGE`, `FALLOC_FL_INSERT_RANGE`, and
> * `FALLOC_FL_UNSHARE_RANGE`.
> *
> * Returns 0 on success and returns -1 and sets `errno` on failure.
> */
> int fallocate(int __fd, int __mode, off_t __offset, off_t __length) __RENAME_IF_FILE_OFFSET64(fallocate64);
> /** See fallocate(). */
> int fallocate64(int __fd, int __mode, off64_t __offset, off64_t __length);
>
> libc/include/sys/cdefs.h (our lovely off_t)
> /*
> * Note that __RENAME_IF_FILE_OFFSET64 is only valid if the off_t and off64_t
> * functions were both added at the same API level because if you use this,
> * you only have one declaration to attach __INTRODUCED_IN to.
> */
> # define __RENAME_IF_FILE_OFFSET64(func) __RENAME(func)
> #else
> # define __RENAME_IF_FILE_OFFSET64(func)
> #endif
>
> /* Used to rename functions so that the compiler emits a call to 'x' rather than the function this was applied to. */
> #define __RENAME(x) __asm__(#x)
>
> === musl ===
> include/fcntl.h
>
> /* pvorel: note not under _GNU_SOURCE
@enh pointed out that this is actually under
#if defined(_GNU_SOURCE)
see: https://git.musl-libc.org/cgit/musl/tree/include/fcntl.h#n162
> int fallocate(int, int, off_t, off_t);
>
> #if defined(_GNU_SOURCE)
> #define fallocate64 fallocate
> #endif
>
> === uClibc-ng ===
> include/fcntl.h
>
> #if (defined __UCLIBC_LINUX_SPECIFIC__ && defined __USE_GNU) || defined _LIBC
> /* Reserve storage for the data of a file associated with FD. This function
> is Linux-specific. For the portable version, use posix_fallocate().
> Unlike the latter, fallocate can operate in different modes. The default
> mode = 0 is equivalent to posix_fallocate().
>
> Note: These declarations are used in posix_fallocate.c and
> posix_fallocate64.c, so we expose them internally.
> */
>
> /* Flags for fallocate's mode. */
> # define FALLOC_FL_KEEP_SIZE 1 /* Don't extend size of file
> even if offset + len is
> greater than file size. */
> # define FALLOC_FL_PUNCH_HOLE 2 /* Create a hole in the file. */
>
> # ifndef __USE_FILE_OFFSET64
> extern int fallocate (int __fd, int __mode, __off_t __offset, __off_t __len);
> # else
> # ifdef __REDIRECT
> extern int __REDIRECT (fallocate, (int __fd, int __mode, __off64_t __offset,
> __off64_t __len),
> fallocate64);
> # else
> # define fallocate fallocate64
> # endif
> # endif
> # ifdef __USE_LARGEFILE64
> extern int fallocate64 (int __fd, int __mode, __off64_t __offset, __off64_t __len);
> # endif
> #endif
>
> Kind regards,
> Petr
>
> > #endif /* LAPI_FALLOCATE_H__ */
> > diff --git a/include/tst_safe_macros_inline.h b/include/tst_safe_macros_inline.h
> > index c497f60599d5..e229bead8372 100644
> > --- a/include/tst_safe_macros_inline.h
> > +++ b/include/tst_safe_macros_inline.h
> > @@ -226,4 +226,36 @@ static inline int safe_setrlimit(const char
> > *file, const int lineno,
> > #define SAFE_SETRLIMIT(resource, rlim) \
> > safe_setrlimit(__FILE__, __LINE__, (resource), (rlim))
>
> > +
> > +#include "lapi/fallocate.h"
> > +
> > +#define SAFE_FALLOCATE(fd, mode, offset, len) \
> > + safe_fallocate(__FILE__, __LINE__, (fd), (mode), (offset),
> > (len), #mode)
> > +
> > +static inline int safe_fallocate(const char *file, const int lineno,
> > + int fd, int mode, off_t offset, off_t len, const char *smode)
> > +{
> > + int rval;
> > +
> > + rval = fallocate(fd, mode, offset, len);
> > +
> > + if (rval == -1) {
> > + if (tst_fs_type_(NULL, ".") == TST_NFS_MAGIC &&
> > (errno == EOPNOTSUPP ||
> > + errno == ENOSYS)) {
> > + tst_brk_(file, lineno, TCONF | TERRNO,
> > + "fallocate(%d, %s, %ld, %ld)
> > unsupported",
> > + fd, smode, (long)offset, (long)len);
> > + }
> > + tst_brk_(file, lineno, TBROK | TERRNO,
> > + "fallocate(%d, %s, %ld, %ld) failed",
> > + fd, smode, (long)offset, (long)len);
> > + } else if (rval < 0) {
> > + tst_brk_(file, lineno, TBROK | TERRNO,
> > + "Invalid fallocate(%d, %s, %ld, %ld) return value %d",
> > + fd, smode, (long)offset, (long)len, rval);
> > + }
> > +
> > + return rval;
> > +}
> > +
> > #endif /* TST_SAFE_MACROS_INLINE_H__ */
>
>
> > > I tried to put it into:
>
> > > a) include/tst_safe_macros_inline.h
> > > Natural choice, but that would require to add to include/tst_test.h:
>
> > > #ifndef _GNU_SOURCE
> > > # define _GNU_SOURCE
> > > #endif
>
> > > because it includes tst_safe_macros.h. Which means whole new API started to use
> > > _GNU_SOURCE. Would it be OK? I don't think so.
>
> > > And #define _GNU_SOURCE into tst_test.c and few other lib/*.c sources (not that dramatic),
> > > because we cannot rely on <fcntl.h> not being loaded before #define _GNU_SOURCE.
>
> > > b) include/lapi/fallocate.h
> > > I'm not sure if this is against LTP lapi conventions, because it would require
> > > lapi header include tst_test.h due tst_fs_type_ and TBROK. Also, we'd make it
> > > new API dependent (thus use tst_fs_type(".") instead of tst_fs_type_(NULL, ".")
>
> > > Also we have error on fallocate01.c which is still old API:
>
> > > from fallocate01.c:103:
> > > ../../../../include/tst_test.h:11:3: error: #error Oldlib test.h already included
> > > 11 | # error Oldlib test.h already included
>
> > > I could rewrite fallocate01.c and fallocate02.c first, so that there is nothing
> > > using old API which also uses include/lapi/fallocate.h.
>
> > > Another solution would be to pass int parameter fsmagic so that the caller would
> > > have to run tst_fs_type(".") itself.
>
> > > Kind regards,
> > > Petr
>
>
More information about the ltp
mailing list