Re: [PATCH 04/24] Unionfs: Common file operations

[Andrew Morton <akpm@osdl.org>]

On Sun,  7 Jan 2007 23:12:56 -0500
"Josef 'Jeff' Sipek" <jsipek@cs.sunysb.edu> wrote:

> From: Josef "Jeff" Sipek <jsipek@cs.sunysb.edu>
> 
> This patch contains helper functions used through the rest of the code which
> pertains to files.
> 
> Signed-off-by: Josef "Jeff" Sipek <jsipek@cs.sunysb.edu>
> Signed-off-by: David Quigley <dquigley@fsl.cs.sunysb.edu>
> Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
>
> ...
>
> +#include "union.h"
> +
> +/* 1) Copyup the file
> + * 2) Rename the file to '.unionfs<original inode#><counter>' - obviously
> + * stolen from NFS's silly rename
> + */
> +static int copyup_deleted_file(struct file *file, struct dentry *dentry,
> +			       int bstart, int bindex)
> +{
> +	static unsigned int counter;
> +	const int i_inosize = sizeof(dentry->d_inode->i_ino) * 2;
> +	const int countersize = sizeof(counter) * 2;
> +	const int nlen = sizeof(".unionfs") + i_inosize + countersize - 1;
> +	char name[nlen + 1];
> +
> +	int err;
> +	struct dentry *tmp_dentry = NULL;
> +	struct dentry *hidden_dentry = NULL;
> +	struct dentry *hidden_dir_dentry = NULL;
> +
> +	hidden_dentry = unionfs_lower_dentry_idx(dentry, bstart);

Slightly strange to initialise a variable twice in a row like this.

> +	sprintf(name, ".unionfs%*.*lx",
> +			i_inosize, i_inosize, hidden_dentry->d_inode->i_ino);
> +
> +	tmp_dentry = NULL;
> +	do {
> +		char *suffix = name + nlen - countersize;
> +
> +		dput(tmp_dentry);
> +		counter++;
> +		sprintf(suffix, "%*.*x", countersize, countersize, counter);
> +
> +		printk(KERN_DEBUG "unionfs: trying to rename %s to %s\n",
> +				dentry->d_name.name, name);
> +
> +		tmp_dentry = lookup_one_len(name, hidden_dentry->d_parent,
> +					    UNIONFS_TMPNAM_LEN);
> +		if (IS_ERR(tmp_dentry)) {
> +			err = PTR_ERR(tmp_dentry);
> +			goto out;
> +		}
> +	} while (tmp_dentry->d_inode != NULL);	/* need negative dentry */
> +
> +	err = copyup_named_file(dentry->d_parent->d_inode, file, name, bstart,
> +				bindex, file->f_dentry->d_inode->i_size);
> +	if (err)
> +		goto out;
> +
> +	/* bring it to the same state as an unlinked file */
> +	hidden_dentry = unionfs_lower_dentry_idx(dentry, dbstart(dentry));
> +	hidden_dir_dentry = lock_parent(hidden_dentry);
> +	err = vfs_unlink(hidden_dir_dentry->d_inode, hidden_dentry);
> +	unlock_dir(hidden_dir_dentry);
> +
> +out:
> +	return err;
> +}
> +
> +/* put all references held by upper struct file and free lower file pointer
> + * array
> + */

Where in this patchset would the reader go to understand what an "upper
file" is, what a "lower file" is?  The relationship between them, lifecycle
management, locking, etc?

<looks at the data structures in union.h>

That's the place.  It would be useful to describe things in there a lot
better.  For example, bstart, bend and bindev could do with help.

unionfs_get_nlinks() is huge.  it has twelve callsites and two out-of-line
copies are generated.  Suggest that it not be inlined ;)

alloc_whname() should be out-of-line.

lock_parent() and unlock_dir() are poorly named.

> +long unionfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +	long err;
> +
> +	if ((err = unionfs_file_revalidate(file, 1)))
> +		goto out;
> +
> +	/* check if asked for local commands */
> +	switch (cmd) {
> +		case UNIONFS_IOCTL_INCGEN:
> +			/* Increment the superblock generation count */
> +			err = -EACCES;
> +			if (!capable(CAP_SYS_ADMIN))
> +				goto out;
> +			err = unionfs_ioctl_incgen(file, cmd, arg);
> +			break;
> +
> +		case UNIONFS_IOCTL_QUERYFILE:
> +			/* Return list of branches containing the given file */
> +			err = unionfs_ioctl_queryfile(file, cmd, arg);
> +			break;
> +
> +		default:
> +			/* pass the ioctl down */
> +			err = do_ioctl(file, cmd, arg);
> +			break;
> +	}

We normally use one tabstop less than this.