On Wed, 7 Jan 2009 22:56:51 -0500 Chris Mason <chris.mason@xxxxxxxxxx> wrote:
> Xattrs in btrfs are stored in the btree close to the inode item. They use an
> indexing scheme very similar to directories.
>
> The max size of a btrfs xattr is limited by the size of a btree item,
> or about 4k. A future version will support larger items.
>
>
> ...
>
> +ssize_t __btrfs_getxattr(struct inode *inode, const char *name,
> + void *buffer, size_t size)
> +{
> + struct btrfs_dir_item *di;
> + struct btrfs_root *root = BTRFS_I(inode)->root;
> + struct btrfs_path *path;
> + struct extent_buffer *leaf;
> + int ret = 0;
> + unsigned long data_ptr;
> +
> + path = btrfs_alloc_path();
> + if (!path)
> + return -ENOMEM;
> +
> + /* lookup the xattr by name */
> + di = btrfs_lookup_xattr(NULL, root, path, inode->i_ino, name,
> + strlen(name), 0);
> + if (!di || IS_ERR(di)) {
This seems quite bad. An error return from btrfs_lookup_xattr() can
arise from (I assume) OOM, corrupted filesystem, I/O error, etc.
Yet here we treat such errors as being the same as "not found". This
seems grossly misleading to userspace and possible a security problem?
(I'm kind of guessing here, and might be wrong and wasting everyone's
time, because the semantics of the btrfs_lookup_xattr() and
btrfs_getxattr() return values aren't documented).
> + ret = -ENODATA;
> + goto out;
> + }
> +
> + leaf = path->nodes[0];
> + /* if size is 0, that means we want the size of the attr */
> + if (!size) {
> + ret = btrfs_dir_data_len(leaf, di);
> + goto out;
> + }
> +
> + /* now get the data out of our dir_item */
> + if (btrfs_dir_data_len(leaf, di) > size) {
OK, where did we hide the btrfs_dir_data_len() definition?
> + ret = -ERANGE;
> + goto out;
> + }
> + data_ptr = (unsigned long)((char *)(di + 1) +
> + btrfs_dir_name_len(leaf, di));
> + read_extent_buffer(leaf, buffer, data_ptr,
> + btrfs_dir_data_len(leaf, di));
Please document read_extent_buffer().
Please rename read_extent_buffer() to something more appropriate to a
kernel-wide symbol.
<spies map_private_extent_buffer as well>
Please review all kernel-wide identifiers in btrfs for appropriateness.
I'm scratching my head wondering about this `data_ptr' thing. Is it a
disk offset? Is it really a pointer to kernel memory? According to
this code it is indeed a kernel pointer, but it then gets stuffed into
an unsigned long (wtf?) and then passed to the mysterious
read_extent_buffer().
<reviewer throws in the towel on this part of the code>
> + ret = btrfs_dir_data_len(leaf, di);
> +
> +out:
> + btrfs_free_path(path);
> + return ret;
> +}
> +
> +int __btrfs_setxattr(struct inode *inode, const char *name,
> + const void *value, size_t size, int flags)
> +{
> + struct btrfs_dir_item *di;
> + struct btrfs_root *root = BTRFS_I(inode)->root;
> + struct btrfs_trans_handle *trans;
> + struct btrfs_path *path;
> + int ret = 0, mod = 0;
> +
> + path = btrfs_alloc_path();
> + if (!path)
> + return -ENOMEM;
> +
> + trans = btrfs_start_transaction(root, 1);
> + btrfs_set_trans_block_group(trans, inode);
> +
> + /* first lets see if we already have this xattr */
let's :)
> + di = btrfs_lookup_xattr(trans, root, path, inode->i_ino, name,
> + strlen(name), -1);
<wonders what the -1 does>
<goes to the btrfs_lookup_xattr() definition site>
<towel goes flying again>
> + if (IS_ERR(di)) {
> + ret = PTR_ERR(di);
> + goto out;
> + }
> +
> + /* ok we already have this xattr, lets remove it */
> + if (di) {
> + /* if we want create only exit */
> + if (flags & XATTR_CREATE) {
> + ret = -EEXIST;
> + goto out;
> + }
> +
> + ret = btrfs_delete_one_dir_name(trans, root, path, di);
> + if (ret)
> + goto out;
> + btrfs_release_path(root, path);
> +
> + /* if we don't have a value then we are removing the xattr */
> + if (!value) {
> + mod = 1;
> + goto out;
> + }
> + } else {
> + btrfs_release_path(root, path);
> +
> + if (flags & XATTR_REPLACE) {
> + /* we couldn't find the attr to replace */
> + ret = -ENODATA;
> + goto out;
> + }
> + }
> +
> + /* ok we have to create a completely new xattr */
> + ret = btrfs_insert_xattr_item(trans, root, name, strlen(name),
> + value, size, inode->i_ino);
> + if (ret)
> + goto out;
> + mod = 1;
> +
> +out:
> + if (mod) {
> + inode->i_ctime = CURRENT_TIME;
> + ret = btrfs_update_inode(trans, root, inode);
> + }
> +
> + btrfs_end_transaction(trans, root);
> + btrfs_free_path(path);
> + return ret;
> +}
> +
> +ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size)
> +{
> + struct btrfs_key key, found_key;
> + struct inode *inode = dentry->d_inode;
> + struct btrfs_root *root = BTRFS_I(inode)->root;
> + struct btrfs_path *path;
> + struct btrfs_item *item;
> + struct extent_buffer *leaf;
> + struct btrfs_dir_item *di;
> + int ret = 0, slot, advance;
> + size_t total_size = 0, size_left = size;
> + unsigned long name_ptr;
> + size_t name_len;
> + u32 nritems;
> +
> + /*
> + * ok we want all objects associated with this id.
> + * NOTE: we set key.offset = 0; because we want to start with the
> + * first xattr that we find and walk forward
> + */
> + key.objectid = inode->i_ino;
> + btrfs_set_key_type(&key, BTRFS_XATTR_ITEM_KEY);
> + key.offset = 0;
> +
> + path = btrfs_alloc_path();
> + if (!path)
> + return -ENOMEM;
> + path->reada = 2;
<gets interested in btrfs_path.reada>
<greps for a while>
It's snowing towels in here!
path->reada = 2;
gack!
<greps some more>
int direction = path->reada;
...
if (direction < 0) {
if (nr == 0)
break;
nr--;
} else if (direction > 0) {
nr++;
if (nr >= nritems)
break;
}
<delicately moves on...>
> + /* search for our xattrs */
> + ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
> + if (ret < 0)
> + goto err;
> + ret = 0;
this statement isn't needed.
> + advance = 0;
> + while (1) {
> + leaf = path->nodes[0];
> + nritems = btrfs_header_nritems(leaf);
> + slot = path->slots[0];
> +
> + /* this is where we start walking through the path */
> + if (advance || slot >= nritems) {
> + /*
> + * if we've reached the last slot in this leaf we need
> + * to go to the next leaf and reset everything
> + */
> + if (slot >= nritems-1) {
> + ret = btrfs_next_leaf(root, path);
> + if (ret)
> + break;
> + leaf = path->nodes[0];
> + nritems = btrfs_header_nritems(leaf);
> + slot = path->slots[0];
> + } else {
> + /*
> + * just walking through the slots on this leaf
> + */
> + slot++;
> + path->slots[0]++;
> + }
> + }
> + advance = 1;
> +
> + item = btrfs_item_nr(leaf, slot);
> + btrfs_item_key_to_cpu(leaf, &found_key, slot);
> +
> + /* check to make sure this item is what we want */
> + if (found_key.objectid != key.objectid)
> + break;
> + if (btrfs_key_type(&found_key) != BTRFS_XATTR_ITEM_KEY)
> + break;
> +
> + di = btrfs_item_ptr(leaf, slot, struct btrfs_dir_item);
> +
> + name_len = btrfs_dir_name_len(leaf, di);
> + total_size += name_len + 1;
> +
> + /* we are just looking for how big our buffer needs to be */
> + if (!size)
> + continue;
> +
> + if (!buffer || (name_len + 1) > size_left) {
> + ret = -ERANGE;
> + goto err;
> + }
> +
> + name_ptr = (unsigned long)(di + 1);
> + read_extent_buffer(leaf, buffer, name_ptr, name_len);
> + buffer[name_len] = '\0';
> +
> + size_left -= name_len + 1;
> + buffer += name_len + 1;
> + }
> + ret = total_size;
> +
> +err:
> + btrfs_free_path(path);
> +
> + return ret;
> +}
>
> ...
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
