Re: [PATCH 17/25] vfat: move ioctl32 code to fs/fat/dir.c

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

Arnd Bergmann <arnd@arndb.de> writes:

> +#ifdef CONFIG_COMPAT
> +/* vfat */
> +#define	VFAT_IOCTL_READDIR_BOTH32	_IOR('r', 1, struct compat_dirent[2])
> +#define	VFAT_IOCTL_READDIR_SHORT32	_IOR('r', 2, struct compat_dirent[2])
          ^^^^^^

Please use a SPACE after #define, and kill a useless "/* vfat */".

> +static long
> +put_dirent32(struct dirent *d, struct compat_dirent __user * d32)
> +{
> +	if (!access_ok(VERIFY_WRITE, d32, sizeof(struct compat_dirent)))
> +		return -EFAULT;
> +
> +	__put_user(d->d_ino, &d32->d_ino);
> +	__put_user(d->d_off, &d32->d_off);
> +	__put_user(d->d_reclen, &d32->d_reclen);
> +	if (__copy_to_user(d32->d_name, d->d_name, d->d_reclen))
> +		return -EFAULT;

Why don't we need to check the return value of __put_user()?

> +	set_fs(KERNEL_DS);
> +	ret = fat_dir_ioctl(file->f_dentry->d_inode, file, cmd, (unsigned long) &d);
> +	set_fs(oldfs);
> +	if (ret >= 0) {
> +		ret |= put_dirent32(&d[0], p);
> +		ret |= put_dirent32(&d[1], p + 1);

If "ret" is not 0, we can't use "|=" here?

This patch seems to have bugs, although original one too.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>