xfs
[Top] [All Lists]

Re: [PATCH] xfsprogs: replace umode_t with xfs_mode_t

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfsprogs: replace umode_t with xfs_mode_t
From: "Michael L. Semon" <mlsemon35@xxxxxxxxx>
Date: Fri, 14 Jun 2013 14:25:44 -0400
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type:content-transfer-encoding; bh=DM9F+QxXoLYnttHMIByNXjE87SxOAJg9HJqJlTz6Z5A=; b=A7lx3NjYFw4Ht+Amcr2IvhSUmFrLNF4l8CQlB0cH3PXLZcdIkoDh/iNB0fr3V6LIV/ x+GpoFCyeUmUno2CPYMlW8j/UfhdyKVhDq6I6XtnMkbHkQ7fptWuM513pbqtWKO4i6Wx odAeacscFc3n8cbyUObWrcw7mnm52AjfeBZq9qWYicajOnqhAzBwsrkcYmaMdUV91WO/ Uqw1VDPOOEXNQ0CtYglplfay9DoHe/11vS1AIBGON09x3ln+poi91AURTuYfrqR+S4Pg 31gsbuwyWW0DjORVcrr9h9sX1ZGjSDJb2eP2P1s8t8X8FwQaA4VFpKkbSHlSXvmo1h7b xPwQ==
In-reply-to: <20130614023758.GT29338@dastard>
References: <51BA219B.4070503@xxxxxxxxx> <20130614023758.GT29338@dastard>
User-agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20130509 Thunderbird/17.0.6
On 06/13/2013 10:37 PM, Dave Chinner wrote:
> On Thu, Jun 13, 2013 at 03:46:35PM -0400, Michael L. Semon wrote:
>> This patch was made after applying the most recent 3.11-related 
>> patchset for xfsprogs.  It was applied against the crc-dev branch, 
>> in order to get CRC-enabled xfsprogs to build without complaining 
>> about a lack of "umode_t".  The systems used were both 32-bit 
>> slackware-current systems, against public kernel headers of kernel 
>> 3.8.x and 3.9.x vintage.
>>
>> This is an "idea" patch, a way of getting CRC-enabled xfsprogs to 
>> build on kernels later than 3.2 while staying within the conventions 
>> of my test systems.  The idea may be bad, and the XFS crew might be 
>> solving it in a different way while moving header files and functions.
>>
>> Anyway, I don't know how the Debian and Red Hat crews solve header-
>> related issues like these, so comments and flames are welcome.  In 
>> particular, I'm open to a better name than "xfs_mode_t" that does 
>> not match the grep term "umode_t".
> 
> The usual way to do this is with autoconf magic.
> 
> The thing is, we want to keep the user and kernel code as close as
> possible, so if the kernel code uses umode_t, then we need to use
> that internally in the userspace code as well.

Yeah, that's why I originally chose to get by with a one-line sysadmin-
hack typedef.  An extra postprocessing step means one more thing to go 
wrong.

If you would, please put the question, "There needs to be one more 
check, but what?" in your mind as you read this...

<asm/types.h> was used in the test, but I don't really know where 
umode_t was picked up in the past.  That's just an educated guess.

> So what we need is an autoconf rule that detects is umode_t is
> defined by the distro, and if it isn't define it ourselves.
> 
> See, for example, m4/package_types.m4 for how to detect if a
> variable is already defined. Add a new rule to detect umode_t,
> and use it to set a variable called have_umode_t. i.e.
> 
> .....
>       ], [
>               umode_t umode;
>       ], have_umode_t=no
>               AC_MSG_RESULT(yes),
>               AC_MSG_RESULT(no))
>       AC_SUBST(have_umode_t)
>       ])
> 
> then add a line like:
> 
> HAVE_UMODE_T = @have_umode_t@

I couldn't work the have_umode_t=no and AC_SUBST(have_umode_t) in 
there, so I did a copy-and-paste of one of the other checks in 
m4/package_types.m4.  [Patch is submitted at the end of this letter.]

> to include/builddefs.in, as well as add this to the platform compile
> flags like so:
> 
> ifeq ($(HAVE_UMODE_T),yes)
> PCFLAGS += -DHAVE_UMODE_T
> endif

I used this.
 
> then in include/platform_defs.h.in add something like:
> 
> #ifndef HAVE_UMODE_T
> typedef unsigned short umode_t;
> #endif

I used this, too.  My end result was that a `make install-qa` exports 
this to platform_defs.h, which is fine and solves your purposes in 
particular.  In testing, it's exported exactly that way.  Should the 
system have a conflicting definition for umode_t, the xfsdump 
configure (I think) fails with a "could not compile requisite headers" 
message.  [I had typed "typedef unsigned int umode_t;" into 
/usr/include/asm/types.h by accident and found that.]

> And then you shouldn't have to modify any of the actual libxfs code
> at all...

OK.  This was tested against the 2.6.39 headers provided by the 
Slackware 13 kernel-headers package, and xfsdump was rebuilt.  I think 
that Samba 3 uses xfs/libxfs.h in some way, and I didn't have time 
to recompile Samba to see what would happen.  If it makes a difference,
slackware-current uses `make install-qa` in its xfsprogs-3.1.11 build...

ftp://ftp.slackware.com/pub/slackware/slackware-current/source/a/xfsprogs/xfsprogs.SlackBuild

...so any exports from `make install-qa` will make it there to cause 
potential confusion later.  [And I thank Pat for making it that way.]
 
> If you need help, just ping me ;)

Thanks!

Michael (patch follows)

>From 011384927727b8ad5a6f555027b4853b077abf2f Mon Sep 17 00:00:00 2001
From: "Michael L. Semon" <mlsemon35@xxxxxxxxx>
Date: Fri, 14 Jun 2013 03:00:34 -0400
Subject: [PATCH] xfsprogs: define umode_t for build if not defined already

umode_t has not been exported to the kernel private headers since
around kernel 3.2.0-rc7.  Add a check for umode_t and define it
if it has not been defined already.

Signed-off-by: Michael L. Semon <mlsemon35@xxxxxxxxx>
---
 configure.ac               |  1 +
 include/builddefs.in       |  3 +++
 include/platform_defs.h.in |  5 +++++
 m4/package_types.m4        | 11 +++++++++++
 4 files changed, 20 insertions(+)

diff --git a/configure.ac b/configure.ac
index b968977..e5fd94e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -118,6 +118,7 @@ AC_CHECK_SIZEOF([char *])
 AC_TYPE_PSINT
 AC_TYPE_PSUNSIGNED
 AC_TYPE_U32
+AC_TYPE_UMODE_T
 AC_MANUAL_FORMAT
 
 AC_CONFIG_FILES([include/builddefs])
diff --git a/include/builddefs.in b/include/builddefs.in
index 0a3fa1a..744e8d3 100644
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -109,6 +109,9 @@ GCCFLAGS = -funsigned-char -fno-strict-aliasing -Wall
 
 ifeq ($(PKG_PLATFORM),linux)
 PCFLAGS = -D_GNU_SOURCE -D_BSD_SOURCE -D_XOPEN_SOURCE=500 
-D_FILE_OFFSET_BITS=64 $(GCCFLAGS)
+ifeq ($(HAVE_UMODE_T),yes)
+PCFLAGS += -DHAVE_UMODE_T
+endif
 DEPENDFLAGS = -D__linux__
 endif
 ifeq ($(PKG_PLATFORM),gnukfreebsd)
diff --git a/include/platform_defs.h.in b/include/platform_defs.h.in
index 03391f5..ac260bc 100644
--- a/include/platform_defs.h.in
+++ b/include/platform_defs.h.in
@@ -123,6 +123,11 @@ typedef unsigned long long __psunsigned_t;
 # endif
 #endif
 
+/* Check whether to define umode_t ourselves. */
+#ifndef HAVE_UMODE_T
+typedef unsigned short umode_t;
+#endif
+
 /* Define if you want gettext (I18N) support */
 #undef ENABLE_GETTEXT
 #ifdef ENABLE_GETTEXT
diff --git a/m4/package_types.m4 b/m4/package_types.m4
index dfcb0d9..50ce48f 100644
--- a/m4/package_types.m4
+++ b/m4/package_types.m4
@@ -39,3 +39,14 @@ AC_DEFUN([AC_TYPE_U32],
          __u32  u32;
     ], AC_DEFINE(HAVE___U32) AC_MSG_RESULT(yes) , AC_MSG_RESULT(no))
   ])
+# 
+# Check if we have umode_t
+# 
+AC_DEFUN([AC_TYPE_UMODE_T],
+  [ AC_MSG_CHECKING([for umode_t])
+    AC_TRY_COMPILE([
+#include <asm/types.h>
+    ], [
+         umode_t umode;
+    ], AC_DEFINE(HAVE_UMODE_T) AC_MSG_RESULT(yes) , AC_MSG_RESULT(no))
+  ])
-- 
1.8.2


<Prev in Thread] Current Thread [Next in Thread>