On 09/12/2015 09:15 AM, Sheng Yong wrote:
On 9/11/2015 2:25 PM, Dongsheng Yang wrote:
On 09/11/2015 02:18 PM, Sheng Yong wrote:
Hi, Dongsheng
On 9/11/2015 1:01 PM, Dongsheng Yang wrote:
On 09/11/2015 05:09 PM, Sheng Yong wrote:
[...]
Why move it? If you just want to use them before the definitions,
Just declare them before using.
OK.
int ubifs_do_setxattr(struct inode *host, const char *name,
const void *value, size_t size, int flags)
{
@@ -348,7 +462,19 @@ int ubifs_do_setxattr(struct inode *host, const char *name,
goto out_free;
}
+#ifdef CONFIG_UBIFS_FS_POSIX_ACL
+ if (size == 0) {
+ ubifs_assert(inode->i_nlink == 1);
+ clear_nlink(inode);
+ err = remove_xattr(c, host, inode, &nm);
+ if (err)
+ set_nlink(inode, 1);
+ iput(inode);
+ goto out_free;
+ }
+#endif
Is there a testcase for it?
I test `setfacl -b/-k'. I don't know how setfacl is implemented. But for ACL_TYPE_ACCESS,
ubifs_setxattr() is called with size = 0 and value = NULL; while for ACL_TYPE_DEFAULT,
ubifs_removexattr() is called.
I mean, if we don't add the code above, what kind of problem
we would meet?
Then, a new xattr wich length 0 and value NULL, is written to flash, which is useless.
I don't think we should keep this xattr.
err = change_xattr(c, host, inode, value, size);
+
iput(inode);
out_free:
@@ -359,6 +485,9 @@ out_free:
int ubifs_setxattr(struct dentry *dentry, const char *name,
const void *value, size_t size, int flags)
{
+#ifdef CONFIG_UBIFS_FS_POSIX_ACL
+ const struct xattr_handler *handler;
+#endif
struct qstr nm = QSTR_INIT(name, strlen(name));
int type;
@@ -369,6 +498,16 @@ int ubifs_setxattr(struct dentry *dentry, const char *name,
if (type < 0)
return type;
+#ifdef CONFIG_UBIFS_FS_POSIX_ACL
+ if (type == POSIX_ACL_DEFAULT || type == POSIX_ACL_ACCESS) {
+ if (type == POSIX_ACL_DEFAULT)
+ handler = &posix_acl_default_xattr_handler;
+ if (type == POSIX_ACL_ACCESS)
+ handler = &posix_acl_access_xattr_handler;
+ return handler->set(dentry, name, value, size, flags,
+ handler->flags);
+ }
+#endif
What about setting sb->s_xattr and calling generic_setxattr() here?
I have no idea if we should do this :(
If we do, I think, we should call generic functions for all xattr.
I copied the xattr handler discussion from the other email, so that
we could have it at one place :)
That's great :)
No, only for POSIX_ACL_DEFAULT|POSIX_ACL_ACCESS currently. Then
Something like that:
if (type == POSIX_ACL_DEFAULT || type == POSIX_ACL_ACCESS)
return generic_setxattr(dentry, name, value, size, flags);
Acutally, in my v1 patchset, I did like this. The URL is:
http://lists.infradead.org/pipermail/linux-mtd/2015-March/058451.html
Security handler is different with acl handler. Please read the
security_getxattr(), it just return ubifs_getxattr(). That means
we can use ubifs_getxattr() immediately. So we can remove the
security_handler for ubifs.
But acl is different, we need to call a special ubifs_get_acl()
here. Then I found you get the handler by youself in the
ubifs_get|set_xattr() and call handler->set() by youself.
Right, they are different. I mean using the generic way means
ubifs_[get|set]xattr should be modified.
That's not good idea. Please Just set the handler and call
generic_setxattr().
But IMO, calling generic_setxattr means we have to add
posix_acl_[access|default]_xattr_handler in ubifs_xattr_handlers, like
const struct xattr_handler *ubifs_xattr_handlers[] = {
&ubifs_xattr_security_handler,
+#ifdef CONFIG_UBIFS_FS_POSIX_ACL
+ &posix_acl_access_xattr_handler,
+ &posix_acl_default_xattr_handler,
+#endif
NULL,
};
Yes, obviously.
then, generic_setxattr() could call posix_acl_***_xattr_handler.set(),
which will call ubifs_set_acl() to do the real setting. Since security
xattr dead code is going to be removed, ubifs_xattr_handlers may go
away too.
So there would be a conflict with the other patch from Richard to
remove ubifs_xattr_handlers. But that's not a problem at all, just
address it when merging.We do not use security_handler in ubifs, but
that does not mean we can not use sb->xattr in ubifs.
So I agree with Andreas that if we could add all xattr handlers
in ubifs_xattr_handlers (which means ubifs_[get|set]xattr should be
modified and generic_[get|set]xattr should be set in inode_operations,
e.g ext4, jffs2),it's better to use generic_setxattr. I'm not sure
whether we should convert ubifs to use the xattr handler infrastructure,
like what Andreas said.
Okey, I think I got your meaning now. You are proposing something like
jffs2. Let me try to make our discussion more clear. There are some
proposals here.
(1). What you did in this patch, get the handler for acl and call
handler->get() in ubifs_getxattr(). I disagree with this idea.
(2). What you suggest above to implement all handlers and assign
inode_ops->get_xattr with generic_getxattr().I think there would be a
lot of duplication in this implementation. E.g, Why we need a
check_namespace() in ubifs? That would solve the duplicated checking.
So we need much more code to implement this idea and these code are
almost same.
(3). What you did in V1. Use xattr handler for necessary, currently
only for acl handlers. Same with what Btrfs did.
Hi Sheng, I vote (3).
Thanx
Yang
thanks,
Sheng
Yang
thanks,
Sheng
return ubifs_do_setxattr(d_inode(dentry), name, value, size, flags);
}
@@ -428,6 +567,9 @@ out_unlock:
ssize_t ubifs_getxattr(struct dentry *dentry, const char *name,
void *value, size_t size)
{
+#ifdef CONFIG_UBIFS_FS_POSIX_ACL
+ const struct xattr_handler *handler;
+#endif
struct qstr nm = QSTR_INIT(name, strlen(name));
int type;
@@ -438,6 +580,16 @@ ssize_t ubifs_getxattr(struct dentry *dentry, const char *name,
if (type < 0)
return type;
+#ifdef CONFIG_UBIFS_FS_POSIX_ACL
+ if (type == POSIX_ACL_DEFAULT || type == POSIX_ACL_ACCESS) {
+ if (type == POSIX_ACL_DEFAULT)
+ handler = &posix_acl_default_xattr_handler;
+ if (type == POSIX_ACL_ACCESS)
+ handler = &posix_acl_access_xattr_handler;
+ return handler->get(dentry, name, value, size,
+ handler->flags);
+ }
+#endif
Ditto
Thanx
Yang
return ubifs_do_getxattr(d_inode(dentry), name, value, size);
}
@@ -505,95 +657,6 @@ ssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t size)
return written;
}
-static int remove_xattr(struct ubifs_info *c, struct inode *host,
- struct inode *inode, const struct qstr *nm)
-{
- int err;
- struct ubifs_inode *host_ui = ubifs_inode(host);
- struct ubifs_inode *ui = ubifs_inode(inode);
- struct ubifs_budget_req req = { .dirtied_ino = 2, .mod_dent = 1,
- .dirtied_ino_d = ALIGN(host_ui->data_len, 8) };
-
- ubifs_assert(ui->data_len == inode->i_size);
-
- err = ubifs_budget_space(c, &req);
- if (err)
- return err;
-
- mutex_lock(&host_ui->ui_mutex);
- host->i_ctime = ubifs_current_time(host);
- host_ui->xattr_cnt -= 1;
- host_ui->xattr_size -= CALC_DENT_SIZE(nm->len);
- host_ui->xattr_size -= CALC_XATTR_BYTES(ui->data_len);
- host_ui->xattr_names -= nm->len;
-
- err = ubifs_jnl_delete_xattr(c, host, inode, nm);
- if (err)
- goto out_cancel;
- mutex_unlock(&host_ui->ui_mutex);
-
- ubifs_release_budget(c, &req);
- return 0;
-
-out_cancel:
- host_ui->xattr_cnt += 1;
- host_ui->xattr_size += CALC_DENT_SIZE(nm->len);
- host_ui->xattr_size += CALC_XATTR_BYTES(ui->data_len);
- mutex_unlock(&host_ui->ui_mutex);
- ubifs_release_budget(c, &req);
- make_bad_inode(inode);
- return err;
-}
-
-int ubifs_removexattr(struct dentry *dentry, const char *name)
-{
- struct inode *inode, *host = d_inode(dentry);
- struct ubifs_info *c = host->i_sb->s_fs_info;
- struct qstr nm = QSTR_INIT(name, strlen(name));
- struct ubifs_dent_node *xent;
- union ubifs_key key;
- int err;
-
- dbg_gen("xattr '%s', ino %lu ('%pd')", name,
- host->i_ino, dentry);
- ubifs_assert(mutex_is_locked(&host->i_mutex));
-
- err = check_namespace(&nm);
- if (err < 0)
- return err;
-
- xent = kmalloc(UBIFS_MAX_XENT_NODE_SZ, GFP_NOFS);
- if (!xent)
- return -ENOMEM;
-
- xent_key_init(c, &key, host->i_ino, &nm);
- err = ubifs_tnc_lookup_nm(c, &key, xent, &nm);
- if (err) {
- if (err == -ENOENT)
- err = -ENODATA;
- goto out_free;
- }
-
- inode = iget_xattr(c, le64_to_cpu(xent->inum));
- if (IS_ERR(inode)) {
- err = PTR_ERR(inode);
- goto out_free;
- }
-
- ubifs_assert(inode->i_nlink == 1);
- clear_nlink(inode);
- err = remove_xattr(c, host, inode, &nm);
- if (err)
- set_nlink(inode, 1);
-
- /* If @i_nlink is 0, 'iput()' will delete the inode */
- iput(inode);
-
-out_free:
- kfree(xent);
- return err;
-}
-
static size_t security_listxattr(struct dentry *d, char *list, size_t list_size,
const char *name, size_t name_len, int flags)
{
.
.
.
.
--
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
