Page MenuHome GnuPG

posix: don't use LFS64 types in struct linux_dirent64
Needs RevisionPublic

Authored by qyliss on May 3 2024, 1:46 PM.

Details

Reviewers
werner
Group Reviewers
gpgme
Summary

The *64_t types are transitional APIs for applications that do not yet fully support large files on 32-bit platforms. They have been removed in musl 1.2.5, which caused gpgme to fail to build. Since this is for a raw syscall anyway, it doesn't make sense to use libc-specific types here anyway, so I've changed this to match the definition of the struct used in the kernel (except there the kernel-specific u64 and s64 typedefs are used instead).

Test Plan

Try building on a system using musl 1.2.5 before and after this change. Also try building on Glibc. Verify that with this patch, gpgme builds and tests pass for both libc implementations.

Diff Detail

Repository
rM GPGME
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

qyliss requested review of this revision.May 3 2024, 1:46 PM
qyliss created this revision.
werner added a subscriber: werner.

This breaks existing 32 bit systems with the 64 bit types. Thus a test for off64_t is required which redefines it to int64_t if it does not exist.

This revision now requires changes to proceed.May 6 2024, 9:52 AM
In D600#6438, @werner wrote:

This breaks existing 32 bit systems with the 64 bit types.

Breaks them how?

Breaks them how?

off64_t mat not the same as int64_t

Maybe it is easier to fix musl?

off64_t mat not the same as int64_t

Right, but off64_t isn't the type that the kernel uses for this anyway, so it doesn't matter what off64_t is. We're talking directly to the kernel here, not going through a libc wrapper, so we want to match the kernel type, which is s64 i.e. int64_t.

Maybe it is easier to fix musl?

musl removed these deprecated APIs on purpose.

If it is intentional change by musl (requiring some changes by an application), we can use __ino64_t_defined and __off64_t_defined macro to see if those types are defined or not.

diff --git a/src/posix-io.c b/src/posix-io.c
index a422d8f6..505d1627 100644
--- a/src/posix-io.c
+++ b/src/posix-io.c
@@ -70,6 +70,12 @@
 
 
 #ifdef USE_LINUX_GETDENTS
+# ifndef __ino64_t_defined
+typedef uint64_t ino64_t
+# endif
+# ifndef __off64_t_defined
+typedef int64_t off64_t
+# endif
 /* This is not declared in public headers; getdents64(2) says that we must
  * define it ourselves.  */
 struct linux_dirent64
In D600#6445, @gniibe wrote:

If it is intentional change by musl (requiring some changes by an application), we can use __ino64_t_defined and __off64_t_defined macro to see if those types are defined or not.

I understand that, but the question is, why would we ever want to use ino64_t and off64_t here, even if they do exist, when that's not what the kernel uses anyway, and we're communicating with it directly rather than through libc?

$ man getdents64

getdents64()
    The  original  Linux getdents() system call did not handle large filesystems and large file offsets.  Consequently, Linux 2.4
    added getdents64(), with wider types for the d_ino and d_off fields.  In addition, getdents64() supports an  explicit  d_type
    field.

    The  getdents64()  system call is like getdents(), except that its second argument is a pointer to a buffer containing struc‐
    tures of the following type:

        struct linux_dirent64 {
            ino64_t        d_ino;    /* 64-bit inode number */
            off64_t        d_off;    /* 64-bit offset to next structure */
            unsigned short d_reclen; /* Size of this dirent */
            unsigned char  d_type;   /* File type */
            char           d_name[]; /* Filename (null-terminated) */
        };

$ man getdents64

getdents64()
    The  original  Linux getdents() system call did not handle large filesystems and large file offsets.  Consequently, Linux 2.4
    added getdents64(), with wider types for the d_ino and d_off fields.  In addition, getdents64() supports an  explicit  d_type
    field.

    The  getdents64()  system call is like getdents(), except that its second argument is a pointer to a buffer containing struc‐
    tures of the following type:

        struct linux_dirent64 {
            ino64_t        d_ino;    /* 64-bit inode number */
            off64_t        d_off;    /* 64-bit offset to next structure */
            unsigned short d_reclen; /* Size of this dirent */
            unsigned char  d_type;   /* File type */
            char           d_name[]; /* Filename (null-terminated) */
        };

Okay, I'll take that up on the man-pages list and see if I can get it fixed.

Okay.

/include/linux/dirent.h defines

struct linux_dirent64 {
	u64		d_ino;
	s64		d_off;
	unsigned short	d_reclen;
	unsigned char	d_type;
	char		d_name[];
};

Ah, I see what's up with the man page. It's documenting the Glibc getdents64() wrapper interface, so that's why it uses the Glibc types. But gpgme isn't using that wrapper, it's doing the syscall directly, so it should use the types the kernel uses, which as you've noticed are just generic unsigned and signed 64-bit integers, matching what my patch does.

Can this be accepted? Since GPGME is doing a direct syscall, rather than going through the libc wrapper, there's no need for it to use libc-specific types. It makes more sense (and is more portable) to use the sized types equivalent to the definition that the kernel uses.