Re: [PATCH 04/16 v4] pramfs: file operations

[Paul Mundt <lethal@linux-sh.org>]

On Sat, Nov 20, 2010 at 10:58:40AM +0100, Marco Stornelli wrote:
> diff -Nurp linux-2.6.36-orig/fs/pramfs/file.c linux-2.6.36/fs/pramfs/file.c
> --- linux-2.6.36-orig/fs/pramfs/file.c	1970-01-01 01:00:00.000000000 +0100
> +++ linux-2.6.36/fs/pramfs/file.c	2010-09-24 18:34:03.000000000 +0200
> @@ -0,0 +1,166 @@
> +/*
> + * FILE NAME fs/pramfs/file.c
> + *
> + * BRIEF DESCRIPTION
> + *
> + * File operations for files.
> + *
This FILE NAME / BRIEF DESCRIPTION thing should probably die, it's also
not used consistently throughout the series.

> +static int pram_open_file(struct inode *inode, struct file *filp)
> +{
> +#ifndef CONFIG_PRAMFS_XIP
> +	/* Without XIP we force to use Direct IO */
> +	filp->f_flags |= O_DIRECT;
> +#endif
> +	return generic_file_open(inode, filp);
> +}
> +
Relying on a config option for this is in violently poor taste. Rely on
the config option to enable/disable XIP support as you like, but whether
it's actually mounted XIP or not should really depend be determined by a
mount option. Presently you have no way of dealing with the file system
being mounted multiple times with mixed XIP and non-XIP, which doesn't
seem like a particularly exotic configuration. As you seem to have copied
most of this from ext2, I'm curious why you opted to hardcode this
instead of maintaining the flexibility that ext2 XIP has over this.