[Ocfs2-tools-devel] [PATCH 6/9] ocfs2-tools: add xattr support in lib

[Joel Becker]

On Fri, Dec 19, 2008 at 05:41:38PM +0800, Tiger Yang wrote:
> This patch add xattr APIs in libocfs2.
> Such as, read/write xattr block,
> swap xattr structure from/to cpu,
> get xattr record, etc.

	After we land this patch seris, we'll want to flesh out the API
with ocfs2_xattr_get/set/list(), and bucket code, etc.  But for now,
some cleanups inline.

> 
> Signed-off-by: Tiger Yang <tiger.yang at oracle.com>
> ---
>  include/ocfs2/ocfs2.h  |   22 ++++
>  libocfs2/extend_file.c |   38 +++++++
>  libocfs2/ocfs2_err.et  |    4 +-
>  libocfs2/xattr.c       |  272 ++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 335 insertions(+), 1 deletions(-)
> 
> diff --git a/include/ocfs2/ocfs2.h b/include/ocfs2/ocfs2.h
> index dfc2089..4b01d27 100644
> --- a/include/ocfs2/ocfs2.h
> +++ b/include/ocfs2/ocfs2.h
> @@ -1084,5 +1084,27 @@ errcode_t ocfs2_block_iterate_inode(ocfs2_filesys *fs,
>  				    void *priv_data);
>  
>  uint32_t xattr_uuid_hash(unsigned char *uuid);
> +int ocfs2_xattr_find_leaf(ocfs2_filesys *fs, struct ocfs2_xattr_block *xb,
> +			  uint32_t cpos, char **leaf_buf);
> +int ocfs2_extent_recs_per_xattr_block(int blocksize);
> +uint16_t ocfs2_xattr_buckets_per_cluster(ocfs2_filesys *fs);
> +uint16_t ocfs2_blocks_per_xattr_bucket(ocfs2_filesys *fs);
> +void ocfs2_swap_xattr_header(struct ocfs2_xattr_header *xh);
> +void ocfs2_swap_xattr_entries_to_cpu(struct ocfs2_xattr_header *xh);
> +void ocfs2_swap_xattr_entries_from_cpu(struct ocfs2_xattr_header *xh);
> +void ocfs2_swap_xattr_block_to_cpu(struct ocfs2_xattr_block *xb);
> +void ocfs2_swap_xattr_block_from_cpu(struct ocfs2_xattr_block *xb);

	Do we really need swap_header/entries in ocfs2.h?  Why doesn't
swap_block work for all callers.  I see in other code you call them for
swapping buckets, but it would seem that we could have an API for
swapping a bucket.
	Maybe this belongs with a later bucket discussion, though.  Not
a show-stopper for inclusion.

>  #endif  /* _FILESYS_H */
> diff --git a/libocfs2/extend_file.c b/libocfs2/extend_file.c
> index 73e4033..a27b478 100644
> --- a/libocfs2/extend_file.c
> +++ b/libocfs2/extend_file.c
> @@ -1027,6 +1027,44 @@ out:
>  	return ret;
>  }
>  
> +int ocfs2_xattr_find_leaf(ocfs2_filesys *fs, struct ocfs2_xattr_block *xb,
> +			  uint32_t cpos, char **leaf_buf)
> +{
> +	int ret;
> +	char *buf = NULL;
> +	struct ocfs2_path *path = NULL;
> +	struct ocfs2_extent_list *el = &xb->xb_attrs.xb_root.xt_list;
> +
> +	assert(el->l_tree_depth > 0);
> +
> +
> +	ret = ocfs2_malloc0(sizeof(*path), &path);
> +
> +	if (!path) {
> +		ret = OCFS2_ET_NO_MEMORY;
> +		goto out;
> +	} else {
> +		path->p_tree_depth = el->l_tree_depth;
> +		path->p_node[0].blkno = xb->xb_blkno;
> +		path->p_node[0].buf = (char *)xb;
> +		path->p_node[0].el = el;
> +	}
> +
> +	ret = ocfs2_find_path(fs, path, cpos);
> +	if (ret)
> +		goto out;
> +
> +	ret = ocfs2_malloc_block(fs->fs_io, &buf);
> +	if (ret)
> +		goto out;
> +
> +	memcpy(buf, path_leaf_buf(path), fs->fs_blocksize);
> +	*leaf_buf = buf;
> +out:
> +	ocfs2_free_path(path);
> +	return ret;
> +}

	My first thought was that we should have generalized
ocfs2_find_leaf() to take multiple extent tree types like we did in the
kernel.  But you managed to get everything you needed from this one
small wrapper function.  So perhaps we don't need to make deeper changes
to the extent code.  Maybe this approach will work for indexed dirs too.

> diff --git a/libocfs2/xattr.c b/libocfs2/xattr.c
> index 287ce0e..86899a9 100644
> --- a/libocfs2/xattr.c
> +++ b/libocfs2/xattr.c
> @@ -32,3 +32,275 @@ uint32_t xattr_uuid_hash(unsigned char *uuid)
>  	return hash;
>  }
>  
> +int ocfs2_extent_recs_per_xattr_block(int blocksize)
> +{
> +	int size;
> +
> +	size = blocksize - offsetof(struct ocfs2_xattr_block,
> +				    xb_attrs.xb_root.xt_list.l_recs);
> +
> +	return size / sizeof(struct ocfs2_extent_rec);
> +}

	This is ocfs2_xattr_recs_per_xb() in ocfs2_fs.h.  No need to
duplicate.

> +static void ocfs2_swap_xattr_entry(struct ocfs2_xattr_entry *xe)
> +{
> +	xe->xe_name_offset	= bswap_16(xe->xe_name_offset);
> +	xe->xe_value_size	= bswap_64(xe->xe_value_size);
> +}

	Why don't we swap xe_name_hash?  Is that always calculated and
compared in little-endian?

> +static void ocfs2_swap_block_check(struct ocfs2_block_check *xk)
> +{
> +	xk->bc_crc32e		= bswap_32(xk->bc_crc32e);
> +	xk->bc_ecc		= bswap_16(xk->bc_ecc);
> +}

	Don't swap the block_check structure.  Only the ecc code touches
it.

> +errcode_t ocfs2_read_xattr_block(ocfs2_filesys *fs,
> +				 uint64_t blkno,
> +				 char *xb_buf)
> +{
> +	errcode_t ret = 0;
> +	char *blk;
> +	struct ocfs2_xattr_block *xb;
> +
> +	if ((blkno < OCFS2_SUPER_BLOCK_BLKNO) ||
> +	    (blkno > fs->fs_blocks))
> +		return OCFS2_ET_BAD_BLKNO;
> +
> +	ret = ocfs2_malloc_block(fs->fs_io, &blk);
> +	if (ret)
> +		return ret;
> +
> +	ret = io_read_block(fs->fs_io, blkno, 1, blk);
> +	if (ret)
> +		goto out;
> +
> +	xb = (struct ocfs2_xattr_block *)blk;
> +
> +	if (memcmp(xb->xb_signature, OCFS2_XATTR_BLOCK_SIGNATURE,
> +		   strlen(OCFS2_XATTR_BLOCK_SIGNATURE))) {
> +		ret = OCFS2_ET_BAD_XATTR_BLOCK_MAGIC;
> +		goto out;
> +	}

	If these patches go in after the metaecc branch, they need to
compute and validate eccs.  Thus, the validate_meta_ecc call needs to be
before the signature check here.

> +errcode_t ocfs2_write_xattr_block(ocfs2_filesys *fs,
> +				  uint64_t blkno,
> +				  char *xb_buf)
> +{
> +	errcode_t ret = 0;
> +	char *blk;
> +	struct ocfs2_xattr_block *xb;
> +
> +	if (!(fs->fs_flags & OCFS2_FLAG_RW))
> +		return OCFS2_ET_RO_FILESYS;
> +
> +	if ((blkno < OCFS2_SUPER_BLOCK_BLKNO) ||
> +	    (blkno > fs->fs_blocks))
> +		return OCFS2_ET_BAD_BLKNO;
> +
> +	ret = ocfs2_malloc_block(fs->fs_io, &blk);
> +	if (ret)
> +		return ret;
> +
> +	memcpy(blk, xb_buf, fs->fs_blocksize);
> +
> +	xb = (struct ocfs2_xattr_block *)blk;
> +	ocfs2_swap_xattr_block_from_cpu(xb);
> +
> +	ret = io_write_block(fs->fs_io, blkno, 1, blk);
> +	if (ret)
> +		goto out;

	And compute_meta_ecc() needs to be before io_write_block() here.
	Also, with no bucket API, there is no validation of the bucket
ecc.

Joel

-- 

"Egotist: a person more interested in himself than in me."
         - Ambrose Bierce 

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127