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

[Khem Raj]

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

>
> Thanks,
> Gao Xiang
>
> >  if ENABLE_LZ4
> >  liberofs_la_CFLAGS += ${LZ4_CFLAGS}
> >  liberofs_la_SOURCES += compressor_lz4.c
> > diff --git a/mkfs/main.c b/mkfs/main.c
> > index d2c9830..0e601d9 100644
> > --- a/mkfs/main.c
> > +++ b/mkfs/main.c
> > @@ -5,6 +5,7 @@
> >   * Created by Li Guifu <bluce.liguifu at huawei.com>
> >   */
> >  #define _GNU_SOURCE
> > +#include "config.h"
> >  #include <time.h>
> >  #include <sys/time.h>
> >  #include <stdlib.h>
> > --
> > 2.38.1
> >