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 <hub...@omnibond.com>
Date: Wed Apr 6 11:19:37 2016 -0400
Orangefs: optimize boilerplate code.
Suggested by David Binderman <dcb...@hotmail.com>
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 <hub...@omnibond.com>
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 <hub...@omnibond.com>
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 <dcb...@hotmail.com>
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 <hub...@omnibond.com>
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 <dcb...@hotmail.com> 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
Pvfs2-developers@beowulf-underground.org
http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers
