xfsprogs: replace umode_t with xfs_mode_t

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

Thanks!

Michael

From: "Michael L. Semon" <[email protected]>
Date: Thu, 13 Jun 2013 03:08:30 -0400
Subject: [PATCH] xfsprogs: replace umode_t with xfs_mode_t

umode_t has not been exported to the public kernel headers by the kernel make headers_install process since about 3.2.0-rc7. For that matter, a grep -rl umode_t /usr/include can return nothing, as if authors are avoiding the term "umode_t" altogether. Follow this convention by changing umode_t to xfs_mode_t.

The simple hack way through this problem is to do this instead:

typedef unsigned short umode_t;

However, when xfsprogs is installed by make install-qa, this can be exported to /usr/include, which is not desired behavior.

---
 include/xfs_ialloc.h  | 8 +++++++-
 include/xfs_symlink.h | 3 ++-
 libxfs/xfs_ialloc.c   | 4 ++--
 3 files changed, 11 insertions(+), 4 deletions(-)

index 68c0732..2cc3e93 100644
@@ -25,6 +25,12 @@ struct xfs_mount;
 struct xfs_trans;
 
 /*
+ * Provide a replacement for a variable that has not been exported 
+ * to public kernel headers since about kernel 3.2.0-rc7.
+ */
+typedef unsigned short xfs_mode_t;
+
+/*
  * Allocation parameters for inode allocation.
  */
 #define	XFS_IALLOC_INODES(mp)	(mp)->m_ialloc_inos

@@ -72,7 +78,7 @@ int					/* error */
 xfs_dialloc(
 	struct xfs_trans *tp,		/* transaction pointer */
 	xfs_ino_t	parent,		/* parent inode (directory) */
-	umode_t		mode,		/* mode bits for new inode */
+	xfs_mode_t	mode,		/* mode bits for new inode */
 	int		okalloc,	/* ok to allocate more space */
 	struct xfs_buf	**agbp,		/* buf for a.g. inode header */
 	xfs_ino_t	*inop);		/* inode number allocated */

index 4818edf..93801ec 100644
@@ -59,7 +59,8 @@ extern const struct xfs_buf_ops xfs_symlink_buf_ops;
 #ifdef __KERNEL__
 
 int xfs_symlink(struct xfs_inode *dp, struct xfs_name *link_name,
-		const char *target_path, umode_t mode, struct xfs_inode **ipp);
+		const char *target_path, xfs_mode_t mode, 
+		struct xfs_inode **ipp);
 int xfs_readlink(struct xfs_inode *ip, char *link);
 int xfs_inactive_symlink_rmt(struct xfs_inode *ip, struct xfs_trans **tpp);
 

index ce32dfa..2ce06c8 100644
@@ -490,7 +490,7 @@ STATIC xfs_agnumber_t
 xfs_ialloc_ag_select(
 	xfs_trans_t	*tp,		/* transaction pointer */
 	xfs_ino_t	parent,		/* parent directory inode number */
-	umode_t		mode,		/* bits set to indicate file type */
+	xfs_mode_t	mode,		/* bits set to indicate file type */
 	int		okalloc)	/* ok to allocate more space */
 {
 	xfs_agnumber_t	agcount;	/* number of ag's in the filesystem */

@@ -936,7 +936,7 @@ int
 xfs_dialloc(
 	struct xfs_trans	*tp,
 	xfs_ino_t		parent,
-	umode_t			mode,
+	xfs_mode_t		mode,
 	int			okalloc,
 	struct xfs_buf		**IO_agbp,
 	xfs_ino_t		*inop)

1.8.2

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

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.

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@

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

then in include/platform_defs.h.in add something like:

#ifndef HAVE_UMODE_T
typedef unsigned short umode_t;
#endif

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

If you need help, just ping me ;)

Cheers,

Dave.

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

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.

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

I used this.

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

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

Thanks!

Michael (patch follows)

From: "Michael L. Semon" <[email protected]>
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.

---
 configure.ac               |  1 +
 include/builddefs.in       |  3 +++
 include/platform_defs.h.in |  5 +++++
 m4/package_types.m4        | 11 +++++++++++
 4 files changed, 20 insertions(+)

index b968977..e5fd94e 100644
@@ -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])

index 0a3fa1a..744e8d3 100644
@@ -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)

index 03391f5..ac260bc 100644
@@ -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

index dfcb0d9..50ce48f 100644
@@ -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


_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Looks ok to me.

Applied..

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs