Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
From: Mark Salyzyn <salyzyn@android.com>
To: James Morris <jmorris@namei.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Latchesar Ionkov" <lucho@ionkov.net>,
	"Dave Kleikamp" <shaggy@kernel.org>,
	jfs-discussion@lists.sourceforge.net,
	linux-integrity@vger.kernel.org,
	"Martin Brandenburg" <martin@omnibond.com>,
	samba-technical@lists.samba.org,
	"Dominique Martinet" <asmadeus@codewreck.org>,
	"Chao Yu" <yuchao0@huawei.com>,
	"Mimi Zohar" <zohar@linux.ibm.com>,
	linux-unionfs@vger.kernel.org,
	"David Howells" <dhowells@redhat.com>, "Chris Mason" <clm@fb.com>,
	"David S. Miller" <davem@davemloft.net>,
	"Andreas Dilger" <adilger.kernel@dilger.ca>,
	"Eric Paris" <eparis@parisplace.org>,
	netdev@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net,
	linux-afs@lists.infradead.org,
	"Mike Marshall" <hubcap@omnibond.com>,
	linux-xfs@vger.kernel.org,
	"Andreas Gruenbacher" <agruenba@redhat.com>,
	"Sage Weil" <sage@redhat.com>,
	"Miklos Szeredi" <miklos@szeredi.hu>,
	"Richard Weinberger" <richard@nod.at>,
	"Mark Fasheh" <mark@fasheh.com>,
	"Hugh Dickins" <hughd@google.com>,
	"Ernesto A. Fernández" <ernesto.mnd.fernandez@gmail.com>,
	cluster-devel@redhat.com, selinux@vger.kernel.org,
	"Vyacheslav Dubeyko" <slava@dubeyko.com>,
	"Casey Schaufler" <casey@schaufler-ca.com>,
	v9fs-developer@lists.sourceforge.net,
	"Ilya Dryomov" <idryomov@gmail.com>,
	linux-ext4@vger.kernel.org, kernel-team@android.com,
	linux-mm@kvack.org, devel@lists.orangefs.org,
	"Serge Hallyn" <serge@hallyn.com>,
	linux-cifs@vger.kernel.org,
	"Eric Van Hensbergen" <ericvh@gmail.com>,
	ecryptfs@vger.kernel.org, "Josef Bacik" <josef@toxicpanda.com>,
	reiserfs-devel@vger.kernel.org, "Tejun Heo" <tj@kernel.org>,
	"Joel Becker" <jlbec@evilplan.org>,
	linux-mtd@lists.infradead.org, "David Sterba" <dsterba@suse.com>,
	"Jaegeuk Kim" <jaegeuk@kernel.org>,
	ceph-devel@vger.kernel.org,
	"Trond Myklebust" <trond.myklebust@hammerspace.com>,
	"Paul Moore" <paul@paul-moore.com>,
	linux-nfs@vger.kernel.org, "Theodore Ts\'o" <tytso@mit.edu>,
	linux-fsdevel@vger.kernel.org,
	"Joseph Qi" <joseph.qi@linux.alibaba.com>,
	"Mathieu Malaterre" <malat@debian.org>,
	"Stephen Smalley" <sds@tycho.nsa.gov>,
	"Darrick J. Wong" <darrick.wong@oracle.com>,
	"Jeff Layton" <jlayton@kernel.org>,
	linux-kernel@vger.kernel.org, stable@vger.kernel.org,
	"Tyler Hicks" <tyhicks@canonical.com>,
	"Steve French" <sfrench@samba.org>,
	linux-security-module@vger.kernel.org,
	ocfs2-devel@oss.oracle.com, "Jan Kara" <jack@suse.com>,
	"Bob Peterson" <rpeterso@redhat.com>,
	"Phillip Lougher" <phillip@squashfs.org.uk>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	"David Woodhouse" <dwmw2@infradead.org>,
	"Anna Schumaker" <anna.schumaker@netapp.com>,
	linux-btrfs@vger.kernel.org,
	"Alexander Viro" <viro@zeniv.linux.org.uk>
Subject: Re: [PATCH] Add flags option to get xattr method paired to __vfs_getxattr
Date: Thu, 15 Aug 2019 14:26:46 -0700
Message-ID: <69889dec-5440-1472-ed57-380f45547581@android.com> (raw)
In-Reply-To: <alpine.LRH.2.21.1908160515130.12729@namei.org>

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




______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

  parent reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-12 19:32 Mark Salyzyn
2019-08-13  8:28 ` kbuild test robot
2019-08-13  8:48 ` Greg Kroah-Hartman
2019-08-13 14:37   ` Mark Salyzyn
2019-08-15 19:20   ` James Morris
2019-08-15 20:43     ` Greg Kroah-Hartman
2019-08-15 21:26     ` Mark Salyzyn [this message]
2019-08-15 22:27       ` James Morris
2019-08-16 15:30         ` Mark Salyzyn

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=69889dec-5440-1472-ed57-380f45547581@android.com \
    --to=salyzyn@android.com \
    --cc=adilger.kernel@dilger.ca \
    --cc=agruenba@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=anna.schumaker@netapp.com \
    --cc=asmadeus@codewreck.org \
    --cc=casey@schaufler-ca.com \
    --cc=ceph-devel@vger.kernel.org \
    --cc=clm@fb.com \
    --cc=cluster-devel@redhat.com \
    --cc=darrick.wong@oracle.com \
    --cc=davem@davemloft.net \
    --cc=devel@lists.orangefs.org \
    --cc=dhowells@redhat.com \
    --cc=dsterba@suse.com \
    --cc=dwmw2@infradead.org \
    --cc=ecryptfs@vger.kernel.org \
    --cc=eparis@parisplace.org \
    --cc=ericvh@gmail.com \
    --cc=ernesto.mnd.fernandez@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hubcap@omnibond.com \
    --cc=hughd@google.com \
    --cc=idryomov@gmail.com \
    --cc=jack@suse.com \
    --cc=jaegeuk@kernel.org \
    --cc=jfs-discussion@lists.sourceforge.net \
    --cc=jlayton@kernel.org \
    --cc=jlbec@evilplan.org \
    --cc=jmorris@namei.org \
    --cc=josef@toxicpanda.com \
    --cc=joseph.qi@linux.alibaba.com \
    --cc=kernel-team@android.com \
    --cc=linux-afs@lists.infradead.org \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=linux-cifs@vger.kernel.org \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-f2fs-devel@lists.sourceforge.net \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=linux-unionfs@vger.kernel.org \
    --cc=linux-xfs@vger.kernel.org \
    --cc=lucho@ionkov.net \
    --cc=malat@debian.org \
    --cc=mark@fasheh.com \
    --cc=martin@omnibond.com \
    --cc=miklos@szeredi.hu \
    --cc=netdev@vger.kernel.org \
    --cc=ocfs2-devel@oss.oracle.com \
    --cc=paul@paul-moore.com \
    --cc=phillip@squashfs.org.uk \
    --cc=reiserfs-devel@vger.kernel.org \
    --cc=richard@nod.at \
    --cc=rpeterso@redhat.com \
    --cc=sage@redhat.com \
    --cc=samba-technical@lists.samba.org \
    --cc=sds@tycho.nsa.gov \
    --cc=selinux@vger.kernel.org \
    --cc=serge@hallyn.com \
    --cc=sfrench@samba.org \
    --cc=shaggy@kernel.org \
    --cc=slava@dubeyko.com \
    --cc=stable@vger.kernel.org \
    --cc=tj@kernel.org \
    --cc=trond.myklebust@hammerspace.com \
    --cc=tyhicks@canonical.com \
    --cc=tytso@mit.edu \
    --cc=v9fs-developer@lists.sourceforge.net \
    --cc=viro@zeniv.linux.org.uk \
    --cc=yuchao0@huawei.com \
    --cc=zohar@linux.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org linux-mtd@archiver.kernel.org
	public-inbox-index linux-mtd


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


AGPL code for this site: git clone https://public-inbox.org/ public-inbox