Bug 1372939

Summary: xfs/300 fails with xfsprogs-4.5.0-6.el7; not built against libattr
Product: Red Hat Enterprise Linux 7 Reporter: Eric Sandeen <esandeen>
Component: xfsprogsAssignee: Eric Sandeen <esandeen>
Status: CLOSED ERRATA QA Contact: Zorro Lang <zlang>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: 7.3CC: eguan, fweimer, jbastian, jcm, tlavigne, xzhou, yselkowi, zlang
Target Milestone: rcKeywords: Patch, Regression
Target Release: ---   
Hardware: All   
OS: Unspecified   
Whiteboard:
Fixed In Version: xfsprogs-4.5.0-7.el7 Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2016-11-04 06:24:48 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Attachments:
Description Flags
Patch for rhel-7.3 none

Description Eric Sandeen 2016-09-04 02:01:30 UTC
xfstest xfs/300 tests xfs_fsr

With the stock build:

# rpm -qif `which xfs_fsr`
Name        : xfsprogs
Version     : 4.5.0
Release     : 6.el7
Architecture: aarch64
Install Date: Sat 03 Sep 2016 09:57:43 PM EDT
Group       : System Environment/Base
Size        : 4402142
License     : GPL+ and LGPLv2+
Signature   : RSA/SHA256, Wed 10 Aug 2016 11:11:30 AM EDT, Key ID 938a80caf21541eb
Source RPM  : xfsprogs-4.5.0-6.el7.src.rpm
Build Date  : Tue 09 Aug 2016 02:21:35 PM EDT
Build Host  : arm64-017.build.eng.bos.redhat.com
Relocations : (not relocatable)
Packager    : Red Hat, Inc. <http://bugzilla.redhat.com/bugzilla>
Vendor      : Red Hat, Inc.
URL         : http://oss.sgi.com/projects/xfs/
Summary     : Utilities for managing the XFS filesystem

It fails:

# ./check xfs/300
...
xfs/300 1s ... - output mismatch (see /root/sandeen/xfstests-dev/results//xfs/300.out.bad)
    --- tests/xfs/300.out	2016-09-03 21:49:47.529941074 -0400
    +++ /root/sandeen/xfstests-dev/results//xfs/300.out.bad	2016-09-03 21:58:45.696283777 -0400
    @@ -1,3 +1,3 @@
     QA output created by 300
     SCRATCH_MNT/300.test
    -extents before:7 after:1 DONE SCRATCH_MNT/300.test
    +XFS_IOC_SWAPEXT failed: SCRATCH_MNT/300.test: Invalid argument
    ...
    (Run 'diff -u tests/xfs/300.out /root/sandeen/xfstests-dev/results//xfs/300.out.bad'  to see the entire diff)
Ran: xfs/300
Failures: xfs/300
Failed 1 of 1 tests

If I rebuild with:
# rpm -q gcc
gcc-4.8.5-9.el7.aarch64

# rpm -e xfsprogs
# wget http://download-node-02.eng.bos.redhat.com/brewroot/packages/xfsprogs/4.5.0/6.el7/src/xfsprogs-4.5.0-6.el7.src.rpm
# rpmbuild --rebuild xfsprogs-4.5.0-6.el7.src.rpm 
# rpm -ivh /root/rpmbuild/RPMS/aarch64/xfsprogs-4.5.0-6.el7.aarch64.rpm

#./check xfs/300
xfs/300 1s ... 1s
Ran: xfs/300
Passed all 1 tests

Comment 2 Zorro Lang 2016-09-04 10:22:26 UTC
Thanks for find this, Eric. We can't find this problem due to another known issue.

When auto test with beaker, xfs/300 get below output:
---
QA output created by 300
---

When I run xfs/300 manually, I got below output:
---
QA output created by 300
SCRATCH_MNT/300.test
XFS_IOC_SWAPEXT failed: SCRATCH_MNT/300.test: Invalid argument
---

Both of them are unexpected.

Thanks,
Zorro

Comment 3 Eric Sandeen 2016-09-06 18:20:22 UTC
I may need to dig into this more; I did a scratch build of xfsprogs:

https://brewweb.engineering.redhat.com/brew/taskinfo?taskID=11707699

which I think used the new gcc, and that build still fails.

Comment 4 Eric Sandeen 2016-09-06 18:43:59 UTC
This fails on all arches because we somehow lost a dependency on libattr, and fsr now #ifdefs away calls if configure didn't find the attr headers.

commit c14c7b795c0bdb92d925bab44e8239867b2883c6
Author: Jan Tulak <jtulak>
Date:   Wed Oct 14 10:58:03 2015 +1100

    build: Add autoconf check for fsetxattr call

This is a regression, and needs to be fixed for all arches.

Comment 5 Yaakov Selkowitz 2016-09-06 19:04:25 UTC
Link to the commit:

http://oss.sgi.com/cgi-bin/gitweb.cgi?p=xfs/cmds/xfsprogs.git;a=commitdiff;h=c14c7b795c0bdb92d925bab44e8239867b2883c6;hp=7f7fd8ac1f5cc0d3af7c2f050ac070a321581de5

Supposed rationale:

> OS X has fsetxattr() in another header and with different arguments.
> For now, check for the Linux variant and if not available, skip
> the code using the call.

So the configure test looks for attr/xattr.h (from libattr-devel) to determine if it's on Linux, but then uses sys/xattr.h in the sources.  Honestly, this looks bogus to me; why not just use #ifndef __APPLE__ around that code?

Looks like we need BuildRequires: libattr-devel for attr/xattr.h in the configure test.  Nothing appears to link with libattr, so glibc's implementation should still be used.

Comment 6 Eric Sandeen 2016-09-06 19:09:27 UTC
(In reply to Yaakov Selkowitz from comment #5)

Yep, not a big fan of that change - I hadn't looked at the two different headers, ugh.

> Looks like we need BuildRequires: libattr-devel for attr/xattr.h in the
> configure test.  Nothing appears to link with libattr, so glibc's
> implementation should still be used.

Yes, that's the simplest/fastest fix for now I think.

I'll look harder at the commit and why we're looking at 2 different headers, thanks!

-Eric

Comment 7 Eric Sandeen 2016-09-06 19:19:22 UTC
This does probably fix it as well, w/o the new dependency:

diff --git a/m4/package_libcdev.m4 b/m4/package_libcdev.m4
index 7a847e9..55946ab 100644
--- a/m4/package_libcdev.m4
+++ b/m4/package_libcdev.m4
@@ -239,7 +239,7 @@ AC_DEFUN([AC_HAVE_FSETXATTR],
        have_fsetxattr=yes,
        [],
        [#include <sys/types.h>
-        #include <attr/xattr.h>]
+        #include <sys/xattr.h>]
        )
     AC_SUBST(have_fsetxattr)
   ])

Comment 8 Yaakov Selkowitz 2016-09-06 19:20:28 UTC
(In reply to Eric Sandeen from comment #6)
> (In reply to Yaakov Selkowitz from comment #5)
> 
> Yep, not a big fan of that change - I hadn't looked at the two different
> headers, ugh.
> 
> Yes, that's the simplest/fastest fix for now I think.

Scratch build in progress.

> I'll look harder at the commit and why we're looking at 2 different headers,
> thanks!

IIUC, both Darwin[1] and Linux have <sys/xattr.h> but libattr is not available on both and it provides <attr/xattr.h>, so someone thought this was a "smart" way of detecting Linux vs. Darwin fsetxattr.

IMHO the entire commit is incorrect; it should really be reverted, and the #ifdef HAVE_FSETXATTR block conditional on #ifndef __APPLE__ instead.  Alternatively, a more complicated configure test can be written to determine how many arguments fsetxattr accepts, but I'm not sure if any other systems have the extra argument.

Comment 9 Yaakov Selkowitz 2016-09-06 19:21:28 UTC
(In reply to Eric Sandeen from comment #7)
> This does probably fix it as well, w/o the new dependency:
> 
>         [#include <sys/types.h>
> -        #include <attr/xattr.h>]
> +        #include <sys/xattr.h>]

Darwin has <sys/xattr.h> too, hence that wouldn't help.

Comment 10 Yaakov Selkowitz 2016-09-06 19:25:03 UTC
Created attachment 1198407 [details]
Patch for rhel-7.3

Successful scratch build:

https://brewweb.engineering.redhat.com/brew/taskinfo?taskID=11709316

Most notably, in build.log:

checking whether fsetxattr is declared... yes

and libattr.so.1 does NOT show up as a dependency.

Comment 14 errata-xmlrpc 2016-11-04 06:24:48 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHBA-2016-2431.html