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

[Gao Xiang]

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?

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

Thanks,
Gao Xiang