Re: [PATCH 15/25] autofs: move ioctl32 to autofs{,4}/root.c

[Ian Kent]

On Sat, 5 Nov 2005, Arnd Bergmann wrote:

> All autofs ioctl calls are simple to convert for 32 bit
> compatibility. This just moves the handler into the file
> system code.

I have a couple of concerns with these patches.

I'm not sure if I like conditional compilation in the code proper but I'll 
leave it to you to make the final decision since your running with the 
change. Is there a reason the definitions can't simply be left in place?

Its been a while since I trawled through the compat ioctl code (please 
point me to the right place) but with this change I think that the 
AUTOFS_IOC_SETTIMEOUT32 is redundant. Consider a conditional define for 
AUTOFS_IOC_SETTIMEOUT in include/linux/auto_fs.h instead. Both autofs and 
autofs4 use that definition.

The lock_kernel()/unlock_kernel() in the autofs4 patch is ineffective as 
the BKL is not used for syncronisation anywhere else in autofs4. If 
removing it causes problems I need to know about'em so I can fix'em 
(hopefully).

Can't see any other obvious issues.

Ian

> 
> CC: hpa@xxxxxxxxx
  CC: raven@xxxxxxxxxx ... also please
> CC: autofs@xxxxxxxxxxxxxxxx
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> 
> Index: linux-cg/fs/autofs/root.c
> ===================================================================
> --- linux-cg.orig/fs/autofs/root.c	2005-11-05 15:47:38.000000000 +0100
> +++ linux-cg/fs/autofs/root.c	2005-11-05 15:48:02.000000000 +0100
> @@ -10,6 +10,8 @@
>   *
>   * ------------------------------------------------------------------------- */
>  
> +#include <linux/config.h>
> +#include <linux/compat.h>
>  #include <linux/errno.h>
>  #include <linux/stat.h>
>  #include <linux/param.h>
> @@ -24,11 +26,15 @@
>  static int autofs_root_rmdir(struct inode *,struct dentry *);
>  static int autofs_root_mkdir(struct inode *,struct dentry *,int);
>  static int autofs_root_ioctl(struct inode *, struct file *,unsigned int,unsigned long);
> +static long autofs_root_compat_ioctl(struct file *,unsigned int,unsigned long);
>  
>  struct file_operations autofs_root_operations = {
>  	.read		= generic_read_dir,
>  	.readdir	= autofs_root_readdir,
>  	.ioctl		= autofs_root_ioctl,
> +#ifdef CONFIG_COMPAT
> +	.compat_ioctl	= autofs_root_compat_ioctl,
> +#endif
>  };
>  
>  struct inode_operations autofs_root_inode_operations = {
> @@ -562,3 +568,32 @@
>  		return -ENOSYS;
>  	}
>  }
> +
> +#ifdef CONFIG_COMPAT
> +/* AUTOFS */
> +#define AUTOFS_IOC_SETTIMEOUT32 _IOWR(0x93,0x64,unsigned int)
> +
> +static long autofs_root_compat_ioctl(struct file *file, unsigned int cmd,
> +				unsigned long arg)
> +{
> +	int ret;
> +
> +	switch (cmd) {
> +	case AUTOFS_IOC_SETTIMEOUT32:
> +		cmd = AUTOFS_IOC_SETTIMEOUT;
> +	case AUTOFS_IOC_CATATONIC:
> +	case AUTOFS_IOC_PROTOVER:
> +	case AUTOFS_IOC_EXPIRE:
> +		arg = (unsigned long) compat_ptr(arg);
> +	case AUTOFS_IOC_READY:
> +	case AUTOFS_IOC_FAIL:
> +		lock_kernel();
> +		ret = autofs_root_ioctl(file->f_dentry->d_inode, file, cmd, arg);
> +		unlock_kernel();
> +		break;
> +	default:
> +		ret = -ENOIOCTLCMD;
> +	}
> +	return ret;
> +}
> +#endif
> Index: linux-cg/fs/autofs4/root.c
> ===================================================================
> --- linux-cg.orig/fs/autofs4/root.c	2005-11-05 15:47:38.000000000 +0100
> +++ linux-cg/fs/autofs4/root.c	2005-11-05 16:00:48.000000000 +0100
> @@ -12,6 +12,8 @@
>   *
>   * ------------------------------------------------------------------------- */
>  
> +#include <linux/config.h>
> +#include <linux/compat.h>
>  #include <linux/errno.h>
>  #include <linux/stat.h>
>  #include <linux/param.h>
> @@ -24,6 +26,7 @@
>  static int autofs4_dir_rmdir(struct inode *,struct dentry *);
>  static int autofs4_dir_mkdir(struct inode *,struct dentry *,int);
>  static int autofs4_root_ioctl(struct inode *, struct file *,unsigned int,unsigned long);
> +static long autofs4_root_compat_ioctl(struct file *,unsigned int,unsigned long);
>  static int autofs4_dir_open(struct inode *inode, struct file *file);
>  static int autofs4_dir_close(struct inode *inode, struct file *file);
>  static int autofs4_dir_readdir(struct file * filp, void * dirent, filldir_t filldir);
> @@ -37,6 +40,9 @@
>  	.read		= generic_read_dir,
>  	.readdir	= autofs4_root_readdir,
>  	.ioctl		= autofs4_root_ioctl,
> +#ifdef CONFIG_COMPAT
> +	.compat_ioctl	= autofs4_root_compat_ioctl,
> +#endif
>  };
>  
>  struct file_operations autofs4_dir_operations = {
> @@ -817,3 +823,38 @@
>  		return -ENOSYS;
>  	}
>  }
> +
> +#ifdef CONFIG_COMPAT
> +/* AUTOFS */
> +#define AUTOFS_IOC_SETTIMEOUT32 _IOWR(0x93,0x64,unsigned int)
> +
> +static long autofs4_root_compat_ioctl(struct file *file, unsigned int cmd,
> +				unsigned long arg)
> +{
> +	int ret;
> +
> +	switch (cmd) {
> +	case AUTOFS_IOC_SETTIMEOUT32:
> +		cmd = AUTOFS_IOC_SETTIMEOUT;
> +	case AUTOFS_IOC_CATATONIC:
> +	case AUTOFS_IOC_PROTOVER:
> +	case AUTOFS_IOC_EXPIRE:
> +	case AUTOFS_IOC_EXPIRE_MULTI:
> +	case AUTOFS_IOC_PROTOSUBVER:
> +	case AUTOFS_IOC_ASKREGHOST:
> +	case AUTOFS_IOC_TOGGLEREGHOST:
> +	case AUTOFS_IOC_ASKUMOUNT:
> +		arg = (unsigned long) compat_ptr(arg);
> +	case AUTOFS_IOC_READY:
> +	case AUTOFS_IOC_FAIL:
> +		lock_kernel();
> +		ret = autofs4_root_ioctl(file->f_dentry->d_inode,
> +					 file, cmd, arg);
> +		unlock_kernel();
> +		break;
> +	default:
> +		ret = -ENOIOCTLCMD;
> +	}
> +	return ret;
> +}
> +#endif
> Index: linux-cg/fs/compat_ioctl.c
> ===================================================================
> --- linux-cg.orig/fs/compat_ioctl.c	2005-11-05 15:48:01.000000000 +0100
> +++ linux-cg/fs/compat_ioctl.c	2005-11-05 16:44:07.000000000 +0100
> @@ -337,11 +337,6 @@
>  	return -EINVAL;
>  }
>  
> -static int ioc_settimeout(unsigned int fd, unsigned int cmd, unsigned long arg)
> -{
> -	return rw_long(fd, AUTOFS_IOC_SETTIMEOUT, arg);
> -}
> -
>  /* Bluetooth ioctls */
>  #define HCIUARTSETPROTO	_IOW('U', 200, int)
>  #define HCIUARTGETPROTO	_IOR('U', 201, int)
> @@ -918,8 +913,6 @@
>  HANDLE_IOCTL(MTIOCPOS32, mt_ioctl_trans)
>  HANDLE_IOCTL(CDROMREADAUDIO, cdrom_ioctl_trans)
>  HANDLE_IOCTL(CDROM_SEND_PACKET, cdrom_ioctl_trans)
> -#define AUTOFS_IOC_SETTIMEOUT32 _IOWR(0x93,0x64,unsigned int)
> -HANDLE_IOCTL(AUTOFS_IOC_SETTIMEOUT32, ioc_settimeout)
>  /* vfat */
>  HANDLE_IOCTL(VFAT_IOCTL_READDIR_BOTH32, vfat_ioctl32)
>  HANDLE_IOCTL(VFAT_IOCTL_READDIR_SHORT32, vfat_ioctl32)
> Index: linux-cg/include/linux/compat_ioctl.h
> ===================================================================
> --- linux-cg.orig/include/linux/compat_ioctl.h	2005-11-05 15:48:01.000000000 +0100
> +++ linux-cg/include/linux/compat_ioctl.h	2005-11-05 16:44:07.000000000 +0100
> @@ -360,17 +360,6 @@
>  COMPATIBLE_IOCTL(SOUND_MIXER_GETLEVELS)
>  COMPATIBLE_IOCTL(SOUND_MIXER_SETLEVELS)
>  COMPATIBLE_IOCTL(OSS_GETVERSION)
> -/* AUTOFS */
> -ULONG_IOCTL(AUTOFS_IOC_READY)
> -ULONG_IOCTL(AUTOFS_IOC_FAIL)
> -COMPATIBLE_IOCTL(AUTOFS_IOC_CATATONIC)
> -COMPATIBLE_IOCTL(AUTOFS_IOC_PROTOVER)
> -COMPATIBLE_IOCTL(AUTOFS_IOC_EXPIRE)
> -COMPATIBLE_IOCTL(AUTOFS_IOC_EXPIRE_MULTI)
> -COMPATIBLE_IOCTL(AUTOFS_IOC_PROTOSUBVER)
> -COMPATIBLE_IOCTL(AUTOFS_IOC_ASKREGHOST)
> -COMPATIBLE_IOCTL(AUTOFS_IOC_TOGGLEREGHOST)
> -COMPATIBLE_IOCTL(AUTOFS_IOC_ASKUMOUNT)
>  /* DEVFS */
>  COMPATIBLE_IOCTL(DEVFSDIOC_GET_PROTO_REV)
>  COMPATIBLE_IOCTL(DEVFSDIOC_SET_EVENT_MASK)
> 
> --
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

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