Hi Andreas...
I'm sorry I didn't notice this right away. And that I missed the merge window
with your patch <g>... anyhow...
We use the return value in this one line you changed, our userspace code gets
ill when we send it (-ENOMEM +1) as a key length... would you change the
patch one more time, or ack my change please... I'll get these in at the next
merge window...
# git diff
diff --git a/fs/orangefs/xattr.c b/fs/orangefs/xattr.c
index 1165047..bdb407e 100644
--- a/fs/orangefs/xattr.c
+++ b/fs/orangefs/xattr.c
@@ -97,7 +97,11 @@ ssize_t orangefs_inode_getxattr(struct inode *inode, const ch
goto out_unlock;
new_op->upcall.req.getxattr.refn = orangefs_inode->refn;
- strcpy(new_op->upcall.req.getxattr.key, name);
+
+ ret = snprintf((char *)new_op->upcall.req.getxattr.key,
+ ORANGEFS_MAX_XATTR_NAMELEN,
+ "%s",
+ name);
/*
* NOTE: Although keys are meant to be NULL terminated textual
On Mon, May 30, 2016 at 5:26 AM, Andreas Gruenbacher
<agruenba@xxxxxxxxxx> wrote:
> Passing the attribute prefix and name to orangefs_inode_*xattr as
> separate arguments was unnecessary before, but now that the trusted
> xattr handler is gone, the prefix is even always the empty string.
> Remove it.
>
> Signed-off-by: Andreas Gruenbacher <agruenba@xxxxxxxxxx>
> ---
> fs/orangefs/acl.c | 9 ++---
> fs/orangefs/file.c | 2 --
> fs/orangefs/orangefs-kernel.h | 2 --
> fs/orangefs/xattr.c | 81 ++++++++++++++-----------------------------
> 4 files changed, 29 insertions(+), 65 deletions(-)
>
> diff --git a/fs/orangefs/acl.c b/fs/orangefs/acl.c
> index df24864..28f2195 100644
> --- a/fs/orangefs/acl.c
> +++ b/fs/orangefs/acl.c
> @@ -43,11 +43,8 @@ struct posix_acl *orangefs_get_acl(struct inode *inode, int type)
> get_khandle_from_ino(inode),
> key,
> type);
> - ret = orangefs_inode_getxattr(inode,
> - "",
> - key,
> - value,
> - ORANGEFS_MAX_XATTR_VALUELEN);
> + ret = orangefs_inode_getxattr(inode, key, value,
> + ORANGEFS_MAX_XATTR_VALUELEN);
> /* if the key exists, convert it to an in-memory rep */
> if (ret > 0) {
> acl = posix_acl_from_xattr(&init_user_ns, value, ret);
> @@ -131,7 +128,7 @@ int orangefs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> * will xlate to a removexattr. However, we don't want removexattr
> * complain if attributes does not exist.
> */
> - error = orangefs_inode_setxattr(inode, "", name, value, size, 0);
> + error = orangefs_inode_setxattr(inode, name, value, size, 0);
>
> out:
> kfree(value);
> diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c
> index 5160a3f..526040e 100644
> --- a/fs/orangefs/file.c
> +++ b/fs/orangefs/file.c
> @@ -516,7 +516,6 @@ static long orangefs_ioctl(struct file *file, unsigned int cmd, unsigned long ar
> if (cmd == FS_IOC_GETFLAGS) {
> val = 0;
> ret = orangefs_inode_getxattr(file_inode(file),
> - "",
> "user.pvfs2.meta_hint",
> &val, sizeof(val));
> if (ret < 0 && ret != -ENODATA)
> @@ -549,7 +548,6 @@ static long orangefs_ioctl(struct file *file, unsigned int cmd, unsigned long ar
> "orangefs_ioctl: FS_IOC_SETFLAGS: %llu\n",
> (unsigned long long)val);
> ret = orangefs_inode_setxattr(file_inode(file),
> - "",
> "user.pvfs2.meta_hint",
> &val, sizeof(val), 0);
> }
> diff --git a/fs/orangefs/orangefs-kernel.h b/fs/orangefs/orangefs-kernel.h
> index 6503e37..7b542f1 100644
> --- a/fs/orangefs/orangefs-kernel.h
> +++ b/fs/orangefs/orangefs-kernel.h
> @@ -517,13 +517,11 @@ __s32 fsid_of_op(struct orangefs_kernel_op_s *op);
> int orangefs_flush_inode(struct inode *inode);
>
> ssize_t orangefs_inode_getxattr(struct inode *inode,
> - const char *prefix,
> const char *name,
> void *buffer,
> size_t size);
>
> int orangefs_inode_setxattr(struct inode *inode,
> - const char *prefix,
> const char *name,
> const void *value,
> size_t size,
> diff --git a/fs/orangefs/xattr.c b/fs/orangefs/xattr.c
> index 640a98f..1165047 100644
> --- a/fs/orangefs/xattr.c
> +++ b/fs/orangefs/xattr.c
> @@ -59,8 +59,8 @@ static inline int convert_to_internal_xattr_flags(int setxattr_flags)
> * unless the key does not exist for the file and/or if
> * there were errors in fetching the attribute value.
> */
> -ssize_t orangefs_inode_getxattr(struct inode *inode, const char *prefix,
> - const char *name, void *buffer, size_t size)
> +ssize_t orangefs_inode_getxattr(struct inode *inode, const char *name,
> + void *buffer, size_t size)
> {
> struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
> struct orangefs_kernel_op_s *new_op = NULL;
> @@ -70,12 +70,12 @@ ssize_t orangefs_inode_getxattr(struct inode *inode, const char *prefix,
> int fsgid;
>
> gossip_debug(GOSSIP_XATTR_DEBUG,
> - "%s: prefix %s name %s, buffer_size %zd\n",
> - __func__, prefix, name, size);
> + "%s: name %s, buffer_size %zd\n",
> + __func__, name, size);
>
> - if ((strlen(name) + strlen(prefix)) >= ORANGEFS_MAX_XATTR_NAMELEN) {
> + if (strlen(name) >= ORANGEFS_MAX_XATTR_NAMELEN) {
> gossip_err("Invalid key length (%d)\n",
> - (int)(strlen(name) + strlen(prefix)));
> + (int)strlen(name));
> return -EINVAL;
> }
>
> @@ -97,8 +97,7 @@ ssize_t orangefs_inode_getxattr(struct inode *inode, const char *prefix,
> goto out_unlock;
>
> new_op->upcall.req.getxattr.refn = orangefs_inode->refn;
> - ret = snprintf((char *)new_op->upcall.req.getxattr.key,
> - ORANGEFS_MAX_XATTR_NAMELEN, "%s%s", prefix, name);
> + strcpy(new_op->upcall.req.getxattr.key, name);
>
> /*
> * NOTE: Although keys are meant to be NULL terminated textual
> @@ -163,10 +162,8 @@ out_unlock:
> return ret;
> }
>
> -static int orangefs_inode_removexattr(struct inode *inode,
> - const char *prefix,
> - const char *name,
> - int flags)
> +static int orangefs_inode_removexattr(struct inode *inode, const char *name,
> + int flags)
> {
> struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
> struct orangefs_kernel_op_s *new_op = NULL;
> @@ -183,12 +180,8 @@ static int orangefs_inode_removexattr(struct inode *inode,
> * textual strings, I am going to explicitly pass the
> * length just in case we change this later on...
> */
> - ret = snprintf((char *)new_op->upcall.req.removexattr.key,
> - ORANGEFS_MAX_XATTR_NAMELEN,
> - "%s%s",
> - (prefix ? prefix : ""),
> - name);
> - new_op->upcall.req.removexattr.key_sz = ret + 1;
> + strcpy(new_op->upcall.req.removexattr.key, name);
> + new_op->upcall.req.removexattr.key_sz = strlen(name) + 1;
>
> gossip_debug(GOSSIP_XATTR_DEBUG,
> "orangefs_inode_removexattr: key %s, key_sz %d\n",
> @@ -223,8 +216,8 @@ out_unlock:
> * Returns a -ve number on error and 0 on success. Key is text, but value
> * can be binary!
> */
> -int orangefs_inode_setxattr(struct inode *inode, const char *prefix,
> - const char *name, const void *value, size_t size, int flags)
> +int orangefs_inode_setxattr(struct inode *inode, const char *name,
> + const void *value, size_t size, int flags)
> {
> struct orangefs_inode_s *orangefs_inode = ORANGEFS_I(inode);
> struct orangefs_kernel_op_s *new_op;
> @@ -232,8 +225,8 @@ int orangefs_inode_setxattr(struct inode *inode, const char *prefix,
> int ret = -ENOMEM;
>
> gossip_debug(GOSSIP_XATTR_DEBUG,
> - "%s: prefix %s, name %s, buffer_size %zd\n",
> - __func__, prefix, name, size);
> + "%s: name %s, buffer_size %zd\n",
> + __func__, name, size);
>
> if (size >= ORANGEFS_MAX_XATTR_VALUELEN ||
> flags < 0) {
> @@ -245,29 +238,19 @@ int orangefs_inode_setxattr(struct inode *inode, const char *prefix,
>
> internal_flag = convert_to_internal_xattr_flags(flags);
>
> - if (prefix) {
> - if (strlen(name) + strlen(prefix) >= ORANGEFS_MAX_XATTR_NAMELEN) {
> - gossip_err
> - ("orangefs_inode_setxattr: bogus key size (%d)\n",
> - (int)(strlen(name) + strlen(prefix)));
> - return -EINVAL;
> - }
> - } else {
> - if (strlen(name) >= ORANGEFS_MAX_XATTR_NAMELEN) {
> - gossip_err
> - ("orangefs_inode_setxattr: bogus key size (%d)\n",
> - (int)(strlen(name)));
> - return -EINVAL;
> - }
> + if (strlen(name) >= ORANGEFS_MAX_XATTR_NAMELEN) {
> + gossip_err
> + ("orangefs_inode_setxattr: bogus key size (%d)\n",
> + (int)(strlen(name)));
> + return -EINVAL;
> }
>
> /* This is equivalent to a removexattr */
> if (size == 0 && value == NULL) {
> gossip_debug(GOSSIP_XATTR_DEBUG,
> - "removing xattr (%s%s)\n",
> - prefix,
> + "removing xattr (%s)\n",
> name);
> - return orangefs_inode_removexattr(inode, prefix, name, flags);
> + return orangefs_inode_removexattr(inode, name, flags);
> }
>
> gossip_debug(GOSSIP_XATTR_DEBUG,
> @@ -288,11 +271,8 @@ int orangefs_inode_setxattr(struct inode *inode, const char *prefix,
> * strings, I am going to explicitly pass the length just in
> * case we change this later on...
> */
> - ret = snprintf((char *)new_op->upcall.req.setxattr.keyval.key,
> - ORANGEFS_MAX_XATTR_NAMELEN,
> - "%s%s",
> - prefix, name);
> - new_op->upcall.req.setxattr.keyval.key_sz = ret + 1;
> + strcpy(new_op->upcall.req.setxattr.keyval.key, name);
> + new_op->upcall.req.setxattr.keyval.key_sz = strlen(name) + 1;
> memcpy(new_op->upcall.req.setxattr.keyval.val, value, size);
> new_op->upcall.req.setxattr.keyval.val_sz = size;
>
> @@ -455,12 +435,7 @@ static int orangefs_xattr_set_default(const struct xattr_handler *handler,
> size_t size,
> int flags)
> {
> - return orangefs_inode_setxattr(inode,
> - "",
> - name,
> - buffer,
> - size,
> - flags);
> + return orangefs_inode_setxattr(inode, name, buffer, size, flags);
> }
>
> static int orangefs_xattr_get_default(const struct xattr_handler *handler,
> @@ -470,11 +445,7 @@ static int orangefs_xattr_get_default(const struct xattr_handler *handler,
> void *buffer,
> size_t size)
> {
> - return orangefs_inode_getxattr(inode,
> - "",
> - name,
> - buffer,
> - size);
> + return orangefs_inode_getxattr(inode, name, buffer, size);
>
> }
>
> --
> 2.5.5
>
--
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
