Re: [BUG] xfsprogs-4.7.0: issues cross-compiling for musl

Re: [BUG] xfsprogs-4.7.0: issues cross-compiling for musl

[Dave Chinner]

On Wed, Sep 07, 2016 at 11:42:56AM +0200, Ralph Sennhauser wrote:
> To get xfsprogs to cross-compile for musl some hacks were needed.
> 
> 1) d_namlen isn't available with musl

musl problem. It needs to define _DIRENT_HAVE_D_RECLEN and friends
to tell applications what the struct dirent it defines contains.

(Haven't we been through this before?)

> 2) crc32selftest doesn't make sense in case of cross-compilation, the
> same can be said if building for a different host with the native
> toolchain. So maybe an --disable-crc32selftest configure option could
> be used here.
> 
> 3) The CFLAGS passed to BUILD_CC weren't understood by the native
> compiler. Set BUILD_CFLAGS to CFLAGS if not cross-compiling or let it
> blank anyway?

Ah, yes, we've definitely been through this before. Last time I said
"send patches" and they never appeared.

As it is, I'm really hesitant to remove the crc32c sanity check,
mainly because we've just been through a XFS corruption drill where
one in 10^6 crc calculations would end up wrong because of a bug in
a compiler.

Can we "auto detect" cross compilation in the makefile and avoid the
check only in that situation? configure options are really only a
last resort...

> 4) sys/ustat.h is missing in musl.

Yup, been reported before. I'm about to commit a fix for that,
mainly because certain new arches don't even implement the ustat
system call, and glibc completely fucks up the handling of that.
i.e. programs that call ustat don't fail at build time, the syscall
is stubbed to print "not supported, will fail" at runtime. Hence
nobody noticed that the xfsprogs code calling ustat was actually
broken until a week ago...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [BUG] xfsprogs-4.7.0: issues cross-compiling for musl

[Eric Sandeen]

On 9/7/16 5:27 PM, Dave Chinner wrote:
> On Wed, Sep 07, 2016 at 11:42:56AM +0200, Ralph Sennhauser wrote:
>> To get xfsprogs to cross-compile for musl some hacks were needed.
>>
>> 1) d_namlen isn't available with musl
> 
> musl problem. It needs to define _DIRENT_HAVE_D_RECLEN and friends
> to tell applications what the struct dirent it defines contains.
> 
> (Haven't we been through this before?)

(yes: http://www.openwall.com/lists/musl/2016/01/15/9)

what *does* it contain?

We do already have:

 #ifdef _DIRENT_HAVE_D_RECLEN
 		*total += dirent->d_reclen;
 #else
		*total += dirent->d_namlen + sizeof(*dirent);
 #endif

but you'd like:

 #ifdef _DIRENT_HAVE_D_RECLEN
 		*total += dirent->d_reclen;
 #else
-		*total += dirent->d_namlen + sizeof(*dirent);
+		*total += strlen(dirent->d_name) + sizeof(*dirent);
 #endif

but musl *does* have d_reclen:

http://git.musl-libc.org/cgit/musl/tree/include/dirent.h

so if it simply advertised that fact we'd be good...

Whatever happened to that request from January above?

That said, I suppose strlen is fine to use here, so I guess I don't
see a strong reason to object to this change.

>> 2) crc32selftest doesn't make sense in case of cross-compilation, the
>> same can be said if building for a different host with the native
>> toolchain. So maybe an --disable-crc32selftest configure option could
>> be used here.
>>
>> 3) The CFLAGS passed to BUILD_CC weren't understood by the native
>> compiler. Set BUILD_CFLAGS to CFLAGS if not cross-compiling or let it
>> blank anyway?
> 
> Ah, yes, we've definitely been through this before. Last time I said
> "send patches" and they never appeared.
> 
> As it is, I'm really hesitant to remove the crc32c sanity check,
> mainly because we've just been through a XFS corruption drill where
> one in 10^6 crc calculations would end up wrong because of a bug in
> a compiler.
> 
> Can we "auto detect" cross compilation in the makefile and avoid the
> check only in that situation? configure options are really only a
> last resort...

That sounds like the right path to me.

Or cross-compile it but don't run it, and let package managers run
it at install time?  ;)

>> 4) sys/ustat.h is missing in musl.
> 
> Yup, been reported before. I'm about to commit a fix for that,
> mainly because certain new arches don't even implement the ustat
> system call, and glibc completely fucks up the handling of that.
> i.e. programs that call ustat don't fail at build time, the syscall
> is stubbed to print "not supported, will fail" at runtime. Hence
> nobody noticed that the xfsprogs code calling ustat was actually
> broken until a week ago...

Partly because we only checked for success, not failure, but nobody
rationally expected -ENOSYS any more than they expected the
Spanish Inquisition...

-Eric

> Cheers,
> 
> Dave.
> 

Re: [BUG] xfsprogs-4.7.0: issues cross-compiling for musl

[Ralph Sennhauser]

Hi Dave,

will answer to 1-3 via Erics mail

On Thu, 8 Sep 2016 08:27:53 +1000
Dave Chinner <david@fromorbit.com> wrote:

> On Wed, Sep 07, 2016 at 11:42:56AM +0200, Ralph Sennhauser wrote:
> > 4) sys/ustat.h is missing in musl.  
> 
> Yup, been reported before. I'm about to commit a fix for that,
> mainly because certain new arches don't even implement the ustat
> system call, and glibc completely fucks up the handling of that.
> i.e. programs that call ustat don't fail at build time, the syscall
> is stubbed to print "not supported, will fail" at runtime. Hence
> nobody noticed that the xfsprogs code calling ustat was actually
> broken until a week ago...
> 
> Cheers,
> 
> Dave.

So this one I can consider taken care of, very nice!

Thanks
Ralph

Re: [BUG] xfsprogs-4.7.0: issues cross-compiling for musl

[Ralph Sennhauser]

Hi Eric

On Wed, 7 Sep 2016 17:40:01 -0500
Eric Sandeen <sandeen@sandeen.net> wrote:

> On 9/7/16 5:27 PM, Dave Chinner wrote:
> > On Wed, Sep 07, 2016 at 11:42:56AM +0200, Ralph Sennhauser wrote:  
> >> To get xfsprogs to cross-compile for musl some hacks were needed.
> >>
> >> 1) d_namlen isn't available with musl  
> > 
> > musl problem. It needs to define _DIRENT_HAVE_D_RECLEN and friends
> > to tell applications what the struct dirent it defines contains.
> > 
> > (Haven't we been through this before?)  
> 
> (yes: http://www.openwall.com/lists/musl/2016/01/15/9)
> 
> what *does* it contain?
> 
> We do already have:
> 
>  #ifdef _DIRENT_HAVE_D_RECLEN
>  		*total += dirent->d_reclen;
>  #else
> 		*total += dirent->d_namlen + sizeof(*dirent);
>  #endif
> 
> but you'd like:
> 
>  #ifdef _DIRENT_HAVE_D_RECLEN
>  		*total += dirent->d_reclen;
>  #else
> -		*total += dirent->d_namlen + sizeof(*dirent);
> +		*total += strlen(dirent->d_name) + sizeof(*dirent);
>  #endif
> 
> but musl *does* have d_reclen:
> 
> http://git.musl-libc.org/cgit/musl/tree/include/dirent.h
> 
> so if it simply advertised that fact we'd be good...

Why not test for d_reclen in configure instead?

> 
> Whatever happened to that request from January above?
> 
> That said, I suppose strlen is fine to use here, so I guess I don't
> see a strong reason to object to this change.
> 

Guessing uClibc would benefit as well.

So how does testing for d_reclen and making the slow-path more portable
by using strlen sound for a plan?

> >> 2) crc32selftest doesn't make sense in case of cross-compilation,
> >> the same can be said if building for a different host with the
> >> native toolchain. So maybe an --disable-crc32selftest configure
> >> option could be used here.
> >>
> >> 3) The CFLAGS passed to BUILD_CC weren't understood by the native
> >> compiler. Set BUILD_CFLAGS to CFLAGS if not cross-compiling or let
> >> it blank anyway?  
> > 
> > Ah, yes, we've definitely been through this before. Last time I said
> > "send patches" and they never appeared.

Well this is a case of easy to write the code but hard to figure out
what the upstream preferred way of handling it is. I'd just put it
behind a test target. Distribution developers are conditioned to run
make test.

> > 
> > As it is, I'm really hesitant to remove the crc32c sanity check,
> > mainly because we've just been through a XFS corruption drill where
> > one in 10^6 crc calculations would end up wrong because of a bug in
> > a compiler.
> > 
> > Can we "auto detect" cross compilation in the makefile and avoid the
> > check only in that situation? configure options are really only a
> > last resort...  
> 
> That sounds like the right path to me.

So for 2) the plan is disable if cross-compiling, run unconditionally
otherwise and no way to overwrite at configure time.

As for 3) is there a point in having BUILD_CFLAGS introduced?

> 
> Or cross-compile it but don't run it, and let package managers run
> it at install time?  ;)

Of course I can implement it but I doubt we would see many packagers go
that length. :)

Cheers
Ralph


> 
> >> 4) sys/ustat.h is missing in musl.  
> > 
> > Yup, been reported before. I'm about to commit a fix for that,
> > mainly because certain new arches don't even implement the ustat
> > system call, and glibc completely fucks up the handling of that.
> > i.e. programs that call ustat don't fail at build time, the syscall
> > is stubbed to print "not supported, will fail" at runtime. Hence
> > nobody noticed that the xfsprogs code calling ustat was actually
> > broken until a week ago...  
> 
> Partly because we only checked for success, not failure, but nobody
> rationally expected -ENOSYS any more than they expected the
> Spanish Inquisition...
> 
> -Eric
> 
> > Cheers,
> > 
> > Dave.
> >   

Re: [BUG] xfsprogs-4.7.0: issues cross-compiling for musl

[Eric Sandeen]

On 9/8/16 3:41 AM, Ralph Sennhauser wrote:
> Hi Eric
> 
> On Wed, 7 Sep 2016 17:40:01 -0500
> Eric Sandeen <sandeen@sandeen.net> wrote:
> 
>> On 9/7/16 5:27 PM, Dave Chinner wrote:
>>> On Wed, Sep 07, 2016 at 11:42:56AM +0200, Ralph Sennhauser wrote:  
>>>> To get xfsprogs to cross-compile for musl some hacks were needed.
>>>>
>>>> 1) d_namlen isn't available with musl  
>>>
>>> musl problem. It needs to define _DIRENT_HAVE_D_RECLEN and friends
>>> to tell applications what the struct dirent it defines contains.
>>>
>>> (Haven't we been through this before?)  
>>
>> (yes: http://www.openwall.com/lists/musl/2016/01/15/9)
>>
>> what *does* it contain?
>>
>> We do already have:
>>
>>  #ifdef _DIRENT_HAVE_D_RECLEN
>>  		*total += dirent->d_reclen;
>>  #else
>> 		*total += dirent->d_namlen + sizeof(*dirent);
>>  #endif
>>
>> but you'd like:
>>
>>  #ifdef _DIRENT_HAVE_D_RECLEN
>>  		*total += dirent->d_reclen;
>>  #else
>> -		*total += dirent->d_namlen + sizeof(*dirent);
>> +		*total += strlen(dirent->d_name) + sizeof(*dirent);
>>  #endif
>>
>> but musl *does* have d_reclen:
>>
>> http://git.musl-libc.org/cgit/musl/tree/include/dirent.h
>>
>> so if it simply advertised that fact we'd be good...
> 
> Why not test for d_reclen in configure instead?

Why not advertise it in headers, as the manpage tells us to expect?

In the end Dave is the final arbiter here.  I don't really care,
but what I would not want to see is #ifdef hell resulting from
several different ways to check.  If it can be hidden in configure,
it doesn't bother me.

>>
>> Whatever happened to that request from January above?

Nobody knows? ;)

>>>> 2) crc32selftest doesn't make sense in case of cross-compilation,
>>>> the same can be said if building for a different host with the
>>>> native toolchain. So maybe an --disable-crc32selftest configure
>>>> option could be used here.

...

> So for 2) the plan is disable if cross-compiling, run unconditionally
> otherwise and no way to overwrite at configure time.

Seems reasonable.  I agree that it should be automatic, not a config-time
switch.  We know if we're cross compiling, so DTRT...

-Eric
 

Re: [BUG] xfsprogs-4.7.0: issues cross-compiling for musl

[Ralph Sennhauser]

On Thu, 8 Sep 2016 07:13:40 -0500
Eric Sandeen <sandeen@sandeen.net> wrote:

> On 9/8/16 3:41 AM, Ralph Sennhauser wrote:
> > Hi Eric
> > 
> > On Wed, 7 Sep 2016 17:40:01 -0500
> > Eric Sandeen <sandeen@sandeen.net> wrote:
> >   
> >> On 9/7/16 5:27 PM, Dave Chinner wrote:  
> >>> On Wed, Sep 07, 2016 at 11:42:56AM +0200, Ralph Sennhauser
> >>> wrote:    
> >>>> To get xfsprogs to cross-compile for musl some hacks were needed.
> >>>>
> >>>> 1) d_namlen isn't available with musl    
> >>>
> >>> musl problem. It needs to define _DIRENT_HAVE_D_RECLEN and friends
> >>> to tell applications what the struct dirent it defines contains.
> >>>
> >>> (Haven't we been through this before?)    
> >>
> >> (yes: http://www.openwall.com/lists/musl/2016/01/15/9)
> >>
> >> what *does* it contain?
> >>
> >> We do already have:
> >>
> >>  #ifdef _DIRENT_HAVE_D_RECLEN
> >>  		*total += dirent->d_reclen;
> >>  #else
> >> 		*total += dirent->d_namlen + sizeof(*dirent);
> >>  #endif
> >>
> >> but you'd like:
> >>
> >>  #ifdef _DIRENT_HAVE_D_RECLEN
> >>  		*total += dirent->d_reclen;
> >>  #else
> >> -		*total += dirent->d_namlen + sizeof(*dirent);
> >> +		*total += strlen(dirent->d_name) +
> >> sizeof(*dirent); #endif
> >>
> >> but musl *does* have d_reclen:
> >>
> >> http://git.musl-libc.org/cgit/musl/tree/include/dirent.h
> >>
> >> so if it simply advertised that fact we'd be good...  
> > 
> > Why not test for d_reclen in configure instead?  
> 
> Why not advertise it in headers, as the manpage tells us to expect?
> 
> In the end Dave is the final arbiter here.  I don't really care,
> but what I would not want to see is #ifdef hell resulting from
> several different ways to check.  If it can be hidden in configure,
> it doesn't bother me.
> 

It can be hidden from the sources. If musl advertises d_reclen the code
is fixed for musl. We still might want the slow-path to be more POSIX
conformant for other libcs.

> >>
> >> Whatever happened to that request from January above?  
> 
> Nobody knows? ;)
> 

It's not in master :)

> >>>> 2) crc32selftest doesn't make sense in case of cross-compilation,
> >>>> the same can be said if building for a different host with the
> >>>> native toolchain. So maybe an --disable-crc32selftest configure
> >>>> option could be used here.  
> 
> ...
> 
> > So for 2) the plan is disable if cross-compiling, run
> > unconditionally otherwise and no way to overwrite at configure
> > time.  
> 
> Seems reasonable.  I agree that it should be automatic, not a
> config-time switch.  We know if we're cross compiling, so DTRT...
> 
> -Eric
>  

Even with a configure option default would be enabled unless
cross-compiling, that would be my take.

Of course "make test" wasn't ruled out either. Guess that is still open
as well.

Cross-compiling crc32selftest instead of compiling native is probably
more meaningful as well. So that remark earlier certainly has some
merit.

Cheers
Ralph

[PATCH] xfs_io: fix building with musl

[Ralph Sennhauser]

The fallback in case the libc doesn't have or doesn't advertise the
existence of d_reclen in struct dirent uses d_namlen. Musl neither
advertises d_reclen nor does it have a d_namlen member.

Calculate the value for d_namlen from d_name in the fallback path.

Signed-off-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
---

With ustat.h issue resolved in 4.8.0-rc1 this is the only musl related
one left.

An alternative could be some autoconf magic [1] or waiting for a long
time for musl to add _DIRENT_HAVE_D_RECLEN [2] and for it to propagate
to distributions.

[1] http://oss.sgi.com/archives/xfs/2016-01/msg00388.html
[2] http://www.openwall.com/lists/musl/2016/01/15/9

---
 io/readdir.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/io/readdir.c b/io/readdir.c
index 151b72e..2b56dc8 100644
--- a/io/readdir.c
+++ b/io/readdir.c
@@ -24,6 +24,10 @@
 #include <sys/types.h>
 #include <dirent.h>
 
+#ifndef _DIRENT_HAVE_D_RECLEN
+#include <string.h>
+#endif
+
 static struct cmdinfo readdir_cmd;
 
 const char *d_type_str(unsigned int type)
@@ -106,7 +110,7 @@ read_directory(
 #ifdef _DIRENT_HAVE_D_RECLEN
 		*total += dirent->d_reclen;
 #else
-		*total += dirent->d_namlen + sizeof(*dirent);
+		*total += strlen(dirent->d_name) + sizeof(*dirent);
 #endif
 		count++;
 
-- 
2.7.3

Re: [PATCH] xfs_io: fix building with musl

[Eric Sandeen]

On 9/9/16 8:32 AM, Ralph Sennhauser wrote:
> The fallback in case the libc doesn't have or doesn't advertise the
> existence of d_reclen in struct dirent uses d_namlen. Musl neither
> advertises d_reclen nor does it have a d_namlen member.
> 
> Calculate the value for d_namlen from d_name in the fallback path.
> 
> Signed-off-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> ---
> 
> With ustat.h issue resolved in 4.8.0-rc1 this is the only musl related
> one left.
> 
> An alternative could be some autoconf magic [1] or waiting for a long
> time for musl to add _DIRENT_HAVE_D_RECLEN [2] and for it to propagate
> to distributions.
> 
> [1] http://oss.sgi.com/archives/xfs/2016-01/msg00388.html
> [2] http://www.openwall.com/lists/musl/2016/01/15/9

eh, this is fine by me.  Even Linus has hated d_namlen for over
a decade ;)  http://lkml.iu.edu/hypermail/linux/kernel/9506/0033.html

Last question, have you tested it beyond the build, i.e.
with xfs_io -c "readdir -v" /some/dir ?  Works here but it'd be nice
to know that it's working properly on musl as well.

the d_reclen field doesn't exist in any xfstests output, so looks
like its loss on some platforms won't affect any existing tests.

If you can verify that you did a runtime test as well, that'd
be great, but in any case:

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

Thanks,
-Eric

> ---
>  io/readdir.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/io/readdir.c b/io/readdir.c
> index 151b72e..2b56dc8 100644
> --- a/io/readdir.c
> +++ b/io/readdir.c
> @@ -24,6 +24,10 @@
>  #include <sys/types.h>
>  #include <dirent.h>
>  
> +#ifndef _DIRENT_HAVE_D_RECLEN
> +#include <string.h>
> +#endif
> +
>  static struct cmdinfo readdir_cmd;
>  
>  const char *d_type_str(unsigned int type)
> @@ -106,7 +110,7 @@ read_directory(
>  #ifdef _DIRENT_HAVE_D_RECLEN
>  		*total += dirent->d_reclen;
>  #else
> -		*total += dirent->d_namlen + sizeof(*dirent);
> +		*total += strlen(dirent->d_name) + sizeof(*dirent);
>  #endif
>  		count++;
>  
> 

Re: [PATCH] xfs_io: fix building with musl

[Ralph Sennhauser]

On Fri, 9 Sep 2016 08:59:16 -0500
Eric Sandeen <sandeen@sandeen.net> wrote:

> On 9/9/16 8:32 AM, Ralph Sennhauser wrote:
> > The fallback in case the libc doesn't have or doesn't advertise the
> > existence of d_reclen in struct dirent uses d_namlen. Musl neither
> > advertises d_reclen nor does it have a d_namlen member.
> > 
> > Calculate the value for d_namlen from d_name in the fallback path.
> > 
> > Signed-off-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> > ---
> > 
> > With ustat.h issue resolved in 4.8.0-rc1 this is the only musl
> > related one left.
> > 
> > An alternative could be some autoconf magic [1] or waiting for a
> > long time for musl to add _DIRENT_HAVE_D_RECLEN [2] and for it to
> > propagate to distributions.
> > 
> > [1] http://oss.sgi.com/archives/xfs/2016-01/msg00388.html
> > [2] http://www.openwall.com/lists/musl/2016/01/15/9  
> 
> eh, this is fine by me.  Even Linus has hated d_namlen for over
> a decade ;)  http://lkml.iu.edu/hypermail/linux/kernel/9506/0033.html
> 
> Last question, have you tested it beyond the build, i.e.
> with xfs_io -c "readdir -v" /some/dir ?  Works here but it'd be nice
> to know that it's working properly on musl as well.
> 

Kernel is 4.8-rc5, xfsprogs 4-8.0-rc1 plus fixups mentioned. An xfs
filesystem is mounted at /mnt/xfs.

# xfs_io -c "readdir -v" /mnt/xfs/test/
00000000: d_ino: 0x00000063 d_name: .
00000000: d_ino: 0x00000060 d_name: ..
00000000: d_ino: 0x00000064 d_name: file
read 847 bytes from offset 0
847.000000 bytes, 3 ops, 0.0000 sec (20 MiB/sec and 75000.0000 ops/sec)

> the d_reclen field doesn't exist in any xfstests output, so looks
> like its loss on some platforms won't affect any existing tests.
> 
> If you can verify that you did a runtime test as well, that'd
> be great, but in any case:
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> 

Thanks for the review
Ralph

> Thanks,
> -Eric
> 
> > ---
> >  io/readdir.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/io/readdir.c b/io/readdir.c
> > index 151b72e..2b56dc8 100644
> > --- a/io/readdir.c
> > +++ b/io/readdir.c
> > @@ -24,6 +24,10 @@
> >  #include <sys/types.h>
> >  #include <dirent.h>
> >  
> > +#ifndef _DIRENT_HAVE_D_RECLEN
> > +#include <string.h>
> > +#endif
> > +
> >  static struct cmdinfo readdir_cmd;
> >  
> >  const char *d_type_str(unsigned int type)
> > @@ -106,7 +110,7 @@ read_directory(
> >  #ifdef _DIRENT_HAVE_D_RECLEN
> >  		*total += dirent->d_reclen;
> >  #else
> > -		*total += dirent->d_namlen + sizeof(*dirent);
> > +		*total += strlen(dirent->d_name) + sizeof(*dirent);
> >  #endif
> >  		count++;
> >  
> >   

[RFC] libxfs: cross-compile fixes

[Ralph Sennhauser]

---

Based on the earlier discussion this seems to be the preferred way of
handling things.

Should I split it into 3 patches?
Do you want something different to happen when cross-compiling?

Cheers
Ralph

---
 configure.ac         |  3 +++
 include/builddefs.in |  1 +
 libxfs/Makefile      | 10 ++++++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 79053e5..89334d6 100644
--- a/configure.ac
+++ b/configure.ac
@@ -10,11 +10,14 @@ AC_PROG_LIBTOOL
 
 AC_PROG_CC
 if test $cross_compiling = no; then
+  CROSS_COMPILING=no
   BUILD_CC="$CC"
   AC_SUBST(BUILD_CC)
 else
+  CROSS_COMPILING=yes
   AC_CHECK_PROGS(BUILD_CC, gcc cc)
 fi
+AC_SUBST(CROSS_COMPILING)
 
 AC_ARG_ENABLE(shared,
 [ --enable-shared=[yes/no] Enable use of shared libraries [default=yes]],,
diff --git a/include/builddefs.in b/include/builddefs.in
index 7153d7a..0c4ce36 100644
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -26,6 +26,7 @@ MALLOCLIB = @malloc_lib@
 LOADERFLAGS = @LDFLAGS@
 LTLDFLAGS = @LDFLAGS@
 CFLAGS = @CFLAGS@
+CROSS_COMPILING = @CROSS_COMPILING@
 
 LIBRT = @librt@
 LIBUUID = @libuuid@
diff --git a/libxfs/Makefile b/libxfs/Makefile
index 62608bd..ad492e5 100644
--- a/libxfs/Makefile
+++ b/libxfs/Makefile
@@ -114,11 +114,15 @@ DEBUG = -DNDEBUG
 
 LDIRT = gen_crc32table crc32table.h crc32selftest
 
+ifeq ($(CROSS_COMPILING),no)
+BUILD_CFLAGS:=$(CFLAGS)
+endif
+
 default: crc32selftest ltdepend $(LTLIBRARY)
 
 crc32table.h: gen_crc32table.c
 	@echo "    [CC]     gen_crc32table"
-	$(Q) $(BUILD_CC) $(CFLAGS) -o gen_crc32table $<
+	$(Q) $(BUILD_CC) $(BUILD_CFLAGS) -o gen_crc32table $<
 	@echo "    [GENERATE] $@"
 	$(Q) ./gen_crc32table > crc32table.h
 
@@ -128,9 +132,11 @@ crc32table.h: gen_crc32table.c
 # busted CRC calculation at build time and hence avoid putting bad CRCs down on
 # disk.
 crc32selftest: gen_crc32table.c crc32table.h crc32.c
+ifeq ($(CROSS_COMPILING),no)
 	@echo "    [TEST]    CRC32"
-	$(Q) $(BUILD_CC) $(CFLAGS) -D CRC32_SELFTEST=1 crc32.c -o $@
+	$(Q) $(CC) $(CFLAGS) -D CRC32_SELFTEST=1 crc32.c -o $@
 	$(Q) ./$@
+endif
 
 # set up include/xfs header directory
 include $(BUILDRULES)
-- 
2.7.3

Re: [RFC] libxfs: cross-compile fixes

[Dave Chinner]

On Sat, Sep 10, 2016 at 09:37:31AM +0200, Ralph Sennhauser wrote:
> ---
> 
> Based on the earlier discussion this seems to be the preferred way of
> handling things.
> 
> Should I split it into 3 patches?
> Do you want something different to happen when cross-compiling?
> 
> Cheers
> Ralph
> 
> ---
>  configure.ac         |  3 +++
>  include/builddefs.in |  1 +
>  libxfs/Makefile      | 10 ++++++++--
>  3 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/libxfs/Makefile b/libxfs/Makefile
> index 62608bd..ad492e5 100644
> --- a/libxfs/Makefile
> +++ b/libxfs/Makefile
> @@ -114,11 +114,15 @@ DEBUG = -DNDEBUG
>  
>  LDIRT = gen_crc32table crc32table.h crc32selftest
>  
> +ifeq ($(CROSS_COMPILING),no)
> +BUILD_CFLAGS:=$(CFLAGS)
> +endif

This seems like the wrong place to define something like this. We
define CFLAGS in include/builddefs.in, and I think BUILD_CFLAGS
should be defined there, too. It should probably only drop the
platform specific flags, rather than everything in the corss
compiling case.

> @@ -128,9 +132,11 @@ crc32table.h: gen_crc32table.c
>  # busted CRC calculation at build time and hence avoid putting bad CRCs down on
>  # disk.
>  crc32selftest: gen_crc32table.c crc32table.h crc32.c
> +ifeq ($(CROSS_COMPILING),no)
>  	@echo "    [TEST]    CRC32"
> -	$(Q) $(BUILD_CC) $(CFLAGS) -D CRC32_SELFTEST=1 crc32.c -o $@
> +	$(Q) $(CC) $(CFLAGS) -D CRC32_SELFTEST=1 crc32.c -o $@
>  	$(Q) ./$@
> +endif

That looks wrong. If CROSS_COMPILING == no, then BUILD_CC has
already been set to CC by the configure script, and CFLAGS is just
fine to use here.

I think what needs to be done here is that the crc32selftest /rule/
needs to be conditional on CROSS_COMPILING, not the /execution/
of the rule. If nothing is dependent on crc32selftest, then it won't
get executed. i.e. this requires dependency manipulation, not
execution changes.

CRC_SRC = crc32table.h crc32.c
ifeq ($(CROSS_COMPILING),no)
	TARGETS = crc32selftest
else
	TARGETS = $CRC_SRC
endif

default: $TARGETS ltdepend $(LTLIBRARY)

.....
crc32selftest: gen_crc32table.c $CRC_SRC
....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [RFC] libxfs: cross-compile fixes

[Ralph Sennhauser]

On Mon, 19 Sep 2016 15:50:18 +1000
Dave Chinner <david@fromorbit.com> wrote:

> On Sat, Sep 10, 2016 at 09:37:31AM +0200, Ralph Sennhauser wrote:
> > ---
> > 
> > Based on the earlier discussion this seems to be the preferred way
> > of handling things.
> > 
> > Should I split it into 3 patches?
> > Do you want something different to happen when cross-compiling?
> > 
> > Cheers
> > Ralph
> > 
> > ---
> >  configure.ac         |  3 +++
> >  include/builddefs.in |  1 +
> >  libxfs/Makefile      | 10 ++++++++--
> >  3 files changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libxfs/Makefile b/libxfs/Makefile
> > index 62608bd..ad492e5 100644
> > --- a/libxfs/Makefile
> > +++ b/libxfs/Makefile
> > @@ -114,11 +114,15 @@ DEBUG = -DNDEBUG
> >  
> >  LDIRT = gen_crc32table crc32table.h crc32selftest
> >  
> > +ifeq ($(CROSS_COMPILING),no)
> > +BUILD_CFLAGS:=$(CFLAGS)
> > +endif  
> 
> This seems like the wrong place to define something like this. We
> define CFLAGS in include/builddefs.in, and I think BUILD_CFLAGS
> should be defined there, too. It should probably only drop the
> platform specific flags, rather than everything in the corss
> compiling case.

Filtering CFLAGS I don't see as practical. On the other hand
constructing meaningful default BUILD_CFLAGS in case they would be used
elsewhere later can be done. I'll add it to builddefs as well.

> 
> > @@ -128,9 +132,11 @@ crc32table.h: gen_crc32table.c
> >  # busted CRC calculation at build time and hence avoid putting bad
> > CRCs down on # disk.
> >  crc32selftest: gen_crc32table.c crc32table.h crc32.c
> > +ifeq ($(CROSS_COMPILING),no)
> >  	@echo "    [TEST]    CRC32"
> > -	$(Q) $(BUILD_CC) $(CFLAGS) -D CRC32_SELFTEST=1 crc32.c -o
> > $@
> > +	$(Q) $(CC) $(CFLAGS) -D CRC32_SELFTEST=1 crc32.c -o $@
> >  	$(Q) ./$@
> > +endif  
> 
> That looks wrong. If CROSS_COMPILING == no, then BUILD_CC has
> already been set to CC by the configure script, and CFLAGS is just
> fine to use here.
> 

I switched to CC to "document" that we are never cross-compiling
crc32selftest. Of course BUILD_CC would be set to the same value here
and could be used instead. CFLAGS are untouched and used as is.

> I think what needs to be done here is that the crc32selftest /rule/
> needs to be conditional on CROSS_COMPILING, not the /execution/
> of the rule. If nothing is dependent on crc32selftest, then it won't
> get executed. i.e. this requires dependency manipulation, not
> execution changes.
> 

Sure can do.

Thanks
Ralph

> CRC_SRC = crc32table.h crc32.c
> ifeq ($(CROSS_COMPILING),no)
> 	TARGETS = crc32selftest
> else
> 	TARGETS = $CRC_SRC
> endif
> 
> default: $TARGETS ltdepend $(LTLIBRARY)
> 
> .....
> crc32selftest: gen_crc32table.c $CRC_SRC
> ....
> 
> Cheers,
> 
> Dave.

Re: [RFC] libxfs: cross-compile fixes

[Eric Sandeen]

On 9/10/16 2:37 AM, Ralph Sennhauser wrote:
> ---
> 
> Based on the earlier discussion this seems to be the preferred way of
> handling things.
> 
> Should I split it into 3 patches?
> Do you want something different to happen when cross-compiling?

Hi Ralph -

I just merged a patch to for-next to address cross-compiling;
it was on the list prior to this one.

Can you confirm that it resolves your issues?

Thanks,
-Eric
 
> Cheers
> Ralph
> 
> ---
>  configure.ac         |  3 +++
>  include/builddefs.in |  1 +
>  libxfs/Makefile      | 10 ++++++++--
>  3 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 79053e5..89334d6 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -10,11 +10,14 @@ AC_PROG_LIBTOOL
>  
>  AC_PROG_CC
>  if test $cross_compiling = no; then
> +  CROSS_COMPILING=no
>    BUILD_CC="$CC"
>    AC_SUBST(BUILD_CC)
>  else
> +  CROSS_COMPILING=yes
>    AC_CHECK_PROGS(BUILD_CC, gcc cc)
>  fi
> +AC_SUBST(CROSS_COMPILING)
>  
>  AC_ARG_ENABLE(shared,
>  [ --enable-shared=[yes/no] Enable use of shared libraries [default=yes]],,
> diff --git a/include/builddefs.in b/include/builddefs.in
> index 7153d7a..0c4ce36 100644
> --- a/include/builddefs.in
> +++ b/include/builddefs.in
> @@ -26,6 +26,7 @@ MALLOCLIB = @malloc_lib@
>  LOADERFLAGS = @LDFLAGS@
>  LTLDFLAGS = @LDFLAGS@
>  CFLAGS = @CFLAGS@
> +CROSS_COMPILING = @CROSS_COMPILING@
>  
>  LIBRT = @librt@
>  LIBUUID = @libuuid@
> diff --git a/libxfs/Makefile b/libxfs/Makefile
> index 62608bd..ad492e5 100644
> --- a/libxfs/Makefile
> +++ b/libxfs/Makefile
> @@ -114,11 +114,15 @@ DEBUG = -DNDEBUG
>  
>  LDIRT = gen_crc32table crc32table.h crc32selftest
>  
> +ifeq ($(CROSS_COMPILING),no)
> +BUILD_CFLAGS:=$(CFLAGS)
> +endif
> +
>  default: crc32selftest ltdepend $(LTLIBRARY)
>  
>  crc32table.h: gen_crc32table.c
>  	@echo "    [CC]     gen_crc32table"
> -	$(Q) $(BUILD_CC) $(CFLAGS) -o gen_crc32table $<
> +	$(Q) $(BUILD_CC) $(BUILD_CFLAGS) -o gen_crc32table $<
>  	@echo "    [GENERATE] $@"
>  	$(Q) ./gen_crc32table > crc32table.h
>  
> @@ -128,9 +132,11 @@ crc32table.h: gen_crc32table.c
>  # busted CRC calculation at build time and hence avoid putting bad CRCs down on
>  # disk.
>  crc32selftest: gen_crc32table.c crc32table.h crc32.c
> +ifeq ($(CROSS_COMPILING),no)
>  	@echo "    [TEST]    CRC32"
> -	$(Q) $(BUILD_CC) $(CFLAGS) -D CRC32_SELFTEST=1 crc32.c -o $@
> +	$(Q) $(CC) $(CFLAGS) -D CRC32_SELFTEST=1 crc32.c -o $@
>  	$(Q) ./$@
> +endif
>  
>  # set up include/xfs header directory
>  include $(BUILDRULES)
> 

Re: [RFC] libxfs: cross-compile fixes

[Ralph Sennhauser]

On Sun, 15 Jan 2017 14:32:00 -0600
Eric Sandeen <sandeen@sandeen.net> wrote:

> On 9/10/16 2:37 AM, Ralph Sennhauser wrote:
> > ---
> > 
> > Based on the earlier discussion this seems to be the preferred way
> > of handling things.
> > 
> > Should I split it into 3 patches?
> > Do you want something different to happen when cross-compiling?  
> 
> Hi Ralph -
> 
> I just merged a patch to for-next to address cross-compiling;
> it was on the list prior to this one.
> 
> Can you confirm that it resolves your issues?

Hi Eric,

I compile tested 4.9.0 with 0a71e38396304b4d1215ba0b51cd6ce8e33eb40d on
top. Cross-compiling is fixed for me with this leaving only the d_namlen
issue with musl for which you already reviewed a patch from me. If that
gets addressed I can use vanilla xfsprogs.

Thanks
Ralph

> 
> Thanks,
> -Eric
>  
> > Cheers
> > Ralph
> > 
> > ---
> >  configure.ac         |  3 +++
> >  include/builddefs.in |  1 +
> >  libxfs/Makefile      | 10 ++++++++--
> >  3 files changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/configure.ac b/configure.ac
> > index 79053e5..89334d6 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -10,11 +10,14 @@ AC_PROG_LIBTOOL
> >  
> >  AC_PROG_CC
> >  if test $cross_compiling = no; then
> > +  CROSS_COMPILING=no
> >    BUILD_CC="$CC"
> >    AC_SUBST(BUILD_CC)
> >  else
> > +  CROSS_COMPILING=yes
> >    AC_CHECK_PROGS(BUILD_CC, gcc cc)
> >  fi
> > +AC_SUBST(CROSS_COMPILING)
> >  
> >  AC_ARG_ENABLE(shared,
> >  [ --enable-shared=[yes/no] Enable use of shared libraries
> > [default=yes]],, diff --git a/include/builddefs.in
> > b/include/builddefs.in index 7153d7a..0c4ce36 100644
> > --- a/include/builddefs.in
> > +++ b/include/builddefs.in
> > @@ -26,6 +26,7 @@ MALLOCLIB = @malloc_lib@
> >  LOADERFLAGS = @LDFLAGS@
> >  LTLDFLAGS = @LDFLAGS@
> >  CFLAGS = @CFLAGS@
> > +CROSS_COMPILING = @CROSS_COMPILING@
> >  
> >  LIBRT = @librt@
> >  LIBUUID = @libuuid@
> > diff --git a/libxfs/Makefile b/libxfs/Makefile
> > index 62608bd..ad492e5 100644
> > --- a/libxfs/Makefile
> > +++ b/libxfs/Makefile
> > @@ -114,11 +114,15 @@ DEBUG = -DNDEBUG
> >  
> >  LDIRT = gen_crc32table crc32table.h crc32selftest
> >  
> > +ifeq ($(CROSS_COMPILING),no)
> > +BUILD_CFLAGS:=$(CFLAGS)
> > +endif
> > +
> >  default: crc32selftest ltdepend $(LTLIBRARY)
> >  
> >  crc32table.h: gen_crc32table.c
> >  	@echo "    [CC]     gen_crc32table"
> > -	$(Q) $(BUILD_CC) $(CFLAGS) -o gen_crc32table $<
> > +	$(Q) $(BUILD_CC) $(BUILD_CFLAGS) -o gen_crc32table $<
> >  	@echo "    [GENERATE] $@"
> >  	$(Q) ./gen_crc32table > crc32table.h
> >  
> > @@ -128,9 +132,11 @@ crc32table.h: gen_crc32table.c
> >  # busted CRC calculation at build time and hence avoid putting bad
> > CRCs down on # disk.
> >  crc32selftest: gen_crc32table.c crc32table.h crc32.c
> > +ifeq ($(CROSS_COMPILING),no)
> >  	@echo "    [TEST]    CRC32"
> > -	$(Q) $(BUILD_CC) $(CFLAGS) -D CRC32_SELFTEST=1 crc32.c -o
> > $@
> > +	$(Q) $(CC) $(CFLAGS) -D CRC32_SELFTEST=1 crc32.c -o $@
> >  	$(Q) ./$@
> > +endif
> >  
> >  # set up include/xfs header directory
> >  include $(BUILDRULES)
> >   

Re: [RFC] libxfs: cross-compile fixes

[Eric Sandeen]

On 1/16/17 7:45 AM, Ralph Sennhauser wrote:
> On Sun, 15 Jan 2017 14:32:00 -0600
> Eric Sandeen <sandeen@sandeen.net> wrote:
> 
>> On 9/10/16 2:37 AM, Ralph Sennhauser wrote:
>>> ---
>>>
>>> Based on the earlier discussion this seems to be the preferred way
>>> of handling things.
>>>
>>> Should I split it into 3 patches?
>>> Do you want something different to happen when cross-compiling?  
>>
>> Hi Ralph -
>>
>> I just merged a patch to for-next to address cross-compiling;
>> it was on the list prior to this one.
>>
>> Can you confirm that it resolves your issues?
> 
> Hi Eric,
> 
> I compile tested 4.9.0 with 0a71e38396304b4d1215ba0b51cd6ce8e33eb40d on
> top. Cross-compiling is fixed for me with this leaving only the d_namlen
> issue with musl for which you already reviewed a patch from me. If that
> gets addressed I can use vanilla xfsprogs.

Hi Ralph, that should have been merged as well.  Check out the for-next
branch of the git tree.

Thanks,
-Eric

Re: [RFC] libxfs: cross-compile fixes

[Ralph Sennhauser]

On Mon, 16 Jan 2017 08:42:46 -0600
Eric Sandeen <sandeen@sandeen.net> wrote:

> On 1/16/17 7:45 AM, Ralph Sennhauser wrote:
> > On Sun, 15 Jan 2017 14:32:00 -0600
> > Eric Sandeen <sandeen@sandeen.net> wrote:
> >   
> >> On 9/10/16 2:37 AM, Ralph Sennhauser wrote:  
> >>> ---
> >>>
> >>> Based on the earlier discussion this seems to be the preferred way
> >>> of handling things.
> >>>
> >>> Should I split it into 3 patches?
> >>> Do you want something different to happen when
> >>> cross-compiling?    
> >>
> >> Hi Ralph -
> >>
> >> I just merged a patch to for-next to address cross-compiling;
> >> it was on the list prior to this one.
> >>
> >> Can you confirm that it resolves your issues?  
> > 
> > Hi Eric,
> > 
> > I compile tested 4.9.0 with
> > 0a71e38396304b4d1215ba0b51cd6ce8e33eb40d on top. Cross-compiling is
> > fixed for me with this leaving only the d_namlen issue with musl
> > for which you already reviewed a patch from me. If that gets
> > addressed I can use vanilla xfsprogs.  
> 
> Hi Ralph, that should have been merged as well.  Check out the
> for-next branch of the git tree.

How did I miss that ... :)

Thanks
Ralph

> 
> Thanks,
> -Eric