Re: [PATCH v2] btrfs-progs: build system: Fix the test for EXT4_EPOCH_MASK

[David Sterba <dsterba@suse.cz>]

On Mon, Mar 15, 2021 at 11:19:47PM +0100, Pierre Labastie wrote:
> On Mon, 2021-03-15 at 15:53 +0100, David Sterba wrote:
> > On Sun, Mar 14, 2021 at 07:49:13PM +0100, Pierre Labastie wrote:
> > > +AX_CHECK_DEFINE([ext2fs/ext2_fs.h], [EXT4_EPOCH_MASK],
> > > +                [AC_DEFINE([HAVE_EXT4_EPOCH_MASK_DEFINE], [1],
> > > +                   [Define to 1 if e2fsprogs defines EXT4_EPOCH_MASK])],
> > > +               [AC_MSG_WARN([no definition of EXT4_EPOCH_MASK found,
> > > probably old e2fsprogs, will use own definition, no 64bit time precision of
> > > converted images])])
> > 
> > Inlining the AC_DEFINE to the check will skip defining the macro in case
> > the EXT4_EPOCH_MASK does not exist and then the C #if won't work.
> > 
> >         HAVE_EXT4_EPOCH_MASK_DEFINE=0
> >         AX_CHECK_DEFINE(...
> >                 HAVE_EXT4_EPOCH_MASK_DEFINE=1,...)
> > 
> >         if x"$HAVE_EXT4_EPOCH_MASK_DEFINE"; then
> >                 AC_DEFINE([HAVE_EXT4_EPOCH_MASK_DEFINE], [1])
> >         else
> >                 AC_DEFINE([HAVE_EXT4_EPOCH_MASK_DEFINE], [0])
> >         fi
> 
> Since autoheader is used, and autoheader uses AC_DEFINE macros to generate
> config.h.in, there is a risk that having two AC_DEFINE macros for the same 
> identifier generates a conflict. I've not been able to find what it does in
> that case in the autoconf doc.
> 
> OTOH, an undefined identifier is replaced by a zero when expanding a #if in the
> C preprocessor (this is in all the norms I have access to: C89 and C99), that
> is:
> 
> #if UNDEFINED_IDENTIFIER
> is equivalent to
> #if 0
> 
> Except if a compiler does not respect the norm, what I have proposed works (I
> have only tested with gcc, and it does what it is supposed to do).

I'll change it back to what you did as it sounds safer. The "#if
undefined" works for all compilers we currently care about (gcc, clang)
and would avoid potential problems with autoheader.

> > This should work, maybe it's not the shortest way to write that but I
> > can't find anything better.
> 
> Now, I've tested your version, and it works too. So, up to you...

I take yours, thanks. Feel free to send a fix to AC_DEFINE of
HAVE_OWN_FIEMAP_EXTENT_SHARED_DEFINE that does the double definition.
There were no problems reported so far but for consistency it would be
better to unify them.