[Ocfs2-tools-devel] [PATCH 5/7] libocfs2: add set and get xattr operations

[Joel Becker]

On Tue, Oct 20, 2009 at 04:42:01PM +0800, Tiger Yang wrote:
> This patch implement set and get xattr in libocfs2,
> they are very similiar with the functions in kernel.

	Sorry this took so long.  There was a lot of code to read.

> +static const char *ocfs2_xattr_prefix[OCFS2_XATTR_MAX] = {
> +	[OCFS2_XATTR_INDEX_USER]	= "user.",
> +	[OCFS2_XATTR_INDEX_POSIX_ACL_ACCESS]
> +					= "system.posix_acl_access",
> +	[OCFS2_XATTR_INDEX_POSIX_ACL_DEFAULT]
> +					= "system.posix_acl_default",
> +	[OCFS2_XATTR_INDEX_TRUSTED]	= "trusted.",
> +	[OCFS2_XATTR_INDEX_SECURITY]	= "security.",
> +};

	Where do these constants come from?  I mean, I know we copied
them from ext3, but are they required to be a certain value?  We persist
them on disk.  Why are they not in ocfs2_fs.h?

> +static int ocfs2_xattr_find_entry(int name_index,
> +				  const char *name,
> +				  struct ocfs2_xattr_search *xs)
> +{
> +	struct ocfs2_xattr_entry *entry;
> +	size_t name_len;
> +	int i, cmp = 1;
> +
> +	if (name == NULL)
> +		return OCFS2_ET_INVALID_ARGUMENT;
> +
> +	name_len = strlen(name);
> +	entry = xs->here;
> +	for (i = 0; i < xs->header->xh_count; i++) {
> +		cmp = name_index - ocfs2_xattr_get_type(entry);
> +		if (!cmp)
> +			cmp = name_len - entry->xe_name_len;
> +		if (!cmp)
> +			cmp = memcmp(name, xs->base + entry->xe_name_offset,
> +				     name_len);
> +		if (cmp == 0)
> +			break;
> +		entry += 1;
> +	}
> +	xs->here = entry;

	What if nothing is found?  This will set xs->here to just beyond
the end of the entries.  Seems silly.

> +static errcode_t ocfs2_xattr_create_index_block(ocfs2_cached_inode *ci,
> +					struct ocfs2_xattr_search_info *xsi)
> +{
> +	errcode_t ret;
> +	uint32_t len;
> +	uint64_t blkno;
> +	ocfs2_filesys *fs = ci->ci_fs;
> +	struct ocfs2_xattr_block *xb = (struct ocfs2_xattr_block *)
> +					xsi->xi_block->blkbuf;
> +	uint16_t xb_flags = xb->xb_flags;
> +	struct ocfs2_xattr_tree_root *xr = NULL;
> +	char *bucket = xsi->xi_bucket->blkbuf;
> +	size_t offs;
> +
> +	DEBUG("create xattr index block for #%"PRIu64"\n", xb->xb_blkno);
> +
> +	ASSERT(!(xb_flags & OCFS2_XATTR_INDEXED));
> +
> +	ret = ocfs2_new_clusters(fs, 1, 1, &blkno, &len);
> +	if (ret)
> +		return ret;
> +
> +	DEBUG("allocate 1 cluster from #%"PRIu64" to xattr block\n", blkno);
> +
> +	ocfs2_cp_xattr_block_to_bucket(ci, (char *)xb, bucket);
> +
> +	/* update xattr search info for xattr block and xattr bucket*/
> +	offs = (char *)xsi->xi_block->here - (char *)xsi->xi_block->header;
> +	xsi->xi_block->header = NULL;
> +	xsi->xi_block->base = NULL;
> +	xsi->xi_block->end = NULL;
> +	xsi->xi_block->here = NULL;
> +
> +	xsi->xi_bucket->blkno = blkno;
> +	xsi->xi_bucket->header = (struct ocfs2_xattr_header *)bucket;
> +	xsi->xi_bucket->base = bucket;
> +	xsi->xi_bucket->end = xsi->xi_bucket->base + fs->fs_blocksize;
> +	xsi->xi_bucket->here = (struct ocfs2_xattr_entry *)(bucket + offs);

	We calculate offs and xsi->xi_bucket->here base don
xsi->xi_block->here.  But we resorted the entries, so the offset in the
bucket may not be the offset in the block, right?

> diff --git a/libocfs2/xattr.h b/libocfs2/xattr.h
> index 475f0f2..a66b0c0 100644
> --- a/libocfs2/xattr.h
> +++ b/libocfs2/xattr.h
> @@ -17,3 +17,18 @@
>  
>  #define OCFS2_MAX_XATTR_TREE_LEAF_SIZE	65536
>  
> +enum ocfs2_xattr_type {
> +	OCFS2_XATTR_INDEX_USER = 1,
> +	OCFS2_XATTR_INDEX_POSIX_ACL_ACCESS,
> +	OCFS2_XATTR_INDEX_POSIX_ACL_DEFAULT,
> +	OCFS2_XATTR_INDEX_TRUSTED,
> +	OCFS2_XATTR_INDEX_SECURITY,
> +	OCFS2_XATTR_MAX
> +};

	I really think these belong in the on-disk header.  Maybe that's
something for future work.

Joel

-- 

Life's Little Instruction Book #15

	"Own a great stereo system."

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