[PATCH v2 3/4] hfsplus: rework functionality of getting, setting and deleting of extended attributes

[PATCH v2 3/4] hfsplus: rework functionality of getting, setting and deleting of extended attributes

[Vyacheslav Dubeyko]

Hi,

This patch reworks functionality of getting, setting and deleting of extended attributes.

With the best regards,
Vyacheslav Dubeyko.
---
From: Vyacheslav Dubeyko <slava@dubeyko.com>
Subject: [PATCH v2 3/4] hfsplus: rework functionality of getting, setting and deleting of extended attributes

This patch reworks functionality of getting, setting and deleting of extended attributes.

Reported-by: Hin-Tak Leung <htl10@users.sourceforge.net>
Signed-off-by: Vyacheslav Dubeyko <slava@dubeyko.com>
---
 fs/hfsplus/ioctl.c |  542 +++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 473 insertions(+), 69 deletions(-)
 mode change 100644 => 100755 fs/hfsplus/ioctl.c

diff --git a/fs/hfsplus/ioctl.c b/fs/hfsplus/ioctl.c
old mode 100644
new mode 100755
index 09addc8..51b78f8c
--- a/fs/hfsplus/ioctl.c
+++ b/fs/hfsplus/ioctl.c
@@ -152,47 +152,205 @@ long hfsplus_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	}
 }
 
+static int strcmp_xattr_finder_info(const char *name)
+{
+	if (name) {
+		return strncmp(name, HFSPLUS_XATTR_FINDER_INFO_NAME,
+				sizeof(HFSPLUS_XATTR_FINDER_INFO_NAME));
+	}
+	return -1;
+}
+
+static int strcmp_xattr_acl(const char *name)
+{
+	if (name) {
+		return strncmp(name, HFSPLUS_XATTR_ACL_NAME,
+				sizeof(HFSPLUS_XATTR_ACL_NAME));
+	}
+	return -1;
+}
+
 int hfsplus_setxattr(struct dentry *dentry, const char *name,
-		     const void *value, size_t size, int flags)
+			const void *value, size_t size, int flags)
 {
+	int err = 0;
 	struct inode *inode = dentry->d_inode;
-	struct hfs_find_data fd;
+	struct hfs_find_data cat_fd;
 	hfsplus_cat_entry entry;
-	struct hfsplus_cat_file *file;
-	int res;
-
-	if (!S_ISREG(inode->i_mode) || HFSPLUS_IS_RSRC(inode))
+	u16 cat_entry_flags, cat_entry_type;
+	u16 folder_finderinfo_len = sizeof(struct DInfo) +
+					sizeof(struct DXInfo);
+	u16 file_finderinfo_len = sizeof(struct FInfo) +
+					sizeof(struct FXInfo);
+
+	if ((!S_ISREG(inode->i_mode) &&
+			!S_ISDIR(inode->i_mode)) ||
+				HFSPLUS_IS_RSRC(inode))
 		return -EOPNOTSUPP;
 
-	res = hfs_find_init(HFSPLUS_SB(inode->i_sb)->cat_tree, &fd);
-	if (res)
-		return res;
-	res = hfsplus_find_cat(inode->i_sb, inode->i_ino, &fd);
-	if (res)
-		goto out;
-	hfs_bnode_read(fd.bnode, &entry, fd.entryoffset,
-			sizeof(struct hfsplus_cat_file));
-	file = &entry.file;
+	err = hfs_find_init(HFSPLUS_SB(inode->i_sb)->cat_tree, &cat_fd);
+	if (err) {
+		printk(KERN_ERR "hfs: can't init xattr find struct\n");
+		return err;
+	}
 
-	if (!strcmp(name, "hfs.type")) {
-		if (size == 4)
-			memcpy(&file->user_info.fdType, value, 4);
-		else
-			res = -ERANGE;
-	} else if (!strcmp(name, "hfs.creator")) {
-		if (size == 4)
-			memcpy(&file->user_info.fdCreator, value, 4);
-		else
-			res = -ERANGE;
-	} else
-		res = -EOPNOTSUPP;
-	if (!res) {
-		hfs_bnode_write(fd.bnode, &entry, fd.entryoffset,
-				sizeof(struct hfsplus_cat_file));
+	err = hfsplus_find_cat(inode->i_sb, inode->i_ino, &cat_fd);
+	if (err) {
+		printk(KERN_ERR "hfs: catalog searching failed\n");
+		goto end_setxattr;
+	}
+
+	if (!strcmp_xattr_finder_info(name)) {
+		if (flags & XATTR_CREATE) {
+			printk(KERN_ERR "hfs: xattr exists yet\n");
+			err = -EOPNOTSUPP;
+			goto end_setxattr;
+		}
+		hfs_bnode_read(cat_fd.bnode, &entry, cat_fd.entryoffset,
+					sizeof(hfsplus_cat_entry));
+		if (be16_to_cpu(entry.type) == HFSPLUS_FOLDER) {
+			if (size == folder_finderinfo_len) {
+				memcpy(&entry.folder.user_info, value,
+						folder_finderinfo_len);
+				hfs_bnode_write(cat_fd.bnode, &entry,
+					cat_fd.entryoffset,
+					sizeof(struct hfsplus_cat_folder));
+				hfsplus_mark_inode_dirty(inode,
+						HFSPLUS_I_CAT_DIRTY);
+			} else {
+				err = -ERANGE;
+				goto end_setxattr;
+			}
+		} else if (be16_to_cpu(entry.type) == HFSPLUS_FILE) {
+			if (size == file_finderinfo_len) {
+				memcpy(&entry.file.user_info, value,
+						file_finderinfo_len);
+				hfs_bnode_write(cat_fd.bnode, &entry,
+					cat_fd.entryoffset,
+					sizeof(struct hfsplus_cat_file));
+				hfsplus_mark_inode_dirty(inode,
+						HFSPLUS_I_CAT_DIRTY);
+			} else {
+				err = -ERANGE;
+				goto end_setxattr;
+			}
+		} else {
+			err = -EOPNOTSUPP;
+			goto end_setxattr;
+		}
+		goto end_setxattr;
+	}
+
+	if (!HFSPLUS_SB(inode->i_sb)->attr_tree) {
+		err = -EOPNOTSUPP;
+		goto end_setxattr;
+	}
+
+	if (hfsplus_attr_exists(inode, name)) {
+		if (flags & XATTR_CREATE) {
+			printk(KERN_ERR "hfs: xattr exists yet\n");
+			err = -EOPNOTSUPP;
+			goto end_setxattr;
+		}
+		err = hfsplus_delete_attr(inode, name);
+		if (err)
+			goto end_setxattr;
+		err = hfsplus_create_attr(inode, name, value, size);
+		if (err)
+			goto end_setxattr;
+	} else {
+		if (flags & XATTR_REPLACE) {
+			printk(KERN_ERR "hfs: cannot replace xattr\n");
+			err = -EOPNOTSUPP;
+			goto end_setxattr;
+		}
+		err = hfsplus_create_attr(inode, name, value, size);
+		if (err)
+			goto end_setxattr;
+	}
+
+	cat_entry_type = hfs_bnode_read_u16(cat_fd.bnode, cat_fd.entryoffset);
+	if (cat_entry_type == HFSPLUS_FOLDER) {
+		cat_entry_flags = hfs_bnode_read_u16(cat_fd.bnode,
+				    cat_fd.entryoffset +
+				    offsetof(struct hfsplus_cat_folder, flags));
+		cat_entry_flags |= HFSPLUS_XATTR_EXISTS;
+		if (!strcmp_xattr_acl(name))
+			cat_entry_flags |= HFSPLUS_ACL_EXISTS;
+		hfs_bnode_write_u16(cat_fd.bnode, cat_fd.entryoffset +
+				offsetof(struct hfsplus_cat_folder, flags),
+				cat_entry_flags);
+		hfsplus_mark_inode_dirty(inode, HFSPLUS_I_CAT_DIRTY);
+	} else if (cat_entry_type == HFSPLUS_FILE) {
+		cat_entry_flags = hfs_bnode_read_u16(cat_fd.bnode,
+				    cat_fd.entryoffset +
+				    offsetof(struct hfsplus_cat_file, flags));
+		cat_entry_flags |= HFSPLUS_XATTR_EXISTS;
+		if (!strcmp_xattr_acl(name))
+			cat_entry_flags |= HFSPLUS_ACL_EXISTS;
+		hfs_bnode_write_u16(cat_fd.bnode, cat_fd.entryoffset +
+				    offsetof(struct hfsplus_cat_file, flags),
+				    cat_entry_flags);
 		hfsplus_mark_inode_dirty(inode, HFSPLUS_I_CAT_DIRTY);
+	} else {
+		printk(KERN_ERR "hfs: invalid catalog entry type\n");
+		err = -EIO;
+		goto end_setxattr;
 	}
-out:
-	hfs_find_exit(&fd);
+
+end_setxattr:
+	hfs_find_exit(&cat_fd);
+	return err;
+}
+
+static ssize_t hfsplus_getxattr_finder_info(struct dentry *dentry,
+						void *value, size_t size)
+{
+	ssize_t res = 0;
+	struct inode *inode = dentry->d_inode;
+	struct hfs_find_data fd;
+	u16 entry_type;
+	u16 folder_rec_len = sizeof(struct DInfo) + sizeof(struct DXInfo);
+	u16 file_rec_len = sizeof(struct FInfo) + sizeof(struct FXInfo);
+	u16 record_len = max(folder_rec_len, file_rec_len);
+	u8 folder_finder_info[sizeof(struct DInfo) + sizeof(struct DXInfo)];
+	u8 file_finder_info[sizeof(struct FInfo) + sizeof(struct FXInfo)];
+
+	if (size >= record_len) {
+		res = hfs_find_init(HFSPLUS_SB(inode->i_sb)->cat_tree, &fd);
+		if (res) {
+			printk(KERN_ERR "hfs: can't init xattr find struct\n");
+			return res;
+		}
+		res = hfsplus_find_cat(inode->i_sb, inode->i_ino, &fd);
+		if (res)
+			goto end_getxattr_finder_info;
+		entry_type = hfs_bnode_read_u16(fd.bnode, fd.entryoffset);
+
+		if (entry_type == HFSPLUS_FOLDER) {
+			hfs_bnode_read(fd.bnode, folder_finder_info,
+				fd.entryoffset +
+				offsetof(struct hfsplus_cat_folder, user_info),
+				folder_rec_len);
+			memcpy(value, folder_finder_info, folder_rec_len);
+			res = folder_rec_len;
+		} else if (entry_type == HFSPLUS_FILE) {
+			hfs_bnode_read(fd.bnode, file_finder_info,
+				fd.entryoffset +
+				offsetof(struct hfsplus_cat_file, user_info),
+				file_rec_len);
+			memcpy(value, file_finder_info, file_rec_len);
+			res = file_rec_len;
+		} else {
+			res = -EOPNOTSUPP;
+			goto end_getxattr_finder_info;
+		}
+	} else
+		res = size ? -ERANGE : record_len;
+
+end_getxattr_finder_info:
+	if (size >= record_len)
+		hfs_find_exit(&fd);
 	return res;
 }
 
@@ -201,60 +359,306 @@ ssize_t hfsplus_getxattr(struct dentry *dentry, const char *name,
 {
 	struct inode *inode = dentry->d_inode;
 	struct hfs_find_data fd;
-	hfsplus_cat_entry entry;
-	struct hfsplus_cat_file *file;
+	hfsplus_attr_entry *entry;
+	u32 record_type;
+	u16 record_length = 0;
 	ssize_t res = 0;
 
-	if (!S_ISREG(inode->i_mode) || HFSPLUS_IS_RSRC(inode))
+	if ((!S_ISREG(inode->i_mode) &&
+			!S_ISDIR(inode->i_mode)) ||
+				HFSPLUS_IS_RSRC(inode))
 		return -EOPNOTSUPP;
 
-	if (size) {
-		res = hfs_find_init(HFSPLUS_SB(inode->i_sb)->cat_tree, &fd);
-		if (res)
-			return res;
-		res = hfsplus_find_cat(inode->i_sb, inode->i_ino, &fd);
-		if (res)
+	if (!strcmp_xattr_finder_info(name))
+		return hfsplus_getxattr_finder_info(dentry, value, size);
+
+	if (!HFSPLUS_SB(inode->i_sb)->attr_tree)
+		return -EOPNOTSUPP;
+
+	entry = hfsplus_alloc_attr_entry();
+	if (!entry) {
+		printk(KERN_ERR "hfs: can't allocate xattr entry\n");
+		return -ENOMEM;
+	}
+
+	res = hfs_find_init(HFSPLUS_SB(inode->i_sb)->attr_tree, &fd);
+	if (res) {
+		printk(KERN_ERR "hfs: can't init xattr find struct\n");
+		goto failed_getxattr_init;
+	}
+
+	res = hfsplus_find_attr(inode->i_sb, inode->i_ino, name, &fd);
+	if (res) {
+		if (res == -ENOENT)
+			res = -ENODATA;
+		else
+			printk(KERN_ERR "hfs: xattr searching failed\n");
+		goto out;
+	}
+
+	hfs_bnode_read(fd.bnode, &record_type,
+			fd.entryoffset, sizeof(record_type));
+	record_type = be32_to_cpu(record_type);
+	if (record_type == HFSPLUS_ATTR_INLINE_DATA) {
+		record_length = hfs_bnode_read_u16(fd.bnode,
+				fd.entryoffset +
+				offsetof(struct hfsplus_attr_inline_data,
+				length));
+		if (record_length > HFSPLUS_MAX_INLINE_DATA_SIZE) {
+			printk(KERN_ERR "hfs: invalid xattr record size\n");
+			res = -EIO;
 			goto out;
-		hfs_bnode_read(fd.bnode, &entry, fd.entryoffset,
-				sizeof(struct hfsplus_cat_file));
+		}
+	} else if (record_type == HFSPLUS_ATTR_FORK_DATA ||
+			record_type == HFSPLUS_ATTR_EXTENTS) {
+		printk(KERN_ERR "hfs: only inline data xattr are supported\n");
+		res = -EOPNOTSUPP;
+		goto out;
+	} else {
+		printk(KERN_ERR "hfs: invalid xattr record\n");
+		res = -EIO;
+		goto out;
+	}
+
+	if (size) {
+		hfs_bnode_read(fd.bnode, entry, fd.entryoffset,
+				offsetof(struct hfsplus_attr_inline_data,
+					raw_bytes) + record_length);
 	}
-	file = &entry.file;
-
-	if (!strcmp(name, "hfs.type")) {
-		if (size >= 4) {
-			memcpy(value, &file->user_info.fdType, 4);
-			res = 4;
-		} else
-			res = size ? -ERANGE : 4;
-	} else if (!strcmp(name, "hfs.creator")) {
-		if (size >= 4) {
-			memcpy(value, &file->user_info.fdCreator, 4);
-			res = 4;
-		} else
-			res = size ? -ERANGE : 4;
+
+	if (size >= record_length) {
+		memcpy(value, entry->inline_data.raw_bytes, record_length);
+		res = record_length;
 	} else
-		res = -EOPNOTSUPP;
+		res = size ? -ERANGE : record_length;
+
 out:
-	if (size)
-		hfs_find_exit(&fd);
+	hfs_find_exit(&fd);
+
+failed_getxattr_init:
+	hfsplus_destroy_attr_entry(entry);
 	return res;
 }
 
-#define HFSPLUS_ATTRLIST_SIZE (sizeof("hfs.creator")+sizeof("hfs.type"))
+static ssize_t hfsplus_listxattr_finder_info(struct dentry *dentry,
+						char *buffer, size_t size)
+{
+	ssize_t res = 0;
+	struct inode *inode = dentry->d_inode;
+	struct hfs_find_data fd;
+	u16 entry_type;
+	u8 folder_finder_info[sizeof(struct DInfo) + sizeof(struct DXInfo)];
+	u8 file_finder_info[sizeof(struct FInfo) + sizeof(struct FXInfo)];
+	unsigned long len, found_bit;
+
+	res = hfs_find_init(HFSPLUS_SB(inode->i_sb)->cat_tree, &fd);
+	if (res) {
+		printk(KERN_ERR "hfs: can't init xattr find struct\n");
+		return res;
+	}
+
+	res = hfsplus_find_cat(inode->i_sb, inode->i_ino, &fd);
+	if (res)
+		goto end_listxattr_finder_info;
+
+	entry_type = hfs_bnode_read_u16(fd.bnode, fd.entryoffset);
+	if (entry_type == HFSPLUS_FOLDER) {
+		len = sizeof(struct DInfo) + sizeof(struct DXInfo);
+		hfs_bnode_read(fd.bnode, folder_finder_info,
+				fd.entryoffset +
+				offsetof(struct hfsplus_cat_folder, user_info),
+				len);
+		found_bit = find_first_bit((void *)folder_finder_info, len*8);
+	} else if (entry_type == HFSPLUS_FILE) {
+		len = sizeof(struct FInfo) + sizeof(struct FXInfo);
+		hfs_bnode_read(fd.bnode, file_finder_info,
+				fd.entryoffset +
+				offsetof(struct hfsplus_cat_file, user_info),
+				len);
+		found_bit = find_first_bit((void *)file_finder_info, len*8);
+	} else {
+		res = -EOPNOTSUPP;
+		goto end_listxattr_finder_info;
+	}
+
+	if (found_bit >= (len*8))
+		res = 0;
+	else {
+		if (!buffer || !size) {
+			res = sizeof(HFSPLUS_XATTR_FINDER_INFO_NAME);
+		} else if (size < sizeof(HFSPLUS_XATTR_FINDER_INFO_NAME)) {
+			res = -ERANGE;
+		} else {
+			strncpy(buffer, HFSPLUS_XATTR_FINDER_INFO_NAME,
+				    sizeof(HFSPLUS_XATTR_FINDER_INFO_NAME));
+			res = sizeof(HFSPLUS_XATTR_FINDER_INFO_NAME);
+		}
+	}
+
+end_listxattr_finder_info:
+	hfs_find_exit(&fd);
+
+	return res;
+}
 
 ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size)
 {
+	ssize_t err;
+	ssize_t res = 0;
 	struct inode *inode = dentry->d_inode;
+	struct hfs_find_data fd;
+	u16 key_len = 0;
+	struct hfsplus_attr_key attr_key;
+	char strbuf[HFSPLUS_ATTR_MAX_STRLEN + 1] = {0};
+	int name_len;
+
+	if ((!S_ISREG(inode->i_mode) &&
+			!S_ISDIR(inode->i_mode)) ||
+				HFSPLUS_IS_RSRC(inode))
+		return -EOPNOTSUPP;
 
-	if (!S_ISREG(inode->i_mode) || HFSPLUS_IS_RSRC(inode))
+	res = hfsplus_listxattr_finder_info(dentry, buffer, size);
+	if (res < 0)
+		return res;
+	else if (!HFSPLUS_SB(inode->i_sb)->attr_tree)
+		return (res == 0) ? -EOPNOTSUPP : res;
+
+	err = hfs_find_init(HFSPLUS_SB(inode->i_sb)->attr_tree, &fd);
+	if (err) {
+		printk(KERN_ERR "hfs: can't init xattr find struct\n");
+		return err;
+	}
+
+	err = hfsplus_find_attr(inode->i_sb, inode->i_ino, NULL, &fd);
+	if (err) {
+		if (err == -ENOENT) {
+			if (res == 0)
+				res = -ENODATA;
+			goto end_listxattr;
+		} else {
+			res = err;
+			goto end_listxattr;
+		}
+	}
+
+	for (;;) {
+		key_len = hfs_bnode_read_u16(fd.bnode, fd.keyoffset);
+		if (key_len == 0 || key_len > fd.tree->max_key_len) {
+			printk(KERN_ERR "hfs: invalid xattr key length: %d\n",
+							key_len);
+			res = -EIO;
+			goto end_listxattr;
+		}
+
+		hfs_bnode_read(fd.bnode, &attr_key,
+				fd.keyoffset, key_len + sizeof(key_len));
+
+		if (be32_to_cpu(attr_key.cnid) != inode->i_ino)
+			goto end_listxattr;
+
+		name_len = HFSPLUS_ATTR_MAX_STRLEN;
+		if (hfsplus_uni2asc(inode->i_sb,
+			(const struct hfsplus_unistr *)&fd.key->attr.key_name,
+					strbuf, &name_len)) {
+			printk(KERN_ERR "hfs: unicode conversion failed\n");
+			res = -EIO;
+			goto end_listxattr;
+		}
+
+		if (!buffer || !size)
+			res += name_len + 1;
+		else if (size < (res + name_len + 1)) {
+			res = -ERANGE;
+			goto end_listxattr;
+		} else {
+			strncpy(buffer + res, strbuf, name_len);
+			memset(buffer + res + name_len, 0, 1);
+			res += name_len + 1;
+		}
+
+		if (hfs_brec_goto(&fd, 1))
+			goto end_listxattr;
+	}
+
+end_listxattr:
+	hfs_find_exit(&fd);
+	return res;
+}
+
+int hfsplus_removexattr(struct dentry *dentry, const char *name)
+{
+	int err = 0;
+	struct inode *inode = dentry->d_inode;
+	struct hfs_find_data cat_fd;
+	u16 flags;
+	u16 cat_entry_type;
+	int is_xattr_acl_deleted = 0;
+	int is_all_xattrs_deleted = 0;
+
+	if ((!S_ISREG(inode->i_mode) &&
+			!S_ISDIR(inode->i_mode)) ||
+				HFSPLUS_IS_RSRC(inode))
+		return -EOPNOTSUPP;
+
+	if (!HFSPLUS_SB(inode->i_sb)->attr_tree)
+		return -EOPNOTSUPP;
+
+	if (!strcmp_xattr_finder_info(name))
 		return -EOPNOTSUPP;
 
-	if (!buffer || !size)
-		return HFSPLUS_ATTRLIST_SIZE;
-	if (size < HFSPLUS_ATTRLIST_SIZE)
-		return -ERANGE;
-	strcpy(buffer, "hfs.type");
-	strcpy(buffer + sizeof("hfs.type"), "hfs.creator");
+	err = hfs_find_init(HFSPLUS_SB(inode->i_sb)->cat_tree, &cat_fd);
+	if (err) {
+		printk(KERN_ERR "hfs: can't init xattr find struct\n");
+		return err;
+	}
+
+	err = hfsplus_find_cat(inode->i_sb, inode->i_ino, &cat_fd);
+	if (err) {
+		printk(KERN_ERR "hfs: catalog searching failed\n");
+		goto end_removexattr;
+	}
 
-	return HFSPLUS_ATTRLIST_SIZE;
+	err = hfsplus_delete_attr(inode, name);
+	if (err)
+		goto end_removexattr;
+
+	is_xattr_acl_deleted = !strcmp_xattr_acl(name);
+	is_all_xattrs_deleted = !hfsplus_attr_exists(inode, NULL);
+
+	if (!is_xattr_acl_deleted && !is_all_xattrs_deleted)
+		goto end_removexattr;
+
+	cat_entry_type = hfs_bnode_read_u16(cat_fd.bnode, cat_fd.entryoffset);
+
+	if (cat_entry_type == HFSPLUS_FOLDER) {
+		flags = hfs_bnode_read_u16(cat_fd.bnode, cat_fd.entryoffset +
+				offsetof(struct hfsplus_cat_folder, flags));
+		if (is_xattr_acl_deleted)
+			flags &= ~HFSPLUS_ACL_EXISTS;
+		if (is_all_xattrs_deleted)
+			flags &= ~HFSPLUS_XATTR_EXISTS;
+		hfs_bnode_write_u16(cat_fd.bnode, cat_fd.entryoffset +
+				offsetof(struct hfsplus_cat_folder, flags),
+				flags);
+		hfsplus_mark_inode_dirty(inode, HFSPLUS_I_CAT_DIRTY);
+	} else if (cat_entry_type == HFSPLUS_FILE) {
+		flags = hfs_bnode_read_u16(cat_fd.bnode, cat_fd.entryoffset +
+				offsetof(struct hfsplus_cat_file, flags));
+		if (is_xattr_acl_deleted)
+			flags &= ~HFSPLUS_ACL_EXISTS;
+		if (is_all_xattrs_deleted)
+			flags &= ~HFSPLUS_XATTR_EXISTS;
+		hfs_bnode_write_u16(cat_fd.bnode, cat_fd.entryoffset +
+				offsetof(struct hfsplus_cat_file, flags),
+				flags);
+		hfsplus_mark_inode_dirty(inode, HFSPLUS_I_CAT_DIRTY);
+	} else {
+		printk(KERN_ERR "hfs: invalid catalog entry type\n");
+		err = -EIO;
+		goto end_removexattr;
+	}
+
+end_removexattr:
+	hfs_find_exit(&cat_fd);
+	return err;
 }
-- 
1.7.9.5

Re: [PATCH v2 3/4] hfsplus: rework functionality of getting, setting and deleting of extended attributes

[Christoph Hellwig]

On Sun, Sep 23, 2012 at 06:49:37PM +0400, Vyacheslav Dubeyko wrote:
> Hi,
> 
> This patch reworks functionality of getting, setting and deleting of extended attributes.

All new or modified xattr implementations should use the generic
dispatching code and just set sb->s_xattr.

Re: [PATCH v2 3/4] hfsplus: rework functionality of getting, setting and deleting of extended attributes

[Vyacheslav Dubeyko]

Hi,

On Tue, 2012-09-25 at 05:45 -0400, Christoph Hellwig wrote:
> On Sun, Sep 23, 2012 at 06:49:37PM +0400, Vyacheslav Dubeyko wrote:
> > Hi,
> > 
> > This patch reworks functionality of getting, setting and deleting of extended attributes.
> 
> All new or modified xattr implementations should use the generic
> dispatching code and just set sb->s_xattr.
> 

As I can understand, you are talking about using xattr_handler's for
dispatching of processing of extended attributes with such complex names
as "system.posix_acl_access" and so on. Am I correct?

The HFS+ has such peculiarities that you can name extended attribute as
you want. And name of extended attribute can keep in Attributes Tree
without any prefixes (for example, as "test"). Moreover, it is not used
such prefixes as "user." or "system." under Mac OS X.

However, it exists under Mac OS X special prefix "com.apple." (for
example, "com.apple.FinderInfo"). So, you suggest to add definition of
additional prefixes "com." and "apple." in include/linux/xattr.h and to
add xattr_handler's for these two prefixes. Am I correct?

Of course, it has to implement xattr_handlers for
"system.posix_acl_access" and "system.posix_acl_default" in the case of
ACLs support implementation. But this patch set doesn't implement ACLs
support for HFS+.

With the best regards,
Vyacheslav Dubeyko. 

Re: [PATCH v2 3/4] hfsplus: rework functionality of getting, setting and deleting of extended attributes

[Christoph Hellwig]

On Tue, Sep 25, 2012 at 02:57:04PM +0400, Vyacheslav Dubeyko wrote:
> As I can understand, you are talking about using xattr_handler's for
> dispatching of processing of extended attributes with such complex names
> as "system.posix_acl_access" and so on. Am I correct?
> 
> The HFS+ has such peculiarities that you can name extended attribute as
> you want. And name of extended attribute can keep in Attributes Tree
> without any prefixes (for example, as "test"). Moreover, it is not used
> such prefixes as "user." or "system." under Mac OS X.

But we will have to use them under Linux.  The whole xattr mechanism
is built around the model that we decide policies, including most
importantly access control, based on these prefixes.  In retrospective
I think the string prefixes are a horrible idea and a simple binary
flag namespace as done by e.g. IRIX or FreeBSD would have made everyones
life a heck lot simpler, but that's not what we implemented 10 years
ago.

> However, it exists under Mac OS X special prefix "com.apple." (for
> example, "com.apple.FinderInfo"). So, you suggest to add definition of
> additional prefixes "com." and "apple." in include/linux/xattr.h and to
> add xattr_handler's for these two prefixes. Am I correct?

No.  I'd suggest mapping any free-form attributes in hfsplus into user.*
in the syscall namespace, while only mapping a few that needs special
treatment into system.* or similar.  Using the proper helpers will make
this actually readable at least.

Re: [PATCH v2 3/4] hfsplus: rework functionality of getting, setting and deleting of extended attributes

[Vyacheslav Dubeyko]

Hi,

On Wed, 2012-09-26 at 08:13 -0400, Christoph Hellwig wrote:
> On Tue, Sep 25, 2012 at 02:57:04PM +0400, Vyacheslav Dubeyko wrote:
> > As I can understand, you are talking about using xattr_handler's for
> > dispatching of processing of extended attributes with such complex names
> > as "system.posix_acl_access" and so on. Am I correct?
> > 
> > The HFS+ has such peculiarities that you can name extended attribute as
> > you want. And name of extended attribute can keep in Attributes Tree
> > without any prefixes (for example, as "test"). Moreover, it is not used
> > such prefixes as "user." or "system." under Mac OS X.
> 
> But we will have to use them under Linux.  The whole xattr mechanism
> is built around the model that we decide policies, including most
> importantly access control, based on these prefixes.  In retrospective
> I think the string prefixes are a horrible idea and a simple binary
> flag namespace as done by e.g. IRIX or FreeBSD would have made everyones
> life a heck lot simpler, but that's not what we implemented 10 years
> ago.
> 
> > However, it exists under Mac OS X special prefix "com.apple." (for
> > example, "com.apple.FinderInfo"). So, you suggest to add definition of
> > additional prefixes "com." and "apple." in include/linux/xattr.h and to
> > add xattr_handler's for these two prefixes. Am I correct?
> 
> No.  I'd suggest mapping any free-form attributes in hfsplus into user.*
> in the syscall namespace, while only mapping a few that needs special
> treatment into system.* or similar.  Using the proper helpers will make
> this actually readable at least.
> 

So, it means that it needs to add special prefix "user." during
listxattr phase for any free-form attributes. And then it needs to
expect on the getxattr phase names with artificially added prefixes (for
example, user.<real-name>) and ignores requests with only real name of
attribute (real name means name of extended attribute that keeps in
metadata).

But it can really confuse, from my point of view. Let's imagine that you
firstly, mount your HFS+ volume under Mac OS X and, secondly, under
Linux. Under Mac OS X you can see real names of the extended attributes
but under Linux you will see another artificially constructed names of
extended attributes for the same file system object:

Mac OS X -> "test"
Linux -> "user.test"

I think this situation can be a very confusing and unexpected for the
end-user. For example, if I remember extended attribute name that I got
under Mac OS X then I can try to get it content by name under Linux (but
will fail in such situation).

I totally agree that it needs to implement prefixes support for the ACLs
case. But I think that it is not proper way for free-form extended
attributes. It needs to remember about possible using the same HFS+
volume as under Mac OS X as under Linux.

With the best regards,
Vyacheslav Dubeyko.


> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] hfsplus: rework functionality of getting, setting and deleting of extended attributes

[Hin-Tak Leung]

--- On Wed, 26/9/12, Vyacheslav Dubeyko <slava@dubeyko.com> wrote:

> Hi,
> 
> On Wed, 2012-09-26 at 08:13 -0400, Christoph Hellwig wrote:
> > On Tue, Sep 25, 2012 at 02:57:04PM +0400, Vyacheslav
> Dubeyko wrote:
> > > As I can understand, you are talking about using
> xattr_handler's for
> > > dispatching of processing of extended attributes
> with such complex names
> > > as "system.posix_acl_access" and so on. Am I
> correct?
> > > 
> > > The HFS+ has such peculiarities that you can name
> extended attribute as
> > > you want. And name of extended attribute can keep
> in Attributes Tree
> > > without any prefixes (for example, as "test").
> Moreover, it is not used
> > > such prefixes as "user." or "system." under Mac OS
> X.
> > 
> > But we will have to use them under Linux.  The
> whole xattr mechanism
> > is built around the model that we decide policies,
> including most
> > importantly access control, based on these
> prefixes.  In retrospective
> > I think the string prefixes are a horrible idea and a
> simple binary
> > flag namespace as done by e.g. IRIX or FreeBSD would
> have made everyones
> > life a heck lot simpler, but that's not what we
> implemented 10 years
> > ago.
> > 
> > > However, it exists under Mac OS X special prefix
> "com.apple." (for
> > > example, "com.apple.FinderInfo"). So, you suggest
> to add definition of
> > > additional prefixes "com." and "apple." in
> include/linux/xattr.h and to
> > > add xattr_handler's for these two prefixes. Am I
> correct?
> > 
> > No.  I'd suggest mapping any free-form attributes
> in hfsplus into user.*
> > in the syscall namespace, while only mapping a few that
> needs special
> > treatment into system.* or similar.  Using the
> proper helpers will make
> > this actually readable at least.
> > 
> 
> So, it means that it needs to add special prefix "user."
> during
> listxattr phase for any free-form attributes. And then it
> needs to
> expect on the getxattr phase names with artificially added
> prefixes (for
> example, user.<real-name>) and ignores requests with
> only real name of
> attribute (real name means name of extended attribute that
> keeps in
> metadata).
> 
> But it can really confuse, from my point of view. Let's
> imagine that you
> firstly, mount your HFS+ volume under Mac OS X and,
> secondly, under
> Linux. Under Mac OS X you can see real names of the extended
> attributes
> but under Linux you will see another artificially
> constructed names of
> extended attributes for the same file system object:
> 
> Mac OS X -> "test"
> Linux -> "user.test"
> 
> I think this situation can be a very confusing and
> unexpected for the
> end-user. For example, if I remember extended attribute name
> that I got
> under Mac OS X then I can try to get it content by name
> under Linux (but
> will fail in such situation).
> 
> I totally agree that it needs to implement prefixes support
> for the ACLs
> case. But I think that it is not proper way for free-form
> extended
> attributes. It needs to remember about possible using the
> same HFS+
> volume as under Mac OS X as under Linux.

I am with Vyacheslav on this - while we should implementing user.* system.* for ACLs , we should not perverse meanings/familiarities of current-, and possibly future-, use of extended attributes in HFS+ under Mac OS X.

In a related but opposite scenario, currently, com.apple.* have special meanings under Mac OS X. If we do map that to system.* , there would be the possibility of a user creating a system.* attribute under linux, which becomes a com.apple.* under Mac OS X, and breaking its use in Mac OS X in small or big ways. Whereas if a user does mess with com.apple.*, he could reasonably expect problems when he tries to use the media under Mac OS X.

There are some patches and reports relating to backing up and restoring Mac OS X with tar or rsync and breaking things on the way.

I think we can create/delete/use xattrs in any way, but we should not represent the attribute's names differently to the user.  

Regards,
Hin-Tak
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html