Re: [LTP] [RFC] Dependency hell on static inline forced by off_t + _GNU_SOURCE

[Petr Vorel <pvorel@suse.cz>]

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.

> > 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,
> 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
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



-- 
Mailing list info: https://lists.linux.it/listinfo/ltp