[Ocfs2-tools-devel] [PATCH 6/6] libocfs2: add set and get xattr operations
Joel Becker
Joel.Becker at oracle.com
Tue Aug 11 14:59:10 PDT 2009
On Wed, Jul 29, 2009 at 03:02:30PM +0800, Tiger Yang wrote:
> --- a/include/ocfs2/ocfs2.h
> +++ b/include/ocfs2/ocfs2.h
> @@ -1176,5 +1176,17 @@ errcode_t ocfs2_xattr_get_clusters(ocfs2_cached_inode *cinode,
> errcode_t ocfs2_xattr_value_shrink(ocfs2_filesys *fs,
> struct ocfs2_xattr_value_root *xv,
> uint32_t new_clusters);
> +const char *ocfs2_xattr_get_prefix(int name_index);
> +errcode_t ocfs2_xattr_get_generic(ocfs2_cached_inode *ci, const char *name,
> + char *buf, size_t count, size_t *got);
> +errcode_t ocfs2_xattr_set_generic(ocfs2_cached_inode *ci,
> + const char *name,
> + const char *buf,
> + size_t count);
> +errcode_t ocfs2_xattr_name_iterate(ocfs2_cached_inode *ci,
> + int (*func)(ocfs2_cached_inode *ci,
> + const char *name,
> + void *priv_data),
> + void *priv_data);
Why are the public functions named
"ocfs2_xattr_[sg]et_generic()" and the internal functions named
"ocfs2_xattr[sg]et()"? Shouldn't it be the other way around?
> diff --git a/libocfs2/xattr.c b/libocfs2/xattr.c
> index c6f69c4..12d23df 100644
> --- a/libocfs2/xattr.c
> +++ b/libocfs2/xattr.c
<snip>
> +#define OCFS2_XATTR_ABORT 0x01
> +#define OCFS2_XATTR_ERROR 0x02
These belong in ocfs2.h alongside ocfs2_xattr_name_iterate(),
right?
> +#define OCFS2_XATTR_HEADER_GAP 4
> +
> +#define XATTR_NODATA (-61)
> +#define XATTR_NOSPC (-28)
What are these? It looks like these are error codes, but you're
not using the existing errno values, and you're not using errcode_t.
Looking at later code, you return these from the get functions,
functions that return errcode_t. That's no good. You should add proper
errors to libocfs2/ocfs2_err.et.
> +static errcode_t __ocfs2_xattr_set_value_outside(ocfs2_cached_inode *ci,
> + struct ocfs2_xattr_value_root *xv,
> + char *blkbuf,
> + uint64_t blkno,
> + const char *value,
> + int value_len)
> +{
> + errcode_t ret = 0;
> + int i, cp_len;
> + ocfs2_filesys *fs = ci->ci_fs;
> + uint16_t blocksize = fs->fs_blocksize;
> + uint32_t p_cluster, num_clusters;
> + uint32_t cpos = 0, bpc = ocfs2_clusters_to_blocks(fs, 1);
> + uint32_t clusters = ocfs2_clusters_in_bytes(fs, value_len);
> + uint64_t p_blkno;
> + char *blk = NULL;
> +
> + ASSERT(clusters <= xv->xr_clusters);
> + ret = ocfs2_malloc_block(fs->fs_io, &blk);
> + if (ret)
> + return ret;
> +
> + while (cpos < clusters) {
> + ret = ocfs2_xattr_get_clusters(ci, cpos, &p_cluster,
> + &num_clusters,
> + &xv->xr_list,
> + blkno, blkbuf);
> + if (ret)
> + goto out;
> +
> + p_blkno = ocfs2_clusters_to_blocks(fs, p_cluster);
> +
> + for (i = 0; i < num_clusters * bpc; i++, p_blkno++) {
> +
> + ret = io_read_block(fs->fs_io, p_blkno, 1, blk);
> + if (ret)
> + goto out;
You're setting the value here, overwriting the old data.
There's no need to read the block.
> +
> + cp_len = value_len > blocksize ? blocksize : value_len;
> + memcpy(blk, value, cp_len);
> + value_len -= cp_len;
> + value += cp_len;
> + if (cp_len < blocksize)
> + memset(blk + cp_len, 0, blocksize - cp_len);
> +
> + ret = io_write_block(fs->fs_io, p_blkno, 1, blk);
> + if (ret)
> + goto out;
> +
> + if (!value_len)
> + break;
> + }
> + cpos += num_clusters;
> + }
Even worse, you're writing a block at a time. You should
allocate a 1MB buffer, and then write out
min(1MB, num_clusters << clustersize_bits).
> +static errcode_t ocfs2_xattr_get_value_outside(ocfs2_cached_inode *ci,
> + struct ocfs2_xattr_value_root *xv,
> + struct ocfs2_xattr_search *xs,
> + void *buffer,
> + size_t len)
> +{
> + uint32_t cpos, p_cluster, num_clusters;
> + uint64_t blkno;
> + int i;
> + size_t cplen;
> + char *buf = NULL;
> + ocfs2_filesys *fs = ci->ci_fs;
> + size_t blocksize = fs->fs_blocksize;
> + uint32_t clusters = xv->xr_clusters;
> + uint32_t bpc = ocfs2_clusters_to_blocks(fs, 1);
> + errcode_t ret = 0;
> +
> + ret = ocfs2_malloc_block(fs->fs_io, &buf);
> + if (ret)
> + return ret;
> +
> + cpos = 0;
> + while (cpos < clusters) {
> + ret = ocfs2_xattr_get_clusters(ci, cpos, &p_cluster,
> + &num_clusters,
> + &xv->xr_list,
> + xs->blkno, xs->blkbuf);
> + if (ret)
> + goto out;
> + blkno = ocfs2_clusters_to_blocks(fs, p_cluster);
> + /* Copy ocfs2_xattr_value */
> + for (i = 0; i < num_clusters * bpc; i++, blkno++) {
> + ret = io_read_block(fs->fs_io, blkno, 1, buf);
> + if (ret)
> + goto out;
> +
> + cplen = len >= blocksize ? blocksize : len;
> + memcpy(buffer, buf, cplen);
> + len -= cplen;
> + buffer += cplen;
> +
> + if (len == 0)
> + break;
> + }
> + cpos += num_clusters;
Again, use a 1MB buffer and read more than one block at a time.
> +static errcode_t ocfs2_xattr_get(ocfs2_cached_inode *ci,
> + int name_index,
> + const char *name,
> + char *buffer,
> + size_t buffer_size,
> + size_t *got)
> +{
> + errcode_t ret = 0;
> + struct ocfs2_dinode *di = ci->ci_inode;
> + ocfs2_filesys *fs = ci->ci_fs;
> + int bpb = ocfs2_blocks_per_xattr_bucket(fs);
> + struct ocfs2_xattr_search xis = {
> + .blkbuf = (char *)ci->ci_inode,
> + .blkno = ci->ci_blkno,
> + .not_found = XATTR_NODATA,
> + };
> + struct ocfs2_xattr_search xbs = {.not_found = XATTR_NODATA,};
> + struct ocfs2_xattr_search xbks = {.not_found = XATTR_NODATA,};
> + struct ocfs2_xattr_search_info xsi = {
> + .xi_inode = &xis,
> + .xi_block = &xbs,
> + .xi_bucket = &xbks,
> + };
> +
> + /* alloc block buffer for xattr block */
> + ret = ocfs2_malloc_block(fs->fs_io, &xbs.blkbuf);
> + if (ret)
> + return ret;
> +
> + /* alloc block buffer for xattr bucket */
> + ret = ocfs2_malloc_blocks(fs->fs_io, bpb, &xbks.blkbuf);
> + if (ret)
> + return ret;
> +
> +
> + ret = ocfs2_xattr_ibody_get(ci, name_index, name, buffer,
> + buffer_size, &xis, got);
> + if (ret == XATTR_NODATA && di->i_xattr_loc)
> + ret = ocfs2_xattr_block_get(ci, name_index, name, buffer,
> + buffer_size, &xsi, got);
> +
> + if (xbs.blkbuf)
> + ocfs2_free(&xbs.blkbuf);
> + if (xbks.blkbuf)
> + ocfs2_free(&xbks.blkbuf);
> + return ret;
> +}
See, here if ibody_get() or block_get() return XATTR_NODATA,
you're going to return that from this function, which purports to return
an errcode_t. Make the xattr errors proper errcode_t values.
> +/*
> + * Read the value of xattr 'name' from the inode 'ci'. Fill 'buf' up
> + * to 'count' bytes. If 'got' is not NULL, fill it with how many bytes
> + * were actually read.
> + */
> +errcode_t ocfs2_xattr_get_generic(ocfs2_cached_inode *ci, const char *name,
> + char *buf, size_t count, size_t *got)
Again, why aren't the public functions named 'ocfs2_xattr_get()'
and 'ocfs2_xattr_set()'?
> +/*
> + * Iterate the xattr names on inode 'ci'. If 'func' returns
> + * OCFS2_XATTR_ABORT or OCFS2_XATTR_ERROR, stop iteration.
> + * If OCFS2_XATTR_ERROR, return an error from ocfs2_xattr_name_iterate.
> + *
> + * If you modify an xattr, you must restart your iteration - there is
> + * no guarantee it is in a consistent state.
> + */
> +
> +errcode_t ocfs2_xattr_name_iterate(ocfs2_cached_inode *ci,
> + int (*func)(ocfs2_cached_inode *ci,
> + const char *name,
> + void *priv_data),
> + void *priv_data)
> +{
> + errcode_t ret = 0;
> + return ret;
> +}
And will you be implementing this?
Joel
--
Life's Little Instruction Book #109
"Know how to drive a stick shift."
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127
More information about the Ocfs2-tools-devel
mailing list