On Mon, Jan 12, 2009 at 08:59:04PM -0800, Andrew Morton wrote:
> 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?
>
Hmm yeah sorry about that, if IS_ERR(di) we should be returning the ERR we got,
I will fix taht.
> (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?
>
Its a macro in ctree.h I believe, just returns the data_len part of the dir item
in that leaf.
> > + 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().
>
Its a pointer to kernel memory, probably should have commented that, sorry again
:). Basically you have the dir item, which holds the name len and data len
info, then directly following that structure you have the name, and then
directly following that you have the "data", which is what is contained in the
xattr. So this is saying read the data from the extent buffer (which is the
leaf) starting from where the data will be.
> <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>
>
Just calls btrfs_path_search with -1 for the ins_len, tells it that we may
remove the item so do the appropriate things to the tree to make it ok to do
that.
> > + 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.
>
Hmm yup you are right, will fix that too. Thanks much,
Josef
--
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
