[PATCH 2/3] erofs_fs.h: Make LFS mandatory for all usecases

[Khem Raj]

On Sat, Dec 10, 2022 at 7:03 AM Gao Xiang <xiang at kernel.org> wrote:
>
> Hi Khem,
>
> On Fri, Dec 09, 2022 at 06:20:12PM -0800, Khem Raj wrote:
> > On Thu, Dec 8, 2022 at 6:18 AM Gao Xiang <xiang at kernel.org> wrote:
> > >
> > > Hi Khem,
> > >
> > > On Thu, Dec 08, 2022 at 12:53:34AM -0800, Khem Raj wrote:
> > > > erosfs depend on the consistent use of a 64bit offset
> > >
> > > Thanks for your patch!
> > >
> > >   ^ erofs
> >
> > Done in v2
> >
> > >
> > > > type, force downstreams to use transparent LFS (_FILE_OFFSET_BITS=64),
> > > > so that it becomes impossible for them to use 32bit interfaces.
> > > >
> > > > include autoconf'ed config.h to get definition of _FILE_OFFSET_BITS
> > > > which was detected by configure. This header needs to be included
> > > > before any system headers are included to ensure they see the correct
> > > > definition of _FILE_OFFSET_BITS for the platform
> > > >
> > > > Signed-off-by: Khem Raj <raj.khem at gmail.com>
> > > > ---
> > >
> > > ...
> > >
> > > > diff --git a/include/erofs/internal.h b/include/erofs/internal.h
> > > > index 6a70f11..9cc20a8 100644
> > > > --- a/include/erofs/internal.h
> > > > +++ b/include/erofs/internal.h
> > > > @@ -12,6 +12,7 @@ extern "C"
> > > >  {
> > > >  #endif
> > > >
> > > > +#include <config.h>
> > >
> > > could we use alternative way? since I'd like to make include/ as
> > > liberofs later, and "config.h" autoconf seems weird to me...
> > >
> >
> > I am using the AC_SYS_LARGEFILE macro from autoconf to enable support for
> > largefile support during configure. configure will generate config.h
> > in build dir which
> > will contain the essential macros which we use e.g. _FILE_OFFSET_BITS defined
> > to right values. Alternate way is to pass it _always_ or demand it to
> > be passed from
> > user. Which in a way it will do with internal.h check added in this
> > series. I am fine
> > if you do not want to depend on autoconf support to enable LFS. Let me know.
> >
> > > >  #include "list.h"
> > > >  #include "err.h"
> > > >
> > > > diff --git a/include/erofs_fs.h b/include/erofs_fs.h
> > > > index 08f9761..a3bd93c 100644
> > > > --- a/include/erofs_fs.h
> > > > +++ b/include/erofs_fs.h
> > > > @@ -9,6 +9,8 @@
> > > >  #ifndef __EROFS_FS_H
> > > >  #define __EROFS_FS_H
> > > >
> > > > +#include <sys/types.h>
> > >
> > > Could you give more hints why we need this here?
> >
> > Its needed to get off_t defined, I have added a comment here
> > in v2i
>
> but we don't use off_t in erofs_fs.h?

the new check I added does use it.

>
> > >
> > > > +
> > > >  #define EROFS_SUPER_MAGIC_V1    0xE0F5E1E2
> > > >  #define EROFS_SUPER_OFFSET      1024
> > > >
> > > > @@ -410,6 +412,10 @@ enum {
> > > >
> > > >  #define EROFS_NAME_LEN      255
> > > >
> > > > +
> > > > +/* make sure that any user of the erofs headers has atleast 64bit off_t type */
> > > > +extern int eros_assert_largefile[sizeof(off_t)-8];
> > >
> > > erofs? also you could add this into erofs/internal.h...
> > >
> > > This file is just the on-disk definition...
> >
> > yeah moved the check to internal.h in v2
> >
> > >
> > > > +
> > > >  /* check the EROFS on-disk layout strictly at compile time */
> > > >  static inline void erofs_check_ondisk_layout_definitions(void)
> > > >  {
> > > > diff --git a/lib/Makefile.am b/lib/Makefile.am
> > > > index 3fad357..88400ed 100644
> > > > --- a/lib/Makefile.am
> > > > +++ b/lib/Makefile.am
> > > > @@ -28,7 +28,7 @@ noinst_HEADERS += compressor.h
> > > >  liberofs_la_SOURCES = config.c io.c cache.c super.c inode.c xattr.c exclude.c \
> > > >                     namei.c data.c compress.c compressor.c zmap.c decompress.c \
> > > >                     compress_hints.c hashmap.c sha256.c blobchunk.c dir.c
> > > > -liberofs_la_CFLAGS = -Wall -I$(top_srcdir)/include
> > > > +liberofs_la_CFLAGS = -Wall -I$(top_builddir) -I$(top_srcdir)/include -include config.h
> > >
> > > same here too...
> >
> > as said above if we are ok to pass it always then we can add -D
> > _FILE_OFFSET_BITS=64 via toplevel Makefile.am
> > it will only be needed on 32bit systems though, so maybe we do not
> > define it and demand it from users via CFLAGS
> > if they compile it for 32bit systems.
> >
>
> I think use -D _FILE_OFFSET_BITS=64 would be a better choice...
>

OK I will rework it in v3.

> Thanks,
> Gao Xiang