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

Dave Chinner
2015-08-26T22:01:44Z

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