Hello David... I was out of town for a week and am just now getting to looking at last week's emails about the kernel module... I have made these patches, mostly based on your suggestions, and will get them in my next pull request unless someone points out problems with them... Thanks...
commit e15da010f7be010082c6daf7d57dbbfbe0f345da Author: Mike Marshall <[email protected]> Date: Wed Apr 6 11:19:37 2016 -0400 Orangefs: optimize boilerplate code. Suggested by David Binderman <[email protected]> The former can potentially be a performance win over the latter. memcpy(d, s, len); memset(d+len, c, size-len); memset(d, c, size); memcpy(d, s, len); Signed-off-by: Mike Marshall <[email protected]> diff --git a/fs/orangefs/protocol.h b/fs/orangefs/protocol.h index 50578a2..a7f21a3 100644 --- a/fs/orangefs/protocol.h +++ b/fs/orangefs/protocol.h @@ -74,8 +74,8 @@ static inline void ORANGEFS_khandle_to(const struct orangefs_khandle *kh, void *p, int size) { - memset(p, 0, size); memcpy(p, kh->u, 16); + memset(p + 16, 0, size - 16); } diff --git a/fs/orangefs/xattr.c b/fs/orangefs/xattr.c index 90a8ae7..63a6280d 100644 --- a/fs/orangefs/xattr.c +++ b/fs/orangefs/xattr.c @@ -142,8 +142,8 @@ ssize_t orangefs_inode_getxattr(struct inode *inode, const char *prefix, goto out_release_op; } - memset(buffer, 0, size); memcpy(buffer, new_op->downcall.resp.getxattr.val, length); + memset(buffer + length, 0, size - length); gossip_debug(GOSSIP_XATTR_DEBUG, "orangefs_inode_getxattr: inode %pU " "key %s key_sz %d, val_len %d\n", commit a16178e9a9c11582ca511d651faf8e1e7e49df5e Author: Mike Marshall <[email protected]> Date: Wed Apr 6 10:52:38 2016 -0400 Orangefs: xattr.c cleanup 1. It is nonsense to test for negative size_t, suggested by David Binderman <[email protected]> 2. By the time Orangefs gets called, the vfs has ensured that name != NULL, and that buffer and size are sane. Signed-off-by: Mike Marshall <[email protected]> diff --git a/fs/orangefs/xattr.c b/fs/orangefs/xattr.c index ef5da75..90a8ae7 100644 --- a/fs/orangefs/xattr.c +++ b/fs/orangefs/xattr.c @@ -73,10 +73,6 @@ ssize_t orangefs_inode_getxattr(struct inode *inode, const char *prefix, "%s: prefix %s name %s, buffer_size %zd\n", __func__, prefix, name, size); - if (name == NULL || (size > 0 && buffer == NULL)) { - gossip_err("orangefs_inode_getxattr: bogus NULL pointers\n"); - return -EINVAL; - } if ((strlen(name) + strlen(prefix)) >= ORANGEFS_MAX_XATTR_NAMELEN) { gossip_err("Invalid key length (%d)\n", (int)(strlen(name) + strlen(prefix))); @@ -239,8 +235,7 @@ int orangefs_inode_setxattr(struct inode *inode, const char *prefix, "%s: prefix %s, name %s, buffer_size %zd\n", __func__, prefix, name, size); - if (size < 0 || - size >= ORANGEFS_MAX_XATTR_VALUELEN || + if (size >= ORANGEFS_MAX_XATTR_VALUELEN || flags < 0) { gossip_err("orangefs_inode_setxattr: bogus values of size(%d), flags(%d)\n", (int)size, @@ -248,12 +243,6 @@ int orangefs_inode_setxattr(struct inode *inode, const char *prefix, return -EINVAL; } - if (name == NULL || - (size > 0 && value == NULL)) { - gossip_err("orangefs_inode_setxattr: bogus NULL pointers!\n"); - return -EINVAL; - } - internal_flag = convert_to_internal_xattr_flags(flags); if (prefix) { @@ -353,10 +342,6 @@ ssize_t orangefs_listxattr(struct dentry *dentry, char *buffer, size_t size) gossip_err("%s: bogus NULL pointers\n", __func__); return -EINVAL; } - if (size < 0) { - gossip_err("Invalid size (%d)\n", (int)size); - return -EINVAL; - } down_read(&orangefs_inode->xattr_sem); new_op = op_alloc(ORANGEFS_VFS_OP_LISTXATTR); On Mon, Mar 28, 2016 at 2:08 PM, David Binderman <[email protected]> wrote: > Hello there, > > 1. > > [linux-4.6-rc1/fs/orangefs/xattr.c:242]: (style) Checking if unsigned > variable 'size' is less than zero. > > Source code is > > if (size < 0 || > > but > > size_t size > > 2. > > linux-4.6-rc1/fs/orangefs/xattr.c:356]: (style) Checking if unsigned variable > 'size' is less than zero. > > Duplicate > > BTW, I also saw this: > > [linux-4.6-rc1/fs/orangefs/protocol.h:78]: (performance) Buffer 'p' is being > written before its old content has been used. > > Source code is > > memset(p, 0, size); > memcpy(p, kh->u, 16); > > If performance is an issue, this could be coded up to run faster. Maybe > something like > > memcpy(p, kh->u, 16); > memset( p + 16, 0, size - 16); > > This fourth message > > [linux-4.6-rc1/fs/orangefs/xattr.c:150]: (performance) Buffer 'buffer' is > being written before its old content has been used. > > looks like a duplicate to me. > > Regards > > David Binderman > > _______________________________________________ Pvfs2-developers mailing list [email protected] http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers
