Subject: [PATCH] btrfs-progs: Fix the test for EXT4_EPOCH_MASK

Subject: [PATCH] btrfs-progs: Fix the test for EXT4_EPOCH_MASK

[Pierre Labastie]

Commit b3df561fbf has introduced the ability to convert extended
inode time precision on ext4, but this breaks builds on older distros,
where ext4 does not have the nsec time precision.

Commit c615287cc tried to fix that by testing the availability of
the EXT4_EPOCH_MASK macro, but the test is not complete.

This patch aims at fixing the macro test, and changes the
name of the associated HAVE_ macro, since the logic is reverted.

This fixes #353 when ext4 has nsec time precision. Note that
the test fails when ext4 does not have the nsec time precision.
Maybe the test shouldn't be run in that case?

Issue: #353
Signed-off-by: Pierre Labastie <pierre.labastie@neuf.fr>
---
 configure.ac          | 8 ++++----
 convert/source-ext2.c | 6 +++---
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/configure.ac b/configure.ac
index 612a3f87..dd6a5de7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -251,10 +251,10 @@ else
 AC_DEFINE([HAVE_OWN_FIEMAP_EXTENT_SHARED_DEFINE], [0], [We did not define
FIEMAP_EXTENT_SHARED])
 fi
 
-HAVE_OWN_EXT4_EPOCH_MASK_DEFINE=0
-AX_CHECK_DEFINE([ext2fs/ext2_fs.h], [EXT4_EPOCH_MASK], [],
-		[HAVE_OWN_EXT4_EPOCH_MASK_DEFINE=1
-		 AC_MSG_WARN([no definition of EXT4_EPOCH_MASK found, probably
old e2fsprogs, will use own definition, no 64bit time precision of converted
images])])
+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])])
 
 dnl Define <NAME>_LIBS= and <NAME>_CFLAGS= by pkg-config
 dnl
diff --git a/convert/source-ext2.c b/convert/source-ext2.c
index bd872086..fb655ec0 100644
--- a/convert/source-ext2.c
+++ b/convert/source-ext2.c
@@ -698,7 +698,7 @@ static void ext2_copy_inode_item(struct btrfs_inode_item
*dst,
 	memset(&dst->reserved, 0, sizeof(dst->reserved));
 }
 
-#if HAVE_OWN_EXT4_EPOCH_MASK_DEFINE
+#if HAVE_EXT4_EPOCH_MASK_DEFINE
 
 /*
  * Copied and modified from fs/ext4/ext4.h
@@ -769,7 +769,7 @@ out:
 	return ret;
 }
 
-#else /* HAVE_OWN_EXT4_EPOCH_MASK_DEFINE */
+#else /* HAVE_EXT4_EPOCH_MASK_DEFINE */
 
 static int ext4_copy_inode_timespec_extra(struct btrfs_inode_item *dst,
 				ext2_ino_t ext2_ino, u32 s_inode_size,
@@ -786,7 +786,7 @@ static int ext4_copy_inode_timespec_extra(struct
btrfs_inode_item *dst,
 	return 0;
 }
 
-#endif /* !HAVE_OWN_EXT4_EPOCH_MASK_DEFINE */
+#endif /* !HAVE_EXT4_EPOCH_MASK_DEFINE */
 
 static int ext2_check_state(struct btrfs_convert_context *cctx)
 {
-- 
2.30.1

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

[Pierre Labastie]

After sending the first version of the patch, I realized that it
was flawed, because of some formatting by the MUA. It took me
some time to set up an MTA so that git send-email works. Now the
patch should apply cleanly. Please remove the present paragraph by using
git am -c. Apologies for the inconvenience(s).
-- >8 --
Commit b3df561fbf has introduced the ability to convert extended
inode time precision on ext4, but this breaks builds on older distros,
where ext4 does not have the nsec time precision.

Commit c615287cc tried to fix that by testing the availability of
the EXT4_EPOCH_MASK macro, but the test is not complete.

This patch aims at fixing the macro test, and changes the
name of the associated HAVE_ macro, since the logic is reverted.

This fixes #353 when ext4 has nsec time precision. Note that
the test fails when ext4 does not have the nsec time precision.
Maybe the test shouldn't be run in that case?

Issue: #353
Signed-off-by: Pierre Labastie <pierre.labastie@neuf.fr>
---
 configure.ac          | 8 ++++----
 convert/source-ext2.c | 6 +++---
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/configure.ac b/configure.ac
index 612a3f87..dd6a5de7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -251,10 +251,10 @@ else
 AC_DEFINE([HAVE_OWN_FIEMAP_EXTENT_SHARED_DEFINE], [0], [We did not define FIEMAP_EXTENT_SHARED])
 fi
 
-HAVE_OWN_EXT4_EPOCH_MASK_DEFINE=0
-AX_CHECK_DEFINE([ext2fs/ext2_fs.h], [EXT4_EPOCH_MASK], [],
-		[HAVE_OWN_EXT4_EPOCH_MASK_DEFINE=1
-		 AC_MSG_WARN([no definition of EXT4_EPOCH_MASK found, probably old e2fsprogs, will use own definition, no 64bit time precision of converted images])])
+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])])
 
 dnl Define <NAME>_LIBS= and <NAME>_CFLAGS= by pkg-config
 dnl
diff --git a/convert/source-ext2.c b/convert/source-ext2.c
index bd872086..fb655ec0 100644
--- a/convert/source-ext2.c
+++ b/convert/source-ext2.c
@@ -698,7 +698,7 @@ static void ext2_copy_inode_item(struct btrfs_inode_item *dst,
 	memset(&dst->reserved, 0, sizeof(dst->reserved));
 }
 
-#if HAVE_OWN_EXT4_EPOCH_MASK_DEFINE
+#if HAVE_EXT4_EPOCH_MASK_DEFINE
 
 /*
  * Copied and modified from fs/ext4/ext4.h
@@ -769,7 +769,7 @@ out:
 	return ret;
 }
 
-#else /* HAVE_OWN_EXT4_EPOCH_MASK_DEFINE */
+#else /* HAVE_EXT4_EPOCH_MASK_DEFINE */
 
 static int ext4_copy_inode_timespec_extra(struct btrfs_inode_item *dst,
 				ext2_ino_t ext2_ino, u32 s_inode_size,
@@ -786,7 +786,7 @@ static int ext4_copy_inode_timespec_extra(struct btrfs_inode_item *dst,
 	return 0;
 }
 
-#endif /* !HAVE_OWN_EXT4_EPOCH_MASK_DEFINE */
+#endif /* !HAVE_EXT4_EPOCH_MASK_DEFINE */
 
 static int ext2_check_state(struct btrfs_convert_context *cctx)
 {
-- 
2.30.1

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

[David Sterba]

On Sun, Mar 14, 2021 at 07:49:13PM +0100, Pierre Labastie wrote:
> After sending the first version of the patch, I realized that it
> was flawed, because of some formatting by the MUA. It took me
> some time to set up an MTA so that git send-email works. Now the
> patch should apply cleanly. Please remove the present paragraph by using
> git am -c. Apologies for the inconvenience(s).
> -- >8 --
> Commit b3df561fbf has introduced the ability to convert extended
> inode time precision on ext4, but this breaks builds on older distros,
> where ext4 does not have the nsec time precision.
> 
> Commit c615287cc tried to fix that by testing the availability of
> the EXT4_EPOCH_MASK macro, but the test is not complete.
> 
> This patch aims at fixing the macro test, and changes the
> name of the associated HAVE_ macro, since the logic is reverted.
> 
> This fixes #353 when ext4 has nsec time precision. Note that
> the test fails when ext4 does not have the nsec time precision.
> Maybe the test shouldn't be run in that case?
> 
> Issue: #353
> Signed-off-by: Pierre Labastie <pierre.labastie@neuf.fr>
> ---
>  configure.ac          | 8 ++++----
>  convert/source-ext2.c | 6 +++---
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 612a3f87..dd6a5de7 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -251,10 +251,10 @@ else
>  AC_DEFINE([HAVE_OWN_FIEMAP_EXTENT_SHARED_DEFINE], [0], [We did not define FIEMAP_EXTENT_SHARED])
>  fi
>  
> -HAVE_OWN_EXT4_EPOCH_MASK_DEFINE=0
> -AX_CHECK_DEFINE([ext2fs/ext2_fs.h], [EXT4_EPOCH_MASK], [],
> -		[HAVE_OWN_EXT4_EPOCH_MASK_DEFINE=1
> -		 AC_MSG_WARN([no definition of EXT4_EPOCH_MASK found, probably old e2fsprogs, will use own definition, no 64bit time precision of converted images])])
> +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

This should work, maybe it's not the shortest way to write that but I
can't find anything better.

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

[Pierre Labastie]

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:
> > After sending the first version of the patch, I realized that it
> > was flawed, because of some formatting by the MUA. It took me
> > some time to set up an MTA so that git send-email works. Now the
> > patch should apply cleanly. Please remove the present paragraph by using
> > git am -c. Apologies for the inconvenience(s).
> > -- >8 --
> > Commit b3df561fbf has introduced the ability to convert extended
> > inode time precision on ext4, but this breaks builds on older distros,
> > where ext4 does not have the nsec time precision.
> > 
> > Commit c615287cc tried to fix that by testing the availability of
> > the EXT4_EPOCH_MASK macro, but the test is not complete.
> > 
> > This patch aims at fixing the macro test, and changes the
> > name of the associated HAVE_ macro, since the logic is reverted.
> > 
> > This fixes #353 when ext4 has nsec time precision. Note that
> > the test fails when ext4 does not have the nsec time precision.
> > Maybe the test shouldn't be run in that case?
> > 
> > Issue: #353
> > Signed-off-by: Pierre Labastie <pierre.labastie@neuf.fr>
> > ---
> >  configure.ac          | 8 ++++----
> >  convert/source-ext2.c | 6 +++---
> >  2 files changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/configure.ac b/configure.ac
> > index 612a3f87..dd6a5de7 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -251,10 +251,10 @@ else
> >  AC_DEFINE([HAVE_OWN_FIEMAP_EXTENT_SHARED_DEFINE], [0], [We did not define
> > FIEMAP_EXTENT_SHARED])
> >  fi
> >  
> > -HAVE_OWN_EXT4_EPOCH_MASK_DEFINE=0
> > -AX_CHECK_DEFINE([ext2fs/ext2_fs.h], [EXT4_EPOCH_MASK], [],
> > -               [HAVE_OWN_EXT4_EPOCH_MASK_DEFINE=1
> > -                AC_MSG_WARN([no definition of EXT4_EPOCH_MASK found,
> > probably old e2fsprogs, will use own definition, no 64bit time precision of
> > converted images])])
> > +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).

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

Pierre

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

[David Sterba]

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.

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

[David Sterba]

On Sun, Mar 14, 2021 at 07:49:13PM +0100, Pierre Labastie wrote:
> After sending the first version of the patch, I realized that it
> was flawed, because of some formatting by the MUA. It took me
> some time to set up an MTA so that git send-email works. Now the
> patch should apply cleanly. Please remove the present paragraph by using
> git am -c. Apologies for the inconvenience(s).
> -- >8 --
> Commit b3df561fbf has introduced the ability to convert extended
> inode time precision on ext4, but this breaks builds on older distros,
> where ext4 does not have the nsec time precision.
> 
> Commit c615287cc tried to fix that by testing the availability of
> the EXT4_EPOCH_MASK macro, but the test is not complete.
> 
> This patch aims at fixing the macro test, and changes the
> name of the associated HAVE_ macro, since the logic is reverted.
> 
> This fixes #353 when ext4 has nsec time precision. Note that
> the test fails when ext4 does not have the nsec time precision.
> Maybe the test shouldn't be run in that case?

Good point. What's the way to find that out? We can create a sample
ext4 filesystem and do a runtime check or parse it out of debugfs dump
looking for the features. I think it's the 'extra_isize', that's what
manual page ext4 says and that the patch adding 64bit timestamp support
checks when reading the extended timestamps.