Re: [PATCH 02/11] xfsprogs: avoid dependency on linux XATTR_SIZE/LIST_MAX

[Dave Chinner <david@fromorbit.com>]

On Wed, Aug 26, 2015 at 02:02:26PM +0200, Jan Tulak wrote:
> Signed-off-by: Jan Tulak <jtulak@redhat.com>

Explanation of the change?

> ---
>  libhandle/handle.c       |  6 ++++--
>  libhandle/jdm.c          |  6 ++++--
>  libxfs/xfs_attr_remote.c |  2 +-
>  libxfs/xfs_format.h      | 11 ++++++++++-
>  4 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/libhandle/handle.c b/libhandle/handle.c
> index b1c0c10..d532f44 100644
> --- a/libhandle/handle.c
> +++ b/libhandle/handle.c
....
> diff --git a/libhandle/jdm.c b/libhandle/jdm.c
> index d804423..db7d1fe 100644
> --- a/libhandle/jdm.c
> +++ b/libhandle/jdm.c
> @@ -21,6 +21,8 @@
>  #include "handle.h"
>  #include "jdm.h"
>  #include "parent.h"
> +#include "xfs/xfs_arch.h"
> +#include "xfs/xfs_format.h"

I don't think you need the xfs/ prefix now.

> diff --git a/libxfs/xfs_format.h b/libxfs/xfs_format.h
> index bb7cc04..2380084 100644
> --- a/libxfs/xfs_format.h
> +++ b/libxfs/xfs_format.h
> @@ -60,6 +60,14 @@ struct xfs_ifork;
>  #define	XFS_SB_VERSION_MOREBITSBIT	0x8000
>  
>  /*
> + * Avoid dependency on linux values of XATTR.
> + * It has to be on the beginning of this file, because we use these Values
> + * later in this header file.
> + */

This is an explanation of why the change is being made, not what
the definitions are for. This belongs in the commit message, not
the code.

> +#define XFS_XATTR_SIZE_MAX 65536    /* size of an extended attribute value (64k) */

We try to avoid comments like these for defines (they tend only to
be added to structure definitions now).

/*
 * The size of a single extended attribute on disk is limited by
 * the size of index values within the attribute entries themselves.
 * These are be16 fields, so we can only support attribute data
 * sizes up to 2^16 bytes in length.
 */
#define XFS_XATTR_SIZE_MAX	(1 << 16)


> +#define XFS_XATTR_LIST_MAX 65536    /* size of extended attribute namelist (64k) */

XATTR_LIST_MAX is not an on-disk format definition - it's a syscall
buffer size limit and is defined by the OS. This belongs in the
platform headers, such as:

#ifndef XATTR_LIST_MAX
#define XATTR_LIST_MAX	65536
#endif

And so in a common header (e.g. include/xfs.h after including the
platform headers):

#define XFS_XATTR_LIST_MAX	XATTR_LIST_MAX

IOWs, this is really two separate patches - one for the
XFS_XATTR_SIZE_MAX change (which also needs to go back to the
kernel) and one for XFS_XATTR_LIST_MAX (which is purely userspace).

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs