[PATCH] xfsprogs: don't include all xfs headers just for crc32

[PATCH] xfsprogs: don't include all xfs headers just for crc32

[Eric Sandeen]

Brian Norris reported that "The $BUILD_CC toolchain might have an
older set of Linux headers than the $CC toolchain. It's generally
unsafe to try to build both with the same definitions, but in
particular, this one can cause compilation failures in the local
crc32selftest build: [failure to find fsmap.h]"

It seems like the most straightforward thing to do here is include
a specific set of system headers, instead of pulling in the whole
xfs.h header chain which has multiple tests and definitions in
place for headers that may or may not be there during the build.

Reported-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Brian, here's try #3, does this also work for you?

diff --git a/libfrog/crc32.c b/libfrog/crc32.c
index 1d52f68..220b33b 100644
--- a/libfrog/crc32.c
+++ b/libfrog/crc32.c
@@ -29,8 +29,11 @@
  * match the hardware acceleration available on Intel CPUs.
  */
 
+#include <inttypes.h>
+#include <asm/types.h>
+#include <sys/time.h>
 #include "platform_defs.h"
-#include "xfs.h"
+/* For endian conversion routines */
 #include "xfs_arch.h"
 #include "crc32defs.h"
 

Re: [PATCH] xfsprogs: don't include all xfs headers just for crc32

[Brian Norris]

On Wed, Oct 10, 2018 at 03:55:32PM -0500, Eric Sandeen wrote:
> Brian Norris reported that "The $BUILD_CC toolchain might have an
> older set of Linux headers than the $CC toolchain. It's generally
> unsafe to try to build both with the same definitions, but in
> particular, this one can cause compilation failures in the local
> crc32selftest build: [failure to find fsmap.h]"
> 
> It seems like the most straightforward thing to do here is include
> a specific set of system headers, instead of pulling in the whole
> xfs.h header chain which has multiple tests and definitions in
> place for headers that may or may not be there during the build.
> 
> Reported-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> Brian, here's try #3, does this also work for you?

Tested-by: Brian Norris <briannorris@chromium.org>

This still leaves $BUILD_CFLAGS with bad entries, but as long as they
don't get used anywhere that matters, it'll be OK.

Brian

> diff --git a/libfrog/crc32.c b/libfrog/crc32.c
> index 1d52f68..220b33b 100644
> --- a/libfrog/crc32.c
> +++ b/libfrog/crc32.c
> @@ -29,8 +29,11 @@
>   * match the hardware acceleration available on Intel CPUs.
>   */
>  
> +#include <inttypes.h>
> +#include <asm/types.h>
> +#include <sys/time.h>
>  #include "platform_defs.h"
> -#include "xfs.h"
> +/* For endian conversion routines */
>  #include "xfs_arch.h"
>  #include "crc32defs.h"
>  
> 

Re: [PATCH] xfsprogs: don't include all xfs headers just for crc32

[Eric Sandeen]

On 10/11/18 2:59 PM, Brian Norris wrote:
> On Wed, Oct 10, 2018 at 03:55:32PM -0500, Eric Sandeen wrote:
>> Brian Norris reported that "The $BUILD_CC toolchain might have an
>> older set of Linux headers than the $CC toolchain. It's generally
>> unsafe to try to build both with the same definitions, but in
>> particular, this one can cause compilation failures in the local
>> crc32selftest build: [failure to find fsmap.h]"
>>
>> It seems like the most straightforward thing to do here is include
>> a specific set of system headers, instead of pulling in the whole
>> xfs.h header chain which has multiple tests and definitions in
>> place for headers that may or may not be there during the build.
>>
>> Reported-by: Brian Norris <briannorris@chromium.org>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> Brian, here's try #3, does this also work for you?
> 
> Tested-by: Brian Norris <briannorris@chromium.org>
> 
> This still leaves $BUILD_CFLAGS with bad entries, but as long as they
> don't get used anywhere that matters, it'll be OK.

Thanks.  At this point I think you have a better grasp of what all the
$FOO_FLAGS do than I do ;)  It may be  to keep things sorted
and separate, but it also seemed useful to get a giant tangle of
xfs header out of a non-xfs library file.

I'm also on the fence about whether cross-compiling the self-check
really even gains us much, given that it may be built or optimized
completely differently from the code on the target arch ...

-Eric

Re: [PATCH] xfsprogs: don't include all xfs headers just for crc32

[Darrick J. Wong]

On Thu, Oct 11, 2018 at 05:36:36PM -0500, Eric Sandeen wrote:
> 
> 
> On 10/11/18 2:59 PM, Brian Norris wrote:
> > On Wed, Oct 10, 2018 at 03:55:32PM -0500, Eric Sandeen wrote:
> >> Brian Norris reported that "The $BUILD_CC toolchain might have an
> >> older set of Linux headers than the $CC toolchain. It's generally
> >> unsafe to try to build both with the same definitions, but in
> >> particular, this one can cause compilation failures in the local
> >> crc32selftest build: [failure to find fsmap.h]"
> >>
> >> It seems like the most straightforward thing to do here is include
> >> a specific set of system headers, instead of pulling in the whole
> >> xfs.h header chain which has multiple tests and definitions in
> >> place for headers that may or may not be there during the build.
> >>
> >> Reported-by: Brian Norris <briannorris@chromium.org>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> >>
> >> Brian, here's try #3, does this also work for you?
> > 
> > Tested-by: Brian Norris <briannorris@chromium.org>
> > 
> > This still leaves $BUILD_CFLAGS with bad entries, but as long as they
> > don't get used anywhere that matters, it'll be OK.
> 
> Thanks.  At this point I think you have a better grasp of what all the
> $FOO_FLAGS do than I do ;)  It may be  to keep things sorted
> and separate, but it also seemed useful to get a giant tangle of
> xfs header out of a non-xfs library file.
> 
> I'm also on the fence about whether cross-compiling the self-check
> really even gains us much, given that it may be built or optimized
> completely differently from the code on the target arch ...

So long as it's a software algorithm with a big lookup table I guess
it's a serviceable sanity check that nobody totally goobered up the
source code, but I don't know if it adds much value in the cross
compilation case either.  If you ever wanted to add a faster
implementation in the built libxfs <cough> that would be more of an
issue.

(Granted, I've been running a patched xfsprogs with hw accelerated
crc32c for a year now and haven't noticed any difference in runtime.
Maybe now that I've finished upgrading everything to flash...)

--D

> -Eric

Re: [PATCH] xfsprogs: don't include all xfs headers just for crc32

[Darrick J. Wong]

On Thu, Oct 11, 2018 at 03:45:36PM -0700, Darrick J. Wong wrote:
> On Thu, Oct 11, 2018 at 05:36:36PM -0500, Eric Sandeen wrote:
> > 
> > 
> > On 10/11/18 2:59 PM, Brian Norris wrote:
> > > On Wed, Oct 10, 2018 at 03:55:32PM -0500, Eric Sandeen wrote:
> > >> Brian Norris reported that "The $BUILD_CC toolchain might have an
> > >> older set of Linux headers than the $CC toolchain. It's generally
> > >> unsafe to try to build both with the same definitions, but in
> > >> particular, this one can cause compilation failures in the local
> > >> crc32selftest build: [failure to find fsmap.h]"
> > >>
> > >> It seems like the most straightforward thing to do here is include
> > >> a specific set of system headers, instead of pulling in the whole
> > >> xfs.h header chain which has multiple tests and definitions in
> > >> place for headers that may or may not be there during the build.
> > >>
> > >> Reported-by: Brian Norris <briannorris@chromium.org>
> > >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > >> ---
> > >>
> > >> Brian, here's try #3, does this also work for you?
> > > 
> > > Tested-by: Brian Norris <briannorris@chromium.org>
> > > 
> > > This still leaves $BUILD_CFLAGS with bad entries, but as long as they
> > > don't get used anywhere that matters, it'll be OK.
> > 
> > Thanks.  At this point I think you have a better grasp of what all the
> > $FOO_FLAGS do than I do ;)  It may be  to keep things sorted
> > and separate, but it also seemed useful to get a giant tangle of
> > xfs header out of a non-xfs library file.
> > 
> > I'm also on the fence about whether cross-compiling the self-check
> > really even gains us much, given that it may be built or optimized
> > completely differently from the code on the target arch ...
> 
> So long as it's a software algorithm with a big lookup table I guess
> it's a serviceable sanity check that nobody totally goobered up the
> source code, but I don't know if it adds much value in the cross
> compilation case either.  If you ever wanted to add a faster
> implementation in the built libxfs <cough> that would be more of an
> issue.
> 
> (Granted, I've been running a patched xfsprogs with hw accelerated
> crc32c for a year now and haven't noticed any difference in runtime.
> Maybe now that I've finished upgrading everything to flash...)
> 

Looks ok, I guess...

(...and by "I guess" what I really mean is that given a lot of people
cross-compiling things these days I think I want a similar test of the
libxfs builtin crc32c algorithm that we can call from xfstests... but
that's not what's under review here. :))

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> --D
> 
> > -Eric

Re: [PATCH] xfsprogs: don't include all xfs headers just for crc32

[Brian Norris]

On Thu, Oct 11, 2018 at 3:36 PM Eric Sandeen <sandeen@sandeen.net> wrote:
> Thanks.  At this point I think you have a better grasp of what all the
> $FOO_FLAGS do than I do ;)  It may be  to keep things sorted
> and separate, but it also seemed useful to get a giant tangle of
> xfs header out of a non-xfs library file.

Yep! I'm happy with this patch. If I'm bothered enough, I can still
send another patch to sort out the flags further.

> I'm also on the fence about whether cross-compiling the self-check
> really even gains us much, given that it may be built or optimized
> completely differently from the code on the target arch ...

I don't have a whole lot of opinion here.

Regards,
Brian