Re: [PATCH 16/35] Unionfs: Copyup Functionality

[Jan Engelhardt <jengelh@linux01.gwdg.de>]

On Dec 4 2006 07:30, Josef 'Jeff' Sipek wrote:
>+/* Determine the mode based on the copyup flags, and the existing dentry. */
>+static int copyup_permissions(struct super_block *sb,
>+			      struct dentry *old_hidden_dentry,
>+			      struct dentry *new_hidden_dentry)
>+{
>+	struct inode *i = old_hidden_dentry->d_inode;

Screams for constification. (Or rather I do.)

>+{
>+	int err = 0;
>+	umode_t old_mode = old_hidden_dentry->d_inode->i_mode;

Generel question for everybody: Why do we have two same (at least on i386
that's the case) types, umode_t and mode_t?

>+	} else if (S_ISBLK(old_mode)
>+		   || S_ISCHR(old_mode)
>+		   || S_ISFIFO(old_mode)
>+		   || S_ISSOCK(old_mode)) {
>+		args.mknod.parent = new_hidden_parent_dentry->d_inode;
>+		args.mknod.dentry = new_hidden_dentry;
>+		args.mknod.mode = old_mode;

I'd say the indent got screwed up, and it's not a mailer thing.

>+	} else if (S_ISBLK(old_mode)
>+	    || S_ISCHR(old_mode)
>+	    || S_ISFIFO(old_mode)
>+	    || S_ISSOCK(old_mode)) {
>+		args.mknod.parent = new_hidden_parent_dentry->d_inode;
>+		args.mknod.dentry = new_hidden_dentry;
>+		args.mknod.mode = old_mode;

Try this ^^^. Or even this vvv:

>+	} else if (S_ISBLK(old_mode) || S_ISCHR(old_mode) ||
>+	    S_ISFIFO(old_mode) || S_ISSOCK(old_mode)) {
>+		args.mknod.parent = new_hidden_parent_dentry->d_inode;
>+		args.mknod.dentry = new_hidden_dentry;
>+		args.mknod.mode = old_mode;


>+static inline int __copyup_reg_data(struct dentry *dentry,
>+				    struct dentry *new_hidden_dentry,
>+				    int new_bindex,
>+				    struct dentry *old_hidden_dentry,
>+				    int old_bindex,
>+				    struct file **copyup_file,
>+				    loff_t len)
>+{
>+	struct super_block *sb = dentry->d_sb;
>+	struct file *input_file;
>+	struct file *output_file;
>+	mm_segment_t old_fs;
>+	char *buf = NULL;
>+	ssize_t read_bytes, write_bytes;
>+	loff_t size;
>+	int err = 0;
>+
>+	/* open old file */
>+	mntget(unionfs_lower_mnt_idx(dentry, old_bindex));
>+	branchget(sb, old_bindex);
>+	input_file = dentry_open(old_hidden_dentry,
>+			unionfs_lower_mnt_idx(dentry, old_bindex), O_RDONLY | O_LARGEFILE);
>+	if (IS_ERR(input_file)) {
>+		dput(old_hidden_dentry);
>+		err = PTR_ERR(input_file);
>+		goto out;
>+	}
>+	if (!input_file->f_op || !input_file->f_op->read) {
>+		err = -EINVAL;
>+		goto out_close_in;
>+	}
>+
>+	/* open new file */
>+	dget(new_hidden_dentry);
>+	mntget(unionfs_lower_mnt_idx(dentry, new_bindex));
>+	branchget(sb, new_bindex);
>+	output_file = dentry_open(new_hidden_dentry,
>+			unionfs_lower_mnt_idx(dentry, new_bindex), O_WRONLY | O_LARGEFILE);

Here we got an 80-column buster.

>+	if (IS_ERR(output_file)) {
>+		err = PTR_ERR(output_file);
>+		goto out_close_in2;
>+	}
>+	if (!output_file->f_op || !output_file->f_op->write) {
>+		err = -EINVAL;
>+		goto out_close_out;
>+	}
>+
>+	/* allocating a buffer */
>+	buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>+	if (!buf) {
>+		err = -ENOMEM;
>+		goto out_close_out;
>+	}
>+
>+	input_file->f_pos = 0;
>+	output_file->f_pos = 0;
>+
>+	old_fs = get_fs();
>+	set_fs(KERNEL_DS);
>+
>+	size = len;
>+	err = 0;
>+	do {
>+		if (len >= PAGE_SIZE)
>+			size = PAGE_SIZE;
>+		else if ((len < PAGE_SIZE) && (len > 0))
>+			size = len;

Some redundant () here.

>+
>+		len -= PAGE_SIZE;
>+
>+		read_bytes =
>+		    input_file->f_op->read(input_file,
>+					   (char __user *)buf, size,
>+					   &input_file->f_pos);
>+		if (read_bytes <= 0) {
>+			err = read_bytes;
>+			break;
>+		}
>+
>+		write_bytes =
>+		    output_file->f_op->write(output_file,
>+					     (char __user *)buf,
>+					     read_bytes,
>+					     &output_file->f_pos);
>+		if (write_bytes < 0 || (write_bytes < read_bytes)) {

dit(t)o

>+			err = write_bytes;
>+			break;
>+		}
>+	} while ((read_bytes > 0) && (len > 0));

~

>+
>+	set_fs(old_fs);
>+
>+	kfree(buf);
>+
>+	if (err)
>+		goto out_close_out;
>+	if (copyup_file) {
>+		*copyup_file = output_file;
>+		goto out_close_in;
>+	}
>+
>+out_close_out:
>+	fput(output_file);
>+
>+out_close_in2:
>+	branchput(sb, new_bindex);
>+
>+out_close_in:
>+	fput(input_file);
>+
>+out:
>+	branchput(sb, old_bindex);
>+
>+	return err;
>+}
>+

>+	/* TODO: should we reset the error to something like -EIO? */

Handle it :)  - if it does not take a paper.




	-`J'
-- 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/