[PATCH] Add compat_ioctl to st

[PATCH] Add compat_ioctl to st

[Andi Kleen]

Call new compat_ioctl host vector from tape driver

Signed-off-by: Andi Kleen <ak@muc.de>

diff -u linux-2.6.11-rc1-bk4/drivers/scsi/scsi_ioctl.c-o linux-2.6.11-rc1-bk4/drivers/scsi/scsi_ioctl.c
diff -u linux-2.6.11-rc1-bk4/drivers/scsi/st.c-o linux-2.6.11-rc1-bk4/drivers/scsi/st.c
--- linux-2.6.11-rc1-bk4/drivers/scsi/st.c-o	2005-01-04 12:13:07.000000000 +0100
+++ linux-2.6.11-rc1-bk4/drivers/scsi/st.c	2005-01-18 04:50:09.000000000 +0100
@@ -3425,6 +3425,22 @@
 	up(&STp->lock);
 	return retval;
 }
+
+#ifdef CONFIG_COMPAT
+static int st_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct scsi_tape *STp = file->private_data;
+	struct scsi_device *sdev = STp->device;
+	int ret = -ENOIOCTLCMD;
+	if (sdev->host->hostt->compat_ioctl) { 
+
+		ret = sdev->host->hostt->compat_ioctl(sdev, cmd, arg);
+
+	}
+	return ret;
+}
+#endif
+
 \f

 
 /* Try to allocate a new tape buffer. Calling function must not hold
@@ -3716,6 +3732,9 @@
 	.read =		st_read,
 	.write =	st_write,
 	.ioctl =	st_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = st_compat_ioctl,
+#endif
 	.open =		st_open,
 	.flush =	st_flush,
 	.release =	st_release,

Re: [PATCH] Add compat_ioctl to st

[Christoph Hellwig]

> +#ifdef CONFIG_COMPAT
> +static int st_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +	struct scsi_tape *STp = file->private_data;
> +	struct scsi_device *sdev = STp->device;
> +	int ret = -ENOIOCTLCMD;
> +	if (sdev->host->hostt->compat_ioctl) { 
> +
> +		ret = sdev->host->hostt->compat_ioctl(sdev, cmd, arg);

This seems to miss a cast for arg.  Also I think it would be really nice
to have a little helper like:

int scsi_compat_ioctl(struct scsi_device *sdev, unsigned int cmd,
		unsiged long arg)
{
	struct scsi_host_template *hostt = sdev->host->hostt;
	if (hostt->compat_ioctl)
		return hostt->compat_ioctl(sdev, cmd, (void __user *)arg);
	return -ENOIOCTLCMD;
}

that all drivers could use.

Re: [PATCH] Add compat_ioctl to st

[Andi Kleen]

On Tue, Jan 18, 2005 at 11:23:57AM +0000, Christoph Hellwig wrote:
> > +#ifdef CONFIG_COMPAT
> > +static int st_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > +{
> > +	struct scsi_tape *STp = file->private_data;
> > +	struct scsi_device *sdev = STp->device;
> > +	int ret = -ENOIOCTLCMD;
> > +	if (sdev->host->hostt->compat_ioctl) { 
> > +
> > +		ret = sdev->host->hostt->compat_ioctl(sdev, cmd, arg);
> 
> This seems to miss a cast for arg.  Also I think it would be really nice
> to have a little helper like:

Hmm, yes.

Well, there are so many warnings in the code currently anyways
that one more won't really hurt ;-) 

> 
> int scsi_compat_ioctl(struct scsi_device *sdev, unsigned int cmd,
> 		unsiged long arg)
> {
> 	struct scsi_host_template *hostt = sdev->host->hostt;
> 	if (hostt->compat_ioctl)
> 		return hostt->compat_ioctl(sdev, cmd, (void __user *)arg);
> 	return -ENOIOCTLCMD;
> }
> 
> that all drivers could use.

I don't think this makes sense for three lines of code. 

-Andi

Re: [PATCH] Add compat_ioctl to st

[Matthew Wilcox]

On Tue, Jan 18, 2005 at 12:06:13PM +0100, Andi Kleen wrote:
> @@ -3716,6 +3732,9 @@
>  	.read =		st_read,
>  	.write =	st_write,
>  	.ioctl =	st_ioctl,
> +#ifdef CONFIG_COMPAT
> +	.compat_ioctl = st_compat_ioctl,
> +#endif
>  	.open =		st_open,
>  	.flush =	st_flush,
>  	.release =	st_release,

How about having a macro ...

	.ioctl =	st_ioctl,
	COMPAT_IOCTL_ENTRY(st_compat_ioctl)
	.open =		st_open,

which could be defined something like:

#ifdef CONFIG_COMPAT
#define COMPAT_IOCTL_ENTRY(x)	.compat_ioctl = x,
#else
#define COMPAT_IOCTL_ENTRY(x)	/* */
#endif

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH] Add compat_ioctl to st

[Andi Kleen]

> How about having a macro ...
> 
> 	.ioctl =	st_ioctl,
> 	COMPAT_IOCTL_ENTRY(st_compat_ioctl)
> 	.open =		st_open,
> 
> which could be defined something like:
> 
> #ifdef CONFIG_COMPAT
> #define COMPAT_IOCTL_ENTRY(x)	.compat_ioctl = x,
> #else
> #define COMPAT_IOCTL_ENTRY(x)	/* */
> #endif

To be honest I prefer the ifdef. It makes it clear 
what is going on, without any unneeded abstraction.
The macro in comparison looks unnatural in the definition
block.

-Andi

Re: [PATCH] Add compat_ioctl to st

[Matthew Wilcox]

On Tue, Jan 18, 2005 at 01:30:24PM +0100, Andi Kleen wrote:
> > How about having a macro ...
> > 
> > 	.ioctl =	st_ioctl,
> > 	COMPAT_IOCTL_ENTRY(st_compat_ioctl)
> > 	.open =		st_open,
> > 
> > which could be defined something like:
> > 
> > #ifdef CONFIG_COMPAT
> > #define COMPAT_IOCTL_ENTRY(x)	.compat_ioctl = x,
> > #else
> > #define COMPAT_IOCTL_ENTRY(x)	/* */
> > #endif
> 
> To be honest I prefer the ifdef. It makes it clear 
> what is going on, without any unneeded abstraction.
> The macro in comparison looks unnatural in the definition
> block.

It would have prevented the mistake you made in sg (which message I only
read after writing this).  Another way of managing without ifdefs would
be:

	.compat_ioctl = COMPAT_IOCTL_ENTRY(st),

with

#ifdef CONFIG_COMPAT
#define COMPAT_IOCTL_ENTRY(x)	x##_compat_ioctl
#else
#define COMPAT_IOCTL_ENTRY(x)	NULL
#endif

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain