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

[Tiger Yang]

Joel Becker wrote:
> 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?

>> 	Again, why aren't the public functions named 'ocfs2_xattr_get()'
>> and 'ocfs2_xattr_set()'?

I presume "generic" is more public. Because the parameter "name" is a
full name string, like "user.name", in this function, we split the full
name to name_index and xattr name.
Anyway, I can exchange the function name if they are not appropriate.


>> +#define OCFS2_XATTR_ABORT	0x01
>> +#define OCFS2_XATTR_ERROR	0x02
> 
> 	These belong in ocfs2.h alongside ocfs2_xattr_name_iterate(),
> right?

right. will correct them in next version.

> 
>> +#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.

OK. will correct them in next version.

>> +			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.

OK.

> 	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).

Got it.


> 	Again, use a 1MB buffer and read more than one block at a time.

OK.


> 
> 	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.

OK.

>> +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?

No, I will remove this. I think this function is not necessary. I add 
listing xattr function in debugfs, and our set/get operations will 
search all xattr entry.

thanks,
tiger