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

[Jan Stancek]

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

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

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

I tried only quickly the idea with extra declaration:

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);
 #endif

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