Re: [PATCH] Add flags option to get xattr method paired to __vfs_getxattr

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 8/15/19 12:20 PM, James Morris wrote:
On Tue, 13 Aug 2019, Greg Kroah-Hartman wrote:

On Mon, Aug 12, 2019 at 12:32:49PM -0700, Mark Salyzyn wrote:
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -30,10 +30,10 @@ struct xattr_handler {
  	const char *prefix;
  	int flags;      /* fs private flags */
  	bool (*list)(struct dentry *dentry);
-	int (*get)(const struct xattr_handler *, struct dentry *dentry,
+	int (*get)(const struct xattr_handler *handler, struct dentry *dentry,
  		   struct inode *inode, const char *name, void *buffer,
-		   size_t size);
-	int (*set)(const struct xattr_handler *, struct dentry *dentry,
+		   size_t size, int flags);
+	int (*set)(const struct xattr_handler *handler, struct dentry *dentry,
  		   struct inode *inode, const char *name, const void *buffer,
  		   size_t size, int flags);
Wow, 7 arguments.  Isn't there some nice rule of thumb that says once
you get more then 5, a function becomes impossible to understand?

Surely this could be a structure passed in here somehow, that way when
you add the 8th argument in the future, you don't have to change
everything yet again?  :)

I don't have anything concrete to offer as a replacement fix for this,
but to me this just feels really wrong...
How about something like:

struct xattr_gs_args {
	struct dentry *dentry;
	struct inode *inode;
	const char *name;
	const void *buffer;
	size_t size;
	int flags;
};

int (*get)(const struct xattr_handler *handler, struct xattr_gs_args *args);
int (*set)(const struct xattr_handler *handler, struct xattr_gs_args *args);

Good Idea, but using the same argument structure for set and get I would be concerned about the loss of compiler protection for the buffer argument; it is void* for get, and const void* for set. And if we pulled out buffer (and size since it is paired with it) from the structure to solve, the 'mixed' argument approach (resulting in 4 args) adds to the difficulty/complexity.

Good news is the same structure(s) can get passed to __vfs_getxattr and __vfs_setxattr, so one less issue with getting the argument order correct from the caller.

From an optimization standpoint, passing an argument to a pointer to a structure assembled on the stack constrains the compiler. Whereas individual arguments allow for the optimization to place all the arguments into registers. All modern processors have no issue with tens of arguments.

So, I will look into what the patch set will look like by splitting into set and get, and trying to reuse the structure down the call chain.

struct getxattr_args {
	struct dentry *dentry;
	struct inode *inode;
	const char *name;
	void *buffer;
	size_t size;
	int flags;
};

struct setxattr_args {
	struct dentry *dentry;
	struct inode *inode;
	const char *name;
	const void *buffer;
	size_t size;
	int flags;
};

-- Mark







[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux