From mboxrd@z Thu Jan 1 00:00:00 1970 From: josh@joshtriplett.org Subject: Re: [PATCH 04/56] fs: Macros to define splice file_operations Date: Thu, 13 Nov 2014 14:24:22 -0800 Message-ID: <20141113222422.GA30412@cloud> References: <20141113215139.GK7996@ZenIV.linux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Pieter Smith , Jeff Layton , "J. Bruce Fields" , linux-fsdevel , open list To: Richard Weinberger , Al Viro Return-path: Content-Disposition: inline In-Reply-To: <20141113215139.GK7996@ZenIV.linux.org.uk> Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Thu, Nov 13, 2014 at 10:49:07PM +0100, Richard Weinberger wrote: > On Thu, Nov 13, 2014 at 10:22 PM, Pieter Smith wrote: > > Provides a CONFIG_SYSCALL_SPLICE compatible way of defining the .splice_read > > and .splice_write file_operations so that they can later be compiled out when > > the kernel is configured without the splice-family syscalls [...] > > --- a/include/linux/fs.h > > +++ b/include/linux/fs.h > > @@ -1512,6 +1512,32 @@ struct file_operations { > > int (*show_fdinfo)(struct seq_file *m, struct file *f); > > }; > > > > +#ifdef CONFIG_SYSCALL_SPLICE > > + > > +/* > > + * Define and init the splice_read member of a file_operations struct > > + */ > > +#define SPLICE_READ_INIT(read) .splice_read = read, > > + > > +/* > > + * Define and init the splice_read member of a file_operations struct > > + */ > > +#define SPLICE_WRITE_INIT(write) .splice_write = write, > > This is ugly like hell. > Why can't you do something like __exit_p()? On Thu, Nov 13, 2014 at 09:51:39PM +0000, Al Viro wrote: > This (and subsequent stuff making use of that) is bloody pointless. You > save 2 words per file_operations instance, at the cost of making things > uglier and harder to grep. NAK. Given the large number of uses of these, I agree that it doesn't seem worth the tradeoff, particularly since very few file_operations structures will exist on any individual tiny configuration. I think we should go with a wrapper similar to __exit_p (splice_p?), which just becomes NULL when !CONFIG_SYSCALL_SPLICE. Removing the actual pointers from file_operations can wait until we have compiler support for tagging specific fields in a structure (like splice_read and splice_write) as dead. Similarly, you shouldn't wrap the functions that get assigned to those pointers with #ifdef; instead, mark them as __maybe_unused, which doesn't even add any lines of code. The compiler will then automatically throw them out when not used, without emiting a warning. That should drastically reduce the number of changes, and in particular eliminate almost all of the ifdefs. - Josh Triplett