review: Simple patch to remove the dmapi support from xfsdump

review: Simple patch to remove the dmapi support from xfsdump

[Russell Cattelan]

[-- Attachment #1: Type: text/plain, Size: 415 bytes --]

Since very few people run xfs with dmapi enabled it seem kind of silly
to force the install of the dmapi utils/libraries just for xfs dump/restore.

This patch stubs out the Hsm functions and adjusts the autoconf
to not fail in the case of missing dmapi headers/libs.

If somebody does trie to do a dump with the -a option a ERROR message
is issued, the dump does continue? this might not be the desired behavior.


[-- Attachment #2: no_dmapi --]
[-- Type: text/plain, Size: 6530 bytes --]

Index: xfs-cmds/xfsdump/aclocal.m4
===================================================================
--- xfs-cmds.orig/xfsdump/aclocal.m4	2006-08-02 14:51:34.000000000 -0500
+++ xfs-cmds/xfsdump/aclocal.m4	2006-08-02 14:58:11.250605250 -0500
@@ -161,27 +161,31 @@ AC_DEFUN([AC_PACKAGE_NEED_XFS_DMAPI_H],
   [ AC_CHECK_HEADERS([xfs/dmapi.h])
     if test "$ac_cv_header_xfs_dmapi_h" != yes; then
         echo
-        echo 'FATAL ERROR: could not find a valid DMAPI library header.'
-        echo 'Install the data migration API (dmapi) development package.'
+        echo 'WARNING: could not find a valid DMAPI library header.'
+        echo 'xfsdump / xfsrestore will not support dmapi file systems.'
         echo 'Alternatively, run "make install-dev" from the dmapi source.'
-        exit 1
     fi
   ])
 
 AC_DEFUN([AC_PACKAGE_NEED_MAKEHANDLE_LIBDM],
-  [ AC_CHECK_LIB(dm, dm_make_handle,, [
+  [ AC_CHECK_LIB(dm, dm_make_handle,
+	[have_dmapi=true
+ 	libdm="-ldm"
+	    test -f `pwd`/../dmapi/libdm/libdm.la && \
+	        libdm="`pwd`/../dmapi/libdm/libdm.la"
+	    test -f ${libexecdir}${libdirsuffix}/libdm.la && \
+		libdm="${libexecdir}${libdirsuffix}/libdm.la"
+	],
+	[
         echo
-        echo 'FATAL ERROR: could not find a valid DMAPI base library.'
-        echo 'Install the data migration API (dmapi) library package.'
+        echo 'WARNING: could not find a valid DMAPI base library.'
+        echo 'xfsdump / xfsrestore will not support dmapi file systems.'
         echo 'Alternatively, run "make install" from the dmapi source.'
-        exit 1
+	have_dmapi=false
+	libdm = ""
     ])
-    libdm="-ldm"
-    test -f `pwd`/../dmapi/libdm/libdm.la && \
-        libdm="`pwd`/../dmapi/libdm/libdm.la"
-    test -f ${libexecdir}${libdirsuffix}/libdm.la && \
-	libdm="${libexecdir}${libdirsuffix}/libdm.la"
     AC_SUBST(libdm)
+    AC_SUBST(have_dmapi)
   ])
 
 # 
Index: xfs-cmds/xfsdump/common/hsmapi_noop.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ xfs-cmds/xfsdump/common/hsmapi_noop.c	2006-08-02 15:38:39.046333000 -0500
@@ -0,0 +1,147 @@
+/*
+ * Copyright (c) 2000-2004 Silicon Graphics, Inc.
+ * All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+
+#include <xfs/xfs.h>
+#include <attr/attributes.h>
+#include <xfs/handle.h>
+
+#include "hsmapi.h"
+#include "mlog.h"
+#include "exit.h"
+
+typedef	struct	{
+} dmf_fs_ctxt_t;
+
+extern hsm_fs_ctxt_t *
+HsmInitFsysContext(
+const	char		*mountpoint,
+	int		dumpversion)
+{
+	mlog( MLOG_NORMAL | MLOG_ERROR, _(
+		      "-%c Dumping of DMF dualstate file not supported in this version\n"));
+	return NULL;
+}
+
+extern void
+HsmDeleteFsysContext(
+	hsm_fs_ctxt_t	*contextp)
+{
+}
+
+extern int
+HsmEstimateFileSpace(
+	hsm_fs_ctxt_t	*contextp,
+const	xfs_bstat_t	*statp,
+	off64_t		*bytes)
+{
+	return 0;
+}
+
+extern int
+HsmEstimateFileOffset(
+	hsm_fs_ctxt_t	*contextp,
+const	xfs_bstat_t	*statp,
+	off64_t		bytecount,
+	off64_t		*byteoffset)
+{
+	return 0;
+}
+
+extern hsm_f_ctxt_t *
+HsmAllocateFileContext(
+	hsm_fs_ctxt_t	*contextp)
+{
+	return NULL;
+}
+
+extern void
+HsmDeleteFileContext(
+	hsm_f_ctxt_t	*contextp)
+{
+}
+
+extern int
+HsmInitFileContext(
+	hsm_f_ctxt_t	*contextp,
+const	xfs_bstat_t	*statp)
+{
+	return 0;
+}
+
+extern int
+HsmModifyInode(
+	hsm_f_ctxt_t	*contextp,
+	xfs_bstat_t	*statp)
+{
+	return 0;
+}
+
+extern int
+HsmModifyExtentMap(
+	hsm_f_ctxt_t	*contextp,
+	struct getbmapx	*bmap)
+{
+	return 0;
+}
+
+extern int
+HsmFilterExistingAttribute(
+	hsm_f_ctxt_t	*hsm_f_ctxtp,
+const	char		*namep,		/* attribute name */
+	u_int32_t	valuesz,	/* value size */
+	int		flag,
+	int		*skip_entry)
+{
+	return 0;
+}
+
+extern int
+HsmAddNewAttribute(
+	hsm_f_ctxt_t	*hsm_f_ctxtp,
+	int		cursor,
+	int		flag,
+	char		**namepp,	/* pointer to new attribute name */
+	char		**valuepp,	/* pointer to its value */
+	u_int32_t	*valueszp)	/* pointer to the value size */
+{
+	return 0;
+}
+
+extern void
+HsmBeginRestoreFile(
+	bstat_t		*bstatp,
+	int		fd,
+	int		*hsm_flagp)
+{
+}
+
+extern void
+HsmRestoreAttribute(
+	int		flag,		/* ext attr flags */
+	char		*namep,		/* pointer to new attribute name */
+	int		*hsm_flagp)
+{
+}
+
+extern void
+HsmEndRestoreFile(
+	char		*path,
+	int		fd,
+	int		*hsm_flagp)
+{
+}
Index: xfs-cmds/xfsdump/dump/Makefile
===================================================================
--- xfs-cmds.orig/xfsdump/dump/Makefile	2006-08-02 14:51:34.000000000 -0500
+++ xfs-cmds/xfsdump/dump/Makefile	2006-08-02 14:52:15.076345750 -0500
@@ -57,7 +57,6 @@ COMMON = \
 	fs.c \
 	getdents.c \
 	global.c \
-	hsmapi.c \
 	lock.c \
 	main.c \
 	mlog.c \
@@ -69,6 +68,12 @@ COMMON = \
 	util.c \
 	sproc.c
 
+ifeq ($(HAVE_DMAPI), true)
+  COMMON += hsmapi.c
+else
+  COMMON += hsmapi_noop.c
+endif
+
 LOCALS = \
 	content.c \
 	inomap.c \
Index: xfs-cmds/xfsdump/include/builddefs.in
===================================================================
--- xfs-cmds.orig/xfsdump/include/builddefs.in	2006-08-02 14:51:34.000000000 -0500
+++ xfs-cmds/xfsdump/include/builddefs.in	2006-08-02 14:52:15.080346000 -0500
@@ -66,6 +66,8 @@ ENABLE_GETTEXT	= @enable_gettext@
 
 HAVE_ZIPPED_MANPAGES = @have_zipped_manpages@
 
+HAVE_DMAPI = @have_dmapi@
+
 LCFLAGS += -DXFS_BIG_FILES=1 -DXFS_BIG_FILESYSTEMS=1
 
 ifeq ($(PKG_PLATFORM),linux)
Index: xfs-cmds/xfsdump/restore/Makefile
===================================================================
--- xfs-cmds.orig/xfsdump/restore/Makefile	2006-08-02 14:51:34.000000000 -0500
+++ xfs-cmds/xfsdump/restore/Makefile	2006-08-02 14:52:15.080346000 -0500
@@ -55,7 +55,6 @@ COMMON = \
 	fs.c \
         getdents.c \
 	global.c \
-	hsmapi.c \
 	lock.c \
 	main.c \
 	mlog.c \
@@ -67,6 +66,12 @@ COMMON = \
 	stream.c \
 	util.c
 
+ifeq ($(HAVE_DMAPI), true)
+  COMMON += hsmapi.c
+else
+  COMMON += hsmapi_noop.c
+endif
+
 LOCALS = \
 	bag.c \
 	content.c \

Re: review: Simple patch to remove the dmapi support from xfsdump

[Vlad Apostolov]

Hi Russel,

I don't understand in details the build changes but they seam to be fine.
In summary if the build system can find the DMAPI lib installed it will
use hsmapi_noop.c otherwise hsmapi.c.
The return code of HsmInitFileContext() stub probably should be non zero.

It is looking good.

Regards,
Vlad

Russell Cattelan wrote:
> Since very few people run xfs with dmapi enabled it seem kind of silly
> to force the install of the dmapi utils/libraries just for xfs 
> dump/restore.
>
> This patch stubs out the Hsm functions and adjusts the autoconf
> to not fail in the case of missing dmapi headers/libs.
>
> If somebody does trie to do a dump with the -a option a ERROR message
> is issued, the dump does continue? this might not be the desired 
> behavior.
>
> ------------------------------------------------------------------------
>
> Index: xfs-cmds/xfsdump/aclocal.m4
> ===================================================================
> --- xfs-cmds.orig/xfsdump/aclocal.m4	2006-08-02 14:51:34.000000000 -0500
> +++ xfs-cmds/xfsdump/aclocal.m4	2006-08-02 14:58:11.250605250 -0500
> @@ -161,27 +161,31 @@ AC_DEFUN([AC_PACKAGE_NEED_XFS_DMAPI_H],
>    [ AC_CHECK_HEADERS([xfs/dmapi.h])
>      if test "$ac_cv_header_xfs_dmapi_h" != yes; then
>          echo
> -        echo 'FATAL ERROR: could not find a valid DMAPI library header.'
> -        echo 'Install the data migration API (dmapi) development package.'
> +        echo 'WARNING: could not find a valid DMAPI library header.'
> +        echo 'xfsdump / xfsrestore will not support dmapi file systems.'
>          echo 'Alternatively, run "make install-dev" from the dmapi source.'
> -        exit 1
>      fi
>    ])
>  
>  AC_DEFUN([AC_PACKAGE_NEED_MAKEHANDLE_LIBDM],
> -  [ AC_CHECK_LIB(dm, dm_make_handle,, [
> +  [ AC_CHECK_LIB(dm, dm_make_handle,
> +	[have_dmapi=true
> + 	libdm="-ldm"
> +	    test -f `pwd`/../dmapi/libdm/libdm.la && \
> +	        libdm="`pwd`/../dmapi/libdm/libdm.la"
> +	    test -f ${libexecdir}${libdirsuffix}/libdm.la && \
> +		libdm="${libexecdir}${libdirsuffix}/libdm.la"
> +	],
> +	[
>          echo
> -        echo 'FATAL ERROR: could not find a valid DMAPI base library.'
> -        echo 'Install the data migration API (dmapi) library package.'
> +        echo 'WARNING: could not find a valid DMAPI base library.'
> +        echo 'xfsdump / xfsrestore will not support dmapi file systems.'
>          echo 'Alternatively, run "make install" from the dmapi source.'
> -        exit 1
> +	have_dmapi=false
> +	libdm = ""
>      ])
> -    libdm="-ldm"
> -    test -f `pwd`/../dmapi/libdm/libdm.la && \
> -        libdm="`pwd`/../dmapi/libdm/libdm.la"
> -    test -f ${libexecdir}${libdirsuffix}/libdm.la && \
> -	libdm="${libexecdir}${libdirsuffix}/libdm.la"
>      AC_SUBST(libdm)
> +    AC_SUBST(have_dmapi)
>    ])
>  
>  # 
> Index: xfs-cmds/xfsdump/common/hsmapi_noop.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ xfs-cmds/xfsdump/common/hsmapi_noop.c	2006-08-02 15:38:39.046333000 -0500
> @@ -0,0 +1,147 @@
> +/*
> + * Copyright (c) 2000-2004 Silicon Graphics, Inc.
> + * All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it would be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write the Free Software Foundation,
> + * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + */
> +
> +#include <xfs/xfs.h>
> +#include <attr/attributes.h>
> +#include <xfs/handle.h>
> +
> +#include "hsmapi.h"
> +#include "mlog.h"
> +#include "exit.h"
> +
> +typedef	struct	{
> +} dmf_fs_ctxt_t;
> +
> +extern hsm_fs_ctxt_t *
> +HsmInitFsysContext(
> +const	char		*mountpoint,
> +	int		dumpversion)
> +{
> +	mlog( MLOG_NORMAL | MLOG_ERROR, _(
> +		      "-%c Dumping of DMF dualstate file not supported in this version\n"));
> +	return NULL;
> +}
> +
> +extern void
> +HsmDeleteFsysContext(
> +	hsm_fs_ctxt_t	*contextp)
> +{
> +}
> +
> +extern int
> +HsmEstimateFileSpace(
> +	hsm_fs_ctxt_t	*contextp,
> +const	xfs_bstat_t	*statp,
> +	off64_t		*bytes)
> +{
> +	return 0;
> +}
> +
> +extern int
> +HsmEstimateFileOffset(
> +	hsm_fs_ctxt_t	*contextp,
> +const	xfs_bstat_t	*statp,
> +	off64_t		bytecount,
> +	off64_t		*byteoffset)
> +{
> +	return 0;
> +}
> +
> +extern hsm_f_ctxt_t *
> +HsmAllocateFileContext(
> +	hsm_fs_ctxt_t	*contextp)
> +{
> +	return NULL;
> +}
> +
> +extern void
> +HsmDeleteFileContext(
> +	hsm_f_ctxt_t	*contextp)
> +{
> +}
> +
> +extern int
> +HsmInitFileContext(
> +	hsm_f_ctxt_t	*contextp,
> +const	xfs_bstat_t	*statp)
> +{
> +	return 0;
> +}
> +
> +extern int
> +HsmModifyInode(
> +	hsm_f_ctxt_t	*contextp,
> +	xfs_bstat_t	*statp)
> +{
> +	return 0;
> +}
> +
> +extern int
> +HsmModifyExtentMap(
> +	hsm_f_ctxt_t	*contextp,
> +	struct getbmapx	*bmap)
> +{
> +	return 0;
> +}
> +
> +extern int
> +HsmFilterExistingAttribute(
> +	hsm_f_ctxt_t	*hsm_f_ctxtp,
> +const	char		*namep,		/* attribute name */
> +	u_int32_t	valuesz,	/* value size */
> +	int		flag,
> +	int		*skip_entry)
> +{
> +	return 0;
> +}
> +
> +extern int
> +HsmAddNewAttribute(
> +	hsm_f_ctxt_t	*hsm_f_ctxtp,
> +	int		cursor,
> +	int		flag,
> +	char		**namepp,	/* pointer to new attribute name */
> +	char		**valuepp,	/* pointer to its value */
> +	u_int32_t	*valueszp)	/* pointer to the value size */
> +{
> +	return 0;
> +}
> +
> +extern void
> +HsmBeginRestoreFile(
> +	bstat_t		*bstatp,
> +	int		fd,
> +	int		*hsm_flagp)
> +{
> +}
> +
> +extern void
> +HsmRestoreAttribute(
> +	int		flag,		/* ext attr flags */
> +	char		*namep,		/* pointer to new attribute name */
> +	int		*hsm_flagp)
> +{
> +}
> +
> +extern void
> +HsmEndRestoreFile(
> +	char		*path,
> +	int		fd,
> +	int		*hsm_flagp)
> +{
> +}
> Index: xfs-cmds/xfsdump/dump/Makefile
> ===================================================================
> --- xfs-cmds.orig/xfsdump/dump/Makefile	2006-08-02 14:51:34.000000000 -0500
> +++ xfs-cmds/xfsdump/dump/Makefile	2006-08-02 14:52:15.076345750 -0500
> @@ -57,7 +57,6 @@ COMMON = \
>  	fs.c \
>  	getdents.c \
>  	global.c \
> -	hsmapi.c \
>  	lock.c \
>  	main.c \
>  	mlog.c \
> @@ -69,6 +68,12 @@ COMMON = \
>  	util.c \
>  	sproc.c
>  
> +ifeq ($(HAVE_DMAPI), true)
> +  COMMON += hsmapi.c
> +else
> +  COMMON += hsmapi_noop.c
> +endif
> +
>  LOCALS = \
>  	content.c \
>  	inomap.c \
> Index: xfs-cmds/xfsdump/include/builddefs.in
> ===================================================================
> --- xfs-cmds.orig/xfsdump/include/builddefs.in	2006-08-02 14:51:34.000000000 -0500
> +++ xfs-cmds/xfsdump/include/builddefs.in	2006-08-02 14:52:15.080346000 -0500
> @@ -66,6 +66,8 @@ ENABLE_GETTEXT	= @enable_gettext@
>  
>  HAVE_ZIPPED_MANPAGES = @have_zipped_manpages@
>  
> +HAVE_DMAPI = @have_dmapi@
> +
>  LCFLAGS += -DXFS_BIG_FILES=1 -DXFS_BIG_FILESYSTEMS=1
>  
>  ifeq ($(PKG_PLATFORM),linux)
> Index: xfs-cmds/xfsdump/restore/Makefile
> ===================================================================
> --- xfs-cmds.orig/xfsdump/restore/Makefile	2006-08-02 14:51:34.000000000 -0500
> +++ xfs-cmds/xfsdump/restore/Makefile	2006-08-02 14:52:15.080346000 -0500
> @@ -55,7 +55,6 @@ COMMON = \
>  	fs.c \
>          getdents.c \
>  	global.c \
> -	hsmapi.c \
>  	lock.c \
>  	main.c \
>  	mlog.c \
> @@ -67,6 +66,12 @@ COMMON = \
>  	stream.c \
>  	util.c
>  
> +ifeq ($(HAVE_DMAPI), true)
> +  COMMON += hsmapi.c
> +else
> +  COMMON += hsmapi_noop.c
> +endif
> +
>  LOCALS = \
>  	bag.c \
>  	content.c \
>   

Re: review: Simple patch to remove the dmapi support from xfsdump

[Dean Roehrich]

On Fri, Aug 04, 2006 at 02:18:13PM +1000, Vlad Apostolov wrote:
> Hi Russel,
> 
> I don't understand in details the build changes but they seam to be fine.

Vlad, why do they seem fine?

> In summary if the build system can find the DMAPI lib installed it will
> use hsmapi_noop.c otherwise hsmapi.c.
> The return code of HsmInitFileContext() stub probably should be non zero.
> 
> It is looking good.

So this is determined at build time...so when you're using a version of
xfsdump/xfsrestore how do you know you're _not_ using one that is DMAPI-aware
_before_ you get into trouble with an invalid dump?

Assuming that issue is addressed, here's another:  The libdm is a shared
object, so why not take advantage of that and load it with dlopen?  Then the
issue is determined at runtime rather than build time.  This is easy, and even
DMF does it this way.

Dean

Re: review: Simple patch to remove the dmapi support from xfsdump

[Russell Cattelan]

Dean Roehrich wrote:
> On Fri, Aug 04, 2006 at 02:18:13PM +1000, Vlad Apostolov wrote:
>   
>> Hi Russel,
>>
>> I don't understand in details the build changes but they seam to be fine.
>>     
>
> Vlad, why do they seem fine?
>
>   
>> In summary if the build system can find the DMAPI lib installed it will
>> use hsmapi_noop.c otherwise hsmapi.c.
>> The return code of HsmInitFileContext() stub probably should be non zero.
>>
>> It is looking good.
>>     
>
> So this is determined at build time...so when you're using a version of
> xfsdump/xfsrestore how do you know you're _not_ using one that is DMAPI-aware
> _before_ you get into trouble with an invalid dump?
>   
I appears as if the Hsm routines are only used if the -a flag is given 
to xfsdump.
(dump DMF dualstate files as offline)
 
 From what I can tell the only users that will need the HSM routines 
would be Suse
based installs or somebody doing a custom kernel with the xfs-tree from oss.
Anybody running with a kernel.org kernel would have no need for  dmapi 
support in xfs dump/restore.
(This includes the Fedora kernel with currently does does build the 
xfsdump package)
Under the current scheme in order to get xfsdump at all you need to 
build and install the dmapi and dmapi-devel
packages, which seemed kinda silly to me on system that don't have the 
kernel support for dmapi.
So basically I just stubbed out the Hsm functions is the case of no 
dmapi libraries

The dlopen is a good idea but still requires that build machine has the 
dmapi libraries installed
which seems like extra work if they are never going to be used.

> Assuming that issue is addressed, here's another:  The libdm is a shared
> object, so why not take advantage of that and load it with dlopen?  Then the
> issue is determined at runtime rather than build time.  This is easy, and even
> DMF does it this way.
>
> Dean
>
>   

Re: review: Simple patch to remove the dmapi support from xfsdump

[Dean Roehrich]

On Fri, Aug 04, 2006 at 10:36:37AM -0500, Russell Cattelan wrote:

> I appears as if the Hsm routines are only used if the -a flag is given 
> to xfsdump.
> (dump DMF dualstate files as offline)

If use of -a causes xfsdump to fail then that may be sufficient.

Also, use of -D should cause xfsrestore to fail.

That might cover my concerns.


Dean

Re: review: Simple patch to remove the dmapi support from xfsdump

[Bill Kendall]

Dean Roehrich wrote:
> On Fri, Aug 04, 2006 at 10:36:37AM -0500, Russell Cattelan wrote:
> 
> 
>>I appears as if the Hsm routines are only used if the -a flag is given 
>>to xfsdump.
>>(dump DMF dualstate files as offline)
> 
> 
> If use of -a causes xfsdump to fail then that may be sufficient.

Then what? Each user (admittedly, this is a small number) has to rebuild
xfsdump locally, and then do so every time their package manager grabs
an updated xfsdump? That sounds like more of an inconvenience than
installing a couple packages on a build machine, especially considering
the xfsdump and libdm source are bundled together with all the other xfs
user space tools. (And isn't there logic in the xfs makefiles to use
the locally built libraries if the required libraries aren't installed?)

> 
> Also, use of -D should cause xfsrestore to fail.

That's not necessary - there are no libdm calls in xfsrestore,
regardless of whether or not the image being restore came from
xfsdump -a.

There are several solutions here:

1) Russell's proposal -- noop hsm routines if libdm doesn't exist on
build machine.

2) Require libdm on build machine, change xfsdump to dlopen libdm so
that libdm is optional at run-time.

3) Add make_handle() routine to libhandle. xfsdump's only dependencies
from libdm are dm_make_handle() and dm_handle_to_fsid() (the latter
of which is in libhandle as handle_to_fshandle(), I think).

4) Noop the existing hsm routines, and allow xfsdump to dlopen a
specified .so to override the default (noop) behavior. DMF could
then ship a .so implementing those functions.

Bill

Re: review: Simple patch to remove the dmapi support from xfsdump

[Dean Roehrich]

On Fri, Aug 04, 2006 at 11:45:26AM -0500, Bill Kendall wrote:

> 3) Add make_handle() routine to libhandle. xfsdump's only dependencies
> from libdm are dm_make_handle() and dm_handle_to_fsid() (the latter
> of which is in libhandle as handle_to_fshandle(), I think).

If libhandle can satisfy everything then that would be a suitable solution.

If Russell wants this changed bad enough then I'm sure he'll get around to
submitting another patch for it someday...

:)

Dean

Re: review: Simple patch to remove the dmapi support from xfsdump

[Eric Sandeen]

Bill Kendall wrote:
> Dean Roehrich wrote:
>> On Fri, Aug 04, 2006 at 10:36:37AM -0500, Russell Cattelan wrote:
>>
>>
>>> I appears as if the Hsm routines are only used if the -a flag is 
>>> given to xfsdump.
>>> (dump DMF dualstate files as offline)
>>
>>
>> If use of -a causes xfsdump to fail then that may be sufficient.
> 
> Then what? Each user (admittedly, this is a small number) has to rebuild
> xfsdump locally, and then do so every time their package manager grabs
> an updated xfsdump? 

Well... if you're using SLES or ProPack or whatever, -a will work fine, when you 
get updates from your dmapi-supporting distro it will continue to work fine...

But if you're using fedora or whatnot, -a will return a (hopefully helpful) 
error message and quit, and if you -really- want to use dmapi, you'll have to 
build a custom kernel and dmapi userspace anyway.  Rebuilding xfsprogs to go 
with it doesn't seem like too big a deal...  In such a scenario you'd tell your 
package manager to exclude both kernel & xfsprogs.

Anyway, other proposals also seem reasonable, so I won't push this point too much :)

-Eric

Re: review: Simple patch to remove the dmapi support from xfsdump

[Russell Cattelan]

Bill Kendall wrote:

[snip]
>
> 3) Add make_handle() routine to libhandle. xfsdump's only dependencies
> from libdm are dm_make_handle() and dm_handle_to_fsid() (the latter
> of which is in libhandle as handle_to_fshandle(), I think).
Hmm looking at dm_handle_to_fsid: it calls parse_handle which appears
be quite dmapi specific and would require much of dm_handle.c?

I suppose we could move dm_handle.c into  libhandle ?
But that seems to be going in the wrong direction in terms of
correct code compartmentalization.

Since dmapi is only available  on Suse , Propack and somebody doing
a custom kernel, I'm not convinced that xfs dump/restore should
support dmapi no matter what.
The only time a problem might arise is somebody not using the shipped 
version
of xfs dump/restore on a Suse or propack system, in which case they
should either know what they are doing or get what they deserve.

>
> 4) Noop the existing hsm routines, and allow xfsdump to dlopen a
> specified .so to override the default (noop) behavior. DMF could
> then ship a .so implementing those functions.
>
> Bill
>

Re: review: Simple patch to remove the dmapi support from xfsdump

[Bill Kendall]

[-- Attachment #1: Type: text/plain, Size: 1537 bytes --]

Here's a patch that accomplishes #3. Turns out no libhandle
changes were required. Built debian and rpm packages and
verified that dmapi/libdm were not mentioned in the dependencies,
and for debian that libdm was not mentioned in the build-deps either.

I'd still like to move the current hsmapi.c out of xfsdump, but it's
just not much of a priority right now.

Bill

Russell Cattelan wrote:
> Bill Kendall wrote:
> 
> [snip]
> 
>>
>> 3) Add make_handle() routine to libhandle. xfsdump's only dependencies
>> from libdm are dm_make_handle() and dm_handle_to_fsid() (the latter
>> of which is in libhandle as handle_to_fshandle(), I think).
> 
> Hmm looking at dm_handle_to_fsid: it calls parse_handle which appears
> be quite dmapi specific and would require much of dm_handle.c?
> 
> I suppose we could move dm_handle.c into  libhandle ?
> But that seems to be going in the wrong direction in terms of
> correct code compartmentalization.
> 
> Since dmapi is only available  on Suse , Propack and somebody doing
> a custom kernel, I'm not convinced that xfs dump/restore should
> support dmapi no matter what.
> The only time a problem might arise is somebody not using the shipped 
> version
> of xfs dump/restore on a Suse or propack system, in which case they
> should either know what they are doing or get what they deserve.
> 
>>
>> 4) Noop the existing hsm routines, and allow xfsdump to dlopen a
>> specified .so to override the default (noop) behavior. DMF could
>> then ship a .so implementing those functions.
>>
>> Bill
>>

[-- Attachment #2: no_dmapi --]
[-- Type: text/plain, Size: 10537 bytes --]

Index: xfs/xfsdump/common/hsmapi.c
===================================================================
--- xfs.orig/xfsdump/common/hsmapi.c	2006-08-04 13:39:36.088684549 -0500
+++ xfs/xfsdump/common/hsmapi.c	2006-08-04 15:34:42.512343704 -0500
@@ -18,8 +18,7 @@
 
 #include <xfs/xfs.h>
 #include <attr/attributes.h>
-#include <xfs/handle.h>
-#include <xfs/dmapi.h>
+#include <xfs/jdm.h>
 
 #include "hsmapi.h"
 #include "mlog.h"
@@ -86,13 +85,12 @@
 #define	DMF_ST_NOMIGR		5	/* file should not be migrated */
 #define	DMF_ST_PARTIAL		6	/* file has backups plus parts online */
 
-/* Interesting bit combinations within the bs_dmevmask field of xfs_bstat_t:
+/* Create mask containing read, write, trunc and destroy dmapi event bits.
+ * Hardcode the values since xfsdump should not depend on dmapi being installed.
+ * Interesting bit combinations within the bs_dmevmask field of xfs_bstat_t:
  * OFL, UNM, and PAR files have exactly these bits set.
  * DUL and MIG files have all but the DM_EVENT_READ bit set */
-#define DMF_EV_BITS	( (1<<DM_EVENT_DESTROY) | \
-			  (1<<DM_EVENT_READ)    | \
-			  (1<<DM_EVENT_WRITE)   | \
-			  (1<<DM_EVENT_TRUNCATE) )
+#define DMF_EV_BITS	( (1<<16) | (1<<17) | (1<<18) | (1<<20) )
 
 /* OFL file's managed region event flags */
 #define DMF_MR_FLAGS	( 0x1 | 0x2 | 0x4 )
@@ -103,7 +101,7 @@
 
 typedef	struct	{
 	int		dumpversion;
-	dm_fsid_t	fsid;
+	jdm_fshandle_t  *fshanp;
 } dmf_fs_ctxt_t;
 
 typedef	struct	{
@@ -183,30 +181,11 @@
 	int		dumpversion)
 {
 	dmf_fs_ctxt_t	*dmf_fs_ctxtp;
-	void		*fshanp;
-	size_t		fshlen = 0;
-	dm_fsid_t	fsid;
-	int		error;
 
 	if (dumpversion != HSM_API_VERSION_1) {
 		return NULL;		/* we can't handle this version */
 	}
 
-	/* Get the filesystem's DMAPI fsid for later use in building file
-	   handles in HsmInitFileContext.  We use path_to_handle() because
-	   dm_path_to_handle() doesn't work if the filesystem isn't mounted
-	   with -o dmi.
-	*/
-
-	if (path_to_fshandle((char *)mountpoint, &fshanp, &fshlen)) {
-		return NULL;
-	}
-	error = dm_handle_to_fsid(fshanp, fshlen, &fsid);
-	free_handle(fshanp, fshlen);
-	if (error) {
-		return NULL;
-	}
-
 	/* Malloc space for a filesystem context, and initialize any fields
 	   needed later by other routines.
 	*/
@@ -215,7 +194,15 @@
 		return NULL;
 	}
 	dmf_fs_ctxtp->dumpversion = dumpversion;
-	dmf_fs_ctxtp->fsid = fsid;
+
+	/* Get the filesystem's handle for later use in building file
+	   handles in HsmInitFileContext.
+	*/
+	dmf_fs_ctxtp->fshanp = jdm_getfshandle((char *)mountpoint);
+	if (dmf_fs_ctxtp->fshanp == NULL) {
+		free(dmf_fs_ctxtp);
+		return NULL;
+	}
 
 	return (hsm_fs_ctxt_t *)dmf_fs_ctxtp;
 }
@@ -411,10 +398,6 @@
 {
 	dmf_f_ctxt_t	*dmf_f_ctxtp = (dmf_f_ctxt_t *)contextp;
 	XFSattrvalue0_t	*dmfattrp;
-	void		*hanp;
-	size_t		hlen = 0;
-	dm_ino_t	ino;
-	dm_igen_t	igen;
 	int		state;
 	int		error;
 	attr_multiop_t	attr_op;
@@ -437,13 +420,6 @@
 	   for the DMF attribute.  (It could be in a disk block separate from
 	   the inode.)
 	*/
-
-	ino = (dm_ino_t)statp->bs_ino;
-	igen = (dm_igen_t)statp->bs_gen;
-	if (dm_make_handle(&dmf_f_ctxtp->fsys.fsid, &ino, &igen, &hanp, &hlen) != 0) {
-		return 0;	/* can't make a proper handle */
-	}
-
 	attr_op.am_opcode    = ATTR_OP_GET;
 	attr_op.am_error     = 0;
 	attr_op.am_attrname  = DMF_ATTR_NAME;
@@ -451,8 +427,11 @@
 	attr_op.am_length    = sizeof(dmf_f_ctxtp->attrval);
 	attr_op.am_flags     = ATTR_ROOT;
 
-	error = attr_multi_by_handle(hanp, hlen, &attr_op, 1, 0);
-	free_handle(hanp, hlen);
+	error = jdm_attr_multi(dmf_f_ctxtp->fsys.fshanp,
+			       (xfs_bstat_t *)statp,
+			       (char *)&attr_op,
+			       1,
+			       0);
 	if (error || attr_op.am_error)
 		return 0; /* no DMF attribute */
 
Index: xfs/xfsdump/aclocal.m4
===================================================================
--- xfs.orig/xfsdump/aclocal.m4	2006-08-04 15:36:45.207573713 -0500
+++ xfs/xfsdump/aclocal.m4	2006-08-04 15:39:03.904854078 -0500
@@ -157,33 +157,6 @@
 	exit 1 ])
   ])
 
-AC_DEFUN([AC_PACKAGE_NEED_XFS_DMAPI_H],
-  [ AC_CHECK_HEADERS([xfs/dmapi.h])
-    if test "$ac_cv_header_xfs_dmapi_h" != yes; then
-        echo
-        echo 'FATAL ERROR: could not find a valid DMAPI library header.'
-        echo 'Install the data migration API (dmapi) development package.'
-        echo 'Alternatively, run "make install-dev" from the dmapi source.'
-        exit 1
-    fi
-  ])
-
-AC_DEFUN([AC_PACKAGE_NEED_MAKEHANDLE_LIBDM],
-  [ AC_CHECK_LIB(dm, dm_make_handle,, [
-        echo
-        echo 'FATAL ERROR: could not find a valid DMAPI base library.'
-        echo 'Install the data migration API (dmapi) library package.'
-        echo 'Alternatively, run "make install" from the dmapi source.'
-        exit 1
-    ])
-    libdm="-ldm"
-    test -f `pwd`/../dmapi/libdm/libdm.la && \
-        libdm="`pwd`/../dmapi/libdm/libdm.la"
-    test -f ${libexecdir}${libdirsuffix}/libdm.la && \
-	libdm="${libexecdir}${libdirsuffix}/libdm.la"
-    AC_SUBST(libdm)
-  ])
-
 #
 # Generic macro, sets up all of the global packaging variables.
 # The following environment variables may be set to override defaults:
Index: xfs/xfsdump/dump/Makefile
===================================================================
--- xfs.orig/xfsdump/dump/Makefile	2006-08-04 15:43:59.901686947 -0500
+++ xfs/xfsdump/dump/Makefile	2006-08-04 15:44:09.206843880 -0500
@@ -85,7 +85,7 @@
 HFILES = $(LOCALINCL) $(COMMINCL) $(INVINCL)
 LINKS  = $(COMMINCL) $(COMMON) $(INVINCL) $(INVCOMMON)
 LDIRT = $(LINKS)
-LLDLIBS = $(LIBUUID) $(LIBHANDLE) $(LIBATTR) $(LIBDM) $(LIBRMT)
+LLDLIBS = $(LIBUUID) $(LIBHANDLE) $(LIBATTR) $(LIBRMT)
 LTDEPENDENCIES = $(LIBRMT)
 
 LCFLAGS = -DDUMP -DRMT -DBASED -DDOSOCKS -DINVCONVFIX -DSIZEEST -DPIPEINVFIX
Index: xfs/xfsdump/include/builddefs.in
===================================================================
--- xfs.orig/xfsdump/include/builddefs.in	2006-08-04 15:41:12.564871559 -0500
+++ xfs/xfsdump/include/builddefs.in	2006-08-04 15:41:54.018029788 -0500
@@ -13,7 +13,6 @@
 LOADERFLAGS = @LDFLAGS@
 
 LIBRMT = $(TOPDIR)/librmt/librmt.la
-LIBDM = @libdm@
 LIBXFS = @libxfs@
 LIBATTR = @libattr@
 LIBUUID = @libuuid@
Index: xfs/xfsdump/debian/control
===================================================================
--- xfs.orig/xfsdump/debian/control	2006-08-04 15:44:59.345076705 -0500
+++ xfs/xfsdump/debian/control	2006-08-04 15:45:06.637983169 -0500
@@ -2,7 +2,7 @@
 Section: admin
 Priority: optional
 Maintainer: Nathan Scott <nathans@debian.org>
-Build-Depends: xfslibs-dev (>= 2.6.4), uuid-dev, libdm0-dev, libattr1-dev (>= 2.4.14), libncurses-dev, autoconf, debhelper (>= 5), gettext, libtool
+Build-Depends: xfslibs-dev (>= 2.6.4), uuid-dev, libattr1-dev (>= 2.4.14), libncurses-dev, autoconf, debhelper (>= 5), gettext, libtool
 Standards-Version: 3.5.9
 
 Package: xfsdump
Index: xfs/xfsdump/m4/package_dmapidev.m4
===================================================================
--- xfs.orig/xfsdump/m4/package_dmapidev.m4	2006-08-04 15:46:39.293496665 -0500
+++ /dev/null	1970-01-01 00:00:00.000000000 +0000
@@ -1,26 +0,0 @@
-AC_DEFUN([AC_PACKAGE_NEED_XFS_DMAPI_H],
-  [ AC_CHECK_HEADERS([xfs/dmapi.h])
-    if test "$ac_cv_header_xfs_dmapi_h" != yes; then
-        echo
-        echo 'FATAL ERROR: could not find a valid DMAPI library header.'
-        echo 'Install the data migration API (dmapi) development package.'
-        echo 'Alternatively, run "make install-dev" from the dmapi source.'
-        exit 1
-    fi
-  ])
-
-AC_DEFUN([AC_PACKAGE_NEED_MAKEHANDLE_LIBDM],
-  [ AC_CHECK_LIB(dm, dm_make_handle,, [
-        echo
-        echo 'FATAL ERROR: could not find a valid DMAPI base library.'
-        echo 'Install the data migration API (dmapi) library package.'
-        echo 'Alternatively, run "make install" from the dmapi source.'
-        exit 1
-    ])
-    libdm="-ldm"
-    test -f `pwd`/../dmapi/libdm/libdm.la && \
-        libdm="`pwd`/../dmapi/libdm/libdm.la"
-    test -f ${libexecdir}${libdirsuffix}/libdm.la && \
-	libdm="${libexecdir}${libdirsuffix}/libdm.la"
-    AC_SUBST(libdm)
-  ])
Index: xfs/xfsdump/debian/shlibs.local
===================================================================
--- xfs.orig/xfsdump/debian/shlibs.local	2006-08-04 15:47:45.321698050 -0500
+++ xfs/xfsdump/debian/shlibs.local	2006-08-04 15:47:50.806379180 -0500
@@ -1,3 +1,2 @@
-libdm		0	libdm0   (>= 2.1.0)
 libattr		1	libattr1 (>= 2.0.0)
 libhandle	1	xfsprogs (>= 2.6.30)
Index: xfs/xfsdump/restore/Makefile
===================================================================
--- xfs.orig/xfsdump/restore/Makefile	2006-08-04 15:48:11.880996211 -0500
+++ xfs/xfsdump/restore/Makefile	2006-08-04 15:48:21.402178453 -0500
@@ -95,7 +95,7 @@
 HFILES = $(LOCALINCL) $(COMMINCL) $(INVINCL)
 LINKS  = $(COMMINCL) $(COMMON) $(INVINCL) $(INVCOMMON)
 LDIRT = $(LINKS)
-LLDLIBS = $(LIBUUID) $(LIBHANDLE) $(LIBATTR) $(LIBDM) $(LIBRMT)
+LLDLIBS = $(LIBUUID) $(LIBHANDLE) $(LIBATTR) $(LIBRMT)
 LTDEPENDENCIES = $(LIBRMT)
 
 LCFLAGS = -DRESTORE -DRMT -DBASED -DDOSOCKS -DINVCONVFIX -DPIPEINVFIX \
Index: xfs/xfsdump/build/rpm/xfsdump.spec.in
===================================================================
--- xfs.orig/xfsdump/build/rpm/xfsdump.spec.in	2006-08-04 16:02:28.743149991 -0500
+++ xfs/xfsdump/build/rpm/xfsdump.spec.in	2006-08-04 16:02:36.688132300 -0500
@@ -5,7 +5,7 @@
 Distribution: @pkg_distribution@
 Packager: Silicon Graphics, Inc. <http://www.sgi.com/>
 BuildRoot: @build_root@ 
-Requires: xfsprogs >= 2.6.30, dmapi >= 2.0.0, attr >= 2.0.0
+Requires: xfsprogs >= 2.6.30, attr >= 2.0.0
 Source: @pkg_name@-@pkg_version@.src.tar.gz
 License: GPL
 Vendor: Silicon Graphics, Inc.
Index: xfs/xfsdump/configure.in
===================================================================
--- xfs.orig/xfsdump/configure.in	2006-08-04 15:49:23.473884494 -0500
+++ xfs/xfsdump/configure.in	2006-08-04 15:49:37.967683519 -0500
@@ -31,9 +31,6 @@
 AC_PACKAGE_NEED_XFS_HANDLE_H
 AC_PACKAGE_NEED_OPEN_BY_FSHANDLE
 
-AC_PACKAGE_NEED_XFS_DMAPI_H
-AC_PACKAGE_NEED_MAKEHANDLE_LIBDM
-
 AC_PACKAGE_NEED_ATTRIBUTES_H
 AC_PACKAGE_NEED_ATTRIBUTES_MACROS
 AC_PACKAGE_NEED_ATTRGET_LIBATTR
Index: xfs/xfsdump/m4/Makefile
===================================================================
--- xfs.orig/xfsdump/m4/Makefile	2006-08-04 16:03:15.648948927 -0500
+++ xfs/xfsdump/m4/Makefile	2006-08-04 16:03:21.013612090 -0500
@@ -8,7 +8,6 @@
 LSRCFILES = \
 	manual_format.m4 \
 	package_attrdev.m4 \
-	package_dmapidev.m4 \
 	package_globals.m4 \
 	package_ncurses.m4 \
 	package_utilies.m4 \

Re: review: Simple patch to remove the dmapi support from xfsdump

[Dean Roehrich]

On Fri, Aug 04, 2006 at 04:59:45PM -0500, Bill Kendall wrote:

> -#define DMF_EV_BITS	( (1<<DM_EVENT_DESTROY) | \
> -			  (1<<DM_EVENT_READ)    | \
> -			  (1<<DM_EVENT_WRITE)   | \
> -			  (1<<DM_EVENT_TRUNCATE) )
> +#define DMF_EV_BITS	( (1<<16) | (1<<17) | (1<<18) | (1<<20) )

Don't do that.

Granted, those bits can never be changed else all of your customers will start
a lynch mob and come after you.

At the very least, don't allow those bits to be anonymous--copy that whole
enum from the dmapi header.  Even that I object to, but at least the bits will
_be_ something.

Dean

Re: review: Simple patch to remove the dmapi support from xfsdump

[Russell Cattelan]

Dean Roehrich wrote:
> On Fri, Aug 04, 2006 at 04:59:45PM -0500, Bill Kendall wrote:
>
>   
>> -#define DMF_EV_BITS	( (1<<DM_EVENT_DESTROY) | \
>> -			  (1<<DM_EVENT_READ)    | \
>> -			  (1<<DM_EVENT_WRITE)   | \
>> -			  (1<<DM_EVENT_TRUNCATE) )
>> +#define DMF_EV_BITS	( (1<<16) | (1<<17) | (1<<18) | (1<<20) )
>>     
>
> Don't do that.
>
> Granted, those bits can never be changed else all of your customers will start
> a lynch mob and come after you.
>
> At the very least, don't allow those bits to be anonymous--copy that whole
> enum from the dmapi header.  Even that I object to, but at least the bits will
> _be_ something.
>   
I'll second that.
It seems rather dangerous to have a #define floating around that could
potentially get out of sync with the original, especially if you 
transport the
number and not the enum table. (It make it really hard for cscope to 
find :-)

Other than that the rest of the patch seem reasonable, it satisfies the goal
of not  requiring libdmapi.

> Dean
>
>   

Re: review: Simple patch to remove the dmapi support from xfsdump

[Chris Wedgwood]

On Mon, Aug 07, 2006 at 10:30:15AM -0500, Russell Cattelan wrote:

> It seems rather dangerous to have a #define floating around that
> could potentially get out of sync with the original, especially if
> you transport the number and not the enum table. (It make it really
> hard for cscope to find :-)

If it saves a header I don't see any harm in the original approach
with a comment preceding it explain what the bits are and why not to
screw them up.

Re: review: Simple patch to remove the dmapi support from xfsdump

[Dean Roehrich]

On Mon, Aug 07, 2006 at 08:52:48AM -0700, Chris Wedgwood wrote:
> On Mon, Aug 07, 2006 at 10:30:15AM -0500, Russell Cattelan wrote:
> 
> > It seems rather dangerous to have a #define floating around that
> > could potentially get out of sync with the original, especially if
> > you transport the number and not the enum table. (It make it really
> > hard for cscope to find :-)
> 
> If it saves a header I don't see any harm in the original approach
> with a comment preceding it explain what the bits are and why not to
> screw them up.

I agree with Russell that you want cscope to tell you about every place those
bits are being used, so you want those defines or at least those symbols to
show up here.  Having them in a comment won't help cscope.

Dean

Re: review: Simple patch to remove the dmapi support from xfsdump

[Bill Kendall]

On 08/07/06 11:51, Dean Roehrich wrote:
> On Mon, Aug 07, 2006 at 08:52:48AM -0700, Chris Wedgwood wrote:
>> On Mon, Aug 07, 2006 at 10:30:15AM -0500, Russell Cattelan wrote:
>>
>>> It seems rather dangerous to have a #define floating around that
>>> could potentially get out of sync with the original, especially if
>>> you transport the number and not the enum table. (It make it really
>>> hard for cscope to find :-)
>> If it saves a header I don't see any harm in the original approach
>> with a comment preceding it explain what the bits are and why not to
>> screw them up.
> 
> I agree with Russell that you want cscope to tell you about every place those
> bits are being used, so you want those defines or at least those symbols to
> show up here.  Having them in a comment won't help cscope.
> 
> Dean

I'll put these defines in hsmapi.c, and revert DMF_EV_BITS back to using
these symbols:

/* DM_EVENT_* are defined in <xfs/dmapi.h>. Trying to avoid a dmapi dependency
  * in xfsdump since dmapi is not commonly used, yet this code needs to know some
  * of the event bits.
  */
#define	DM_EVENT_READ		16
#define	DM_EVENT_WRITE		17
#define	DM_EVENT_TRUNCATE	18
#define	DM_EVENT_DESTROY	20

Bill

Re: review: Simple patch to remove the dmapi support from xfsdump

[Dean Roehrich]

On Mon, Aug 07, 2006 at 02:13:38PM -0500, Bill Kendall wrote:

> I'll put these defines in hsmapi.c, and revert DMF_EV_BITS back to using
> these symbols:

I guess I'd favor copying in the whole enum.  But I'm a bit more concerned
about losing parts of a story than other people.

Dean

Re: review: Simple patch to remove the dmapi support from xfsdump

[Vlad Apostolov]

Dean Roehrich wrote:
> On Fri, Aug 04, 2006 at 02:18:13PM +1000, Vlad Apostolov wrote:
>   
>> Hi Russel,
>>
>> I don't understand in details the build changes but they seam to be fine.
>>     
>
> Vlad, why do they seem fine?
>   
Well, I patiently waited with the hope that someone  more knowledgeable 
on the topic would
do the review.  No one responded in time and I thought that either no 
one cares  about the
change or no one fully understands it. As I am supposed to support 
DMAPI, I had to take
the review and after consulting locally with xfs developers I did the 
review. Now I see
that there are people interested and knowing better than me what needs 
to be done.
>> In summary if the build system can find the DMAPI lib installed it will
>> use hsmapi_noop.c otherwise hsmapi.c.
>> The return code of HsmInitFileContext() stub probably should be non zero.
>>
>> It is looking good.
>>     
>
> So this is determined at build time...so when you're using a version of
> xfsdump/xfsrestore how do you know you're _not_ using one that is DMAPI-aware
> _before_ you get into trouble with an invalid dump?
>
> Assuming that issue is addressed, here's another:  The libdm is a shared
> object, so why not take advantage of that and load it with dlopen?  Then the
> issue is determined at runtime rather than build time.  This is easy, and even
> DMF does it this way.
>
> Dean
>   

Re: review: Simple patch to remove the dmapi support from xfsdump

[Bill Kendall]

I'm concerned about distributions inadvertently turning off dmapi
support in their xfsdump builds. An explicit ./configure switch
to disable dmapi support would be better, IMHO. But either way
it would be a build-time decision rather than a run-time decision.

xfsdump only uses one DMAPI call (dm_make_handle), so this could
easily be done with dlopen/dlsym, and this would eliminate the
dmapi requirement for those not using -a. dmapi would still be
a build requirement for xfsdump.

Ideally, hsmapi.c wouldn't even exist in xfsdump -- the current
implementation belongs in DMF. There should be a plugin mechanism
added to xfsdump to allow the loading libraries that implement
the API in hsmapi.c (though that interface needs some work first).

Bill

Russell Cattelan wrote:
> Since very few people run xfs with dmapi enabled it seem kind of silly
> to force the install of the dmapi utils/libraries just for xfs 
> dump/restore.
> 
> This patch stubs out the Hsm functions and adjusts the autoconf
> to not fail in the case of missing dmapi headers/libs.
> 
> If somebody does trie to do a dump with the -a option a ERROR message
> is issued, the dump does continue? this might not be the desired behavior.
> 
> 
> ------------------------------------------------------------------------
> 
> Index: xfs-cmds/xfsdump/aclocal.m4
> ===================================================================
> --- xfs-cmds.orig/xfsdump/aclocal.m4	2006-08-02 14:51:34.000000000 -0500
> +++ xfs-cmds/xfsdump/aclocal.m4	2006-08-02 14:58:11.250605250 -0500
> @@ -161,27 +161,31 @@ AC_DEFUN([AC_PACKAGE_NEED_XFS_DMAPI_H],
>    [ AC_CHECK_HEADERS([xfs/dmapi.h])
>      if test "$ac_cv_header_xfs_dmapi_h" != yes; then
>          echo
> -        echo 'FATAL ERROR: could not find a valid DMAPI library header.'
> -        echo 'Install the data migration API (dmapi) development package.'
> +        echo 'WARNING: could not find a valid DMAPI library header.'
> +        echo 'xfsdump / xfsrestore will not support dmapi file systems.'
>          echo 'Alternatively, run "make install-dev" from the dmapi source.'
> -        exit 1
>      fi
>    ])
>  
>  AC_DEFUN([AC_PACKAGE_NEED_MAKEHANDLE_LIBDM],
> -  [ AC_CHECK_LIB(dm, dm_make_handle,, [
> +  [ AC_CHECK_LIB(dm, dm_make_handle,
> +	[have_dmapi=true
> + 	libdm="-ldm"
> +	    test -f `pwd`/../dmapi/libdm/libdm.la && \
> +	        libdm="`pwd`/../dmapi/libdm/libdm.la"
> +	    test -f ${libexecdir}${libdirsuffix}/libdm.la && \
> +		libdm="${libexecdir}${libdirsuffix}/libdm.la"
> +	],
> +	[
>          echo
> -        echo 'FATAL ERROR: could not find a valid DMAPI base library.'
> -        echo 'Install the data migration API (dmapi) library package.'
> +        echo 'WARNING: could not find a valid DMAPI base library.'
> +        echo 'xfsdump / xfsrestore will not support dmapi file systems.'
>          echo 'Alternatively, run "make install" from the dmapi source.'
> -        exit 1
> +	have_dmapi=false
> +	libdm = ""
>      ])
> -    libdm="-ldm"
> -    test -f `pwd`/../dmapi/libdm/libdm.la && \
> -        libdm="`pwd`/../dmapi/libdm/libdm.la"
> -    test -f ${libexecdir}${libdirsuffix}/libdm.la && \
> -	libdm="${libexecdir}${libdirsuffix}/libdm.la"
>      AC_SUBST(libdm)
> +    AC_SUBST(have_dmapi)
>    ])
>  
>  # 
> Index: xfs-cmds/xfsdump/common/hsmapi_noop.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ xfs-cmds/xfsdump/common/hsmapi_noop.c	2006-08-02 15:38:39.046333000 -0500
> @@ -0,0 +1,147 @@
> +/*
> + * Copyright (c) 2000-2004 Silicon Graphics, Inc.
> + * All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it would be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write the Free Software Foundation,
> + * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + */
> +
> +#include <xfs/xfs.h>
> +#include <attr/attributes.h>
> +#include <xfs/handle.h>
> +
> +#include "hsmapi.h"
> +#include "mlog.h"
> +#include "exit.h"
> +
> +typedef	struct	{
> +} dmf_fs_ctxt_t;
> +
> +extern hsm_fs_ctxt_t *
> +HsmInitFsysContext(
> +const	char		*mountpoint,
> +	int		dumpversion)
> +{
> +	mlog( MLOG_NORMAL | MLOG_ERROR, _(
> +		      "-%c Dumping of DMF dualstate file not supported in this version\n"));
> +	return NULL;
> +}
> +
> +extern void
> +HsmDeleteFsysContext(
> +	hsm_fs_ctxt_t	*contextp)
> +{
> +}
> +
> +extern int
> +HsmEstimateFileSpace(
> +	hsm_fs_ctxt_t	*contextp,
> +const	xfs_bstat_t	*statp,
> +	off64_t		*bytes)
> +{
> +	return 0;
> +}
> +
> +extern int
> +HsmEstimateFileOffset(
> +	hsm_fs_ctxt_t	*contextp,
> +const	xfs_bstat_t	*statp,
> +	off64_t		bytecount,
> +	off64_t		*byteoffset)
> +{
> +	return 0;
> +}
> +
> +extern hsm_f_ctxt_t *
> +HsmAllocateFileContext(
> +	hsm_fs_ctxt_t	*contextp)
> +{
> +	return NULL;
> +}
> +
> +extern void
> +HsmDeleteFileContext(
> +	hsm_f_ctxt_t	*contextp)
> +{
> +}
> +
> +extern int
> +HsmInitFileContext(
> +	hsm_f_ctxt_t	*contextp,
> +const	xfs_bstat_t	*statp)
> +{
> +	return 0;
> +}
> +
> +extern int
> +HsmModifyInode(
> +	hsm_f_ctxt_t	*contextp,
> +	xfs_bstat_t	*statp)
> +{
> +	return 0;
> +}
> +
> +extern int
> +HsmModifyExtentMap(
> +	hsm_f_ctxt_t	*contextp,
> +	struct getbmapx	*bmap)
> +{
> +	return 0;
> +}
> +
> +extern int
> +HsmFilterExistingAttribute(
> +	hsm_f_ctxt_t	*hsm_f_ctxtp,
> +const	char		*namep,		/* attribute name */
> +	u_int32_t	valuesz,	/* value size */
> +	int		flag,
> +	int		*skip_entry)
> +{
> +	return 0;
> +}
> +
> +extern int
> +HsmAddNewAttribute(
> +	hsm_f_ctxt_t	*hsm_f_ctxtp,
> +	int		cursor,
> +	int		flag,
> +	char		**namepp,	/* pointer to new attribute name */
> +	char		**valuepp,	/* pointer to its value */
> +	u_int32_t	*valueszp)	/* pointer to the value size */
> +{
> +	return 0;
> +}
> +
> +extern void
> +HsmBeginRestoreFile(
> +	bstat_t		*bstatp,
> +	int		fd,
> +	int		*hsm_flagp)
> +{
> +}
> +
> +extern void
> +HsmRestoreAttribute(
> +	int		flag,		/* ext attr flags */
> +	char		*namep,		/* pointer to new attribute name */
> +	int		*hsm_flagp)
> +{
> +}
> +
> +extern void
> +HsmEndRestoreFile(
> +	char		*path,
> +	int		fd,
> +	int		*hsm_flagp)
> +{
> +}
> Index: xfs-cmds/xfsdump/dump/Makefile
> ===================================================================
> --- xfs-cmds.orig/xfsdump/dump/Makefile	2006-08-02 14:51:34.000000000 -0500
> +++ xfs-cmds/xfsdump/dump/Makefile	2006-08-02 14:52:15.076345750 -0500
> @@ -57,7 +57,6 @@ COMMON = \
>  	fs.c \
>  	getdents.c \
>  	global.c \
> -	hsmapi.c \
>  	lock.c \
>  	main.c \
>  	mlog.c \
> @@ -69,6 +68,12 @@ COMMON = \
>  	util.c \
>  	sproc.c
>  
> +ifeq ($(HAVE_DMAPI), true)
> +  COMMON += hsmapi.c
> +else
> +  COMMON += hsmapi_noop.c
> +endif
> +
>  LOCALS = \
>  	content.c \
>  	inomap.c \
> Index: xfs-cmds/xfsdump/include/builddefs.in
> ===================================================================
> --- xfs-cmds.orig/xfsdump/include/builddefs.in	2006-08-02 14:51:34.000000000 -0500
> +++ xfs-cmds/xfsdump/include/builddefs.in	2006-08-02 14:52:15.080346000 -0500
> @@ -66,6 +66,8 @@ ENABLE_GETTEXT	= @enable_gettext@
>  
>  HAVE_ZIPPED_MANPAGES = @have_zipped_manpages@
>  
> +HAVE_DMAPI = @have_dmapi@
> +
>  LCFLAGS += -DXFS_BIG_FILES=1 -DXFS_BIG_FILESYSTEMS=1
>  
>  ifeq ($(PKG_PLATFORM),linux)
> Index: xfs-cmds/xfsdump/restore/Makefile
> ===================================================================
> --- xfs-cmds.orig/xfsdump/restore/Makefile	2006-08-02 14:51:34.000000000 -0500
> +++ xfs-cmds/xfsdump/restore/Makefile	2006-08-02 14:52:15.080346000 -0500
> @@ -55,7 +55,6 @@ COMMON = \
>  	fs.c \
>          getdents.c \
>  	global.c \
> -	hsmapi.c \
>  	lock.c \
>  	main.c \
>  	mlog.c \
> @@ -67,6 +66,12 @@ COMMON = \
>  	stream.c \
>  	util.c
>  
> +ifeq ($(HAVE_DMAPI), true)
> +  COMMON += hsmapi.c
> +else
> +  COMMON += hsmapi_noop.c
> +endif
> +
>  LOCALS = \
>  	bag.c \
>  	content.c \