Re: [PATCH] SCSI: userspace cannot use scsi_command_size_tbl, COMMAND_SIZE and scsi_device_type

Re: [PATCH] SCSI: userspace cannot use scsi_command_size_tbl, COMMAND_SIZE and scsi_device_type

[James Bottomley]

All SCSI patches should be cc'd to the SCSI list

On Sat, 2009-06-27 at 22:35 +0530, Jaswinder Singh Rajput wrote:
> userspace cannot use scsi_command_size_tbl, COMMAND_SIZE
> and scsi_device_type defined in kernel
> 
> fix the following 'make headers_check' warnings:
> 
>   usr/include/scsi/scsi.h:159: userspace cannot call function or variable defined in the kernel
>   usr/include/scsi/scsi.h:285: userspace cannot call function or variable defined in the kernel
> 
> Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput@gmail.com>
> ---
>  include/scsi/scsi.h |   13 +++++++------
>  1 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
> index 084478e..7ba5acf 100644
> --- a/include/scsi/scsi.h
> +++ b/include/scsi/scsi.h
> @@ -156,9 +156,6 @@ scsi_varlen_cdb_length(const void *hdr)
>  	return ((struct scsi_varlen_cdb_hdr *)hdr)->additional_cdb_length + 8;
>  }
>  
> -extern const unsigned char scsi_command_size_tbl[8];
> -#define COMMAND_SIZE(opcode) scsi_command_size_tbl[((opcode) >> 5) & 7]
> -
>  static inline unsigned
>  scsi_command_size(const unsigned char *cmnd)
>  {
> @@ -166,6 +163,13 @@ scsi_command_size(const unsigned char *cmnd)
>  		scsi_varlen_cdb_length(cmnd) : COMMAND_SIZE(cmnd[0]);

Even a simple eyeball inspection of this patch shows that the removal of
COMMAND_SIZE above causes this inline function to fail to compile.

If you can't be bothered even to read your own patches or at the very
least compile test them, what makes you think I should bother reading
anything you send in?

James

Re: [PATCH] SCSI: userspace cannot use scsi_command_size_tbl, COMMAND_SIZE and scsi_device_type

[Jaswinder Singh Rajput]

On Sat, 2009-06-27 at 12:27 -0500, James Bottomley wrote:
> All SCSI patches should be cc'd to the SCSI list
> 
> On Sat, 2009-06-27 at 22:35 +0530, Jaswinder Singh Rajput wrote:
> > userspace cannot use scsi_command_size_tbl, COMMAND_SIZE
> > and scsi_device_type defined in kernel
> > 
> > fix the following 'make headers_check' warnings:
> > 
> >   usr/include/scsi/scsi.h:159: userspace cannot call function or variable defined in the kernel
> >   usr/include/scsi/scsi.h:285: userspace cannot call function or variable defined in the kernel
> > 
> > Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput@gmail.com>
> > ---
> >  include/scsi/scsi.h |   13 +++++++------
> >  1 files changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
> > index 084478e..7ba5acf 100644
> > --- a/include/scsi/scsi.h
> > +++ b/include/scsi/scsi.h
> > @@ -156,9 +156,6 @@ scsi_varlen_cdb_length(const void *hdr)
> >  	return ((struct scsi_varlen_cdb_hdr *)hdr)->additional_cdb_length + 8;
> >  }
> >  
> > -extern const unsigned char scsi_command_size_tbl[8];
> > -#define COMMAND_SIZE(opcode) scsi_command_size_tbl[((opcode) >> 5) & 7]
> > -
> >  static inline unsigned
> >  scsi_command_size(const unsigned char *cmnd)
> >  {
> > @@ -166,6 +163,13 @@ scsi_command_size(const unsigned char *cmnd)
> >  		scsi_varlen_cdb_length(cmnd) : COMMAND_SIZE(cmnd[0]);
> 
> Even a simple eyeball inspection of this patch shows that the removal of
> COMMAND_SIZE above causes this inline function to fail to compile.
> 
> If you can't be bothered even to read your own patches or at the very
> least compile test them, what makes you think I should bother reading
> anything you send in?
> 

oops I am sorry, actually it should be RFC I want to check whether I
need to cover more member under __KERNEL__

[RFC][PATCH] SCSI: userspace cannot use scsi_command_size_tbl, scsi_device_type and friends

userspace cannot use scsi_command_size_tbl and scsi_device_type defined in kernel
as well as its friends :

- SCSI_MAX_VARLEN_CDB_SIZE
- COMMAND_SIZE
- struct scsi_varlen_cdb_hdr
- scsi_varlen_cdb_length()
- scsi_command_size()

fix the following 'make headers_check' warnings:

  usr/include/scsi/scsi.h:159: userspace cannot call function or variable defined in the kernel
  usr/include/scsi/scsi.h:285: userspace cannot call function or variable defined in the kernel

Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput@gmail.com>
---
 include/scsi/scsi.h |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index 084478e..5ac0157 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -134,6 +134,7 @@ struct scsi_cmnd;
 #define	ATA_16		      0x85	/* 16-byte pass-thru */
 #define	ATA_12		      0xa1	/* 12-byte pass-thru */
 
+#ifdef __KERNEL__
 /*
  *	SCSI command lengths
  */
@@ -166,6 +167,9 @@ scsi_command_size(const unsigned char *cmnd)
 		scsi_varlen_cdb_length(cmnd) : COMMAND_SIZE(cmnd[0]);
 }
 
+/* Returns a human-readable name for the device */
+extern const char * scsi_device_type(unsigned type);
+#endif
 /*
  *  SCSI Architecture Model (SAM) Status codes. Taken from SAM-3 draft
  *  T10/1561-D Revision 4 Draft dated 7th November 2002.
@@ -281,9 +285,6 @@ enum scsi_protocol {
 	SCSI_PROTOCOL_UNSPEC = 0xf, /* No specific protocol */
 };
 
-/* Returns a human-readable name for the device */
-extern const char * scsi_device_type(unsigned type);
-
 /*
  * standard mode-select header prepended to all mode-select commands
  */
-- 
1.6.0.6

Re: [PATCH] SCSI: userspace cannot use scsi_command_size_tbl, COMMAND_SIZE and scsi_device_type

[Matthew Wilcox]

On Sat, Jun 27, 2009 at 11:26:28PM +0530, Jaswinder Singh Rajput wrote:
> > On Sat, 2009-06-27 at 22:35 +0530, Jaswinder Singh Rajput wrote:
> > > userspace cannot use scsi_command_size_tbl, COMMAND_SIZE
> > > and scsi_device_type defined in kernel

When did we start exporting include/scsi to userspace?  I thought glibc
had its own separate definitions.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] SCSI: userspace cannot use scsi_command_size_tbl, COMMAND_SIZE and scsi_device_type

[Jaswinder Singh Rajput]

On Sat, 2009-06-27 at 12:28 -0600, Matthew Wilcox wrote:
> On Sat, Jun 27, 2009 at 11:26:28PM +0530, Jaswinder Singh Rajput wrote:
> > > On Sat, 2009-06-27 at 22:35 +0530, Jaswinder Singh Rajput wrote:
> > > > userspace cannot use scsi_command_size_tbl, COMMAND_SIZE
> > > > and scsi_device_type defined in kernel
> 
> When did we start exporting include/scsi to userspace?  I thought glibc
> had its own separate definitions.
> 

commit 9e4f5e29610162fd426366f3b29e3cc6e575b858
Author: James Smart <James.Smart@Emulex.Com>
Date:   Thu Mar 26 13:33:19 2009 -0400

    [SCSI] FC Pass Thru support

--
JSR

Re: [PATCH] SCSI: userspace cannot use scsi_command_size_tbl, COMMAND_SIZE and scsi_device_type

[James Bottomley]

On Sun, 2009-06-28 at 00:10 +0530, Jaswinder Singh Rajput wrote:
> On Sat, 2009-06-27 at 12:28 -0600, Matthew Wilcox wrote:
> > On Sat, Jun 27, 2009 at 11:26:28PM +0530, Jaswinder Singh Rajput wrote:
> > > > On Sat, 2009-06-27 at 22:35 +0530, Jaswinder Singh Rajput wrote:
> > > > > userspace cannot use scsi_command_size_tbl, COMMAND_SIZE
> > > > > and scsi_device_type defined in kernel
> > 
> > When did we start exporting include/scsi to userspace?  I thought glibc
> > had its own separate definitions.
> > 
> 
> commit 9e4f5e29610162fd426366f3b29e3cc6e575b858
> Author: James Smart <James.Smart@Emulex.Com>
> Date:   Thu Mar 26 13:33:19 2009 -0400
> 
>     [SCSI] FC Pass Thru support

That's what you get from a simplistic view.

If you look at the full history, scsi.h and sg.h were exported in 2006
by the initial commit by David Woodhouse

commit 8555255f0b426858d8648c6206b70eb906cf4ec7
Author: David Woodhouse <dwmw2@infradead.org>
Date:   Sun Jun 18 12:14:01 2006 +0100

    Add generic Kbuild files for 'make headers_install'

They were unexported again a year later, apparently on grounds of
clashing with /usr/include/scsi from glibc:

commit e629a7ddc0188e1bb9e956e698a9bd00c19c9854
Author: Olaf Hering <olh@suse.de>
Date:   Tue Oct 16 23:27:01 2007 -0700

    do not export /usr/include/scsi in make headers_install

So perhaps the meta question is how are we supposed to resolve this?
Glibc ceded it's copy of /usr/include/linux to the kernel headers
package, so it looks to be an oversight that it still
retains /usr/include/scsi (there's nothing extra in there in glibc
beyond what SCSI exports).

James

Re: [PATCH] SCSI: userspace cannot use scsi_command_size_tbl, COMMAND_SIZE and scsi_device_type

[Boaz Harrosh]

On 06/28/2009 04:52 PM, James Bottomley wrote:
> On Sun, 2009-06-28 at 00:10 +0530, Jaswinder Singh Rajput wrote:
>> On Sat, 2009-06-27 at 12:28 -0600, Matthew Wilcox wrote:
>>> On Sat, Jun 27, 2009 at 11:26:28PM +0530, Jaswinder Singh Rajput wrote:
>>>>> On Sat, 2009-06-27 at 22:35 +0530, Jaswinder Singh Rajput wrote:
>>>>>> userspace cannot use scsi_command_size_tbl, COMMAND_SIZE
>>>>>> and scsi_device_type defined in kernel
>>> When did we start exporting include/scsi to userspace?  I thought glibc
>>> had its own separate definitions.
>>>
>> commit 9e4f5e29610162fd426366f3b29e3cc6e575b858
>> Author: James Smart <James.Smart@Emulex.Com>
>> Date:   Thu Mar 26 13:33:19 2009 -0400
>>
>>     [SCSI] FC Pass Thru support
> 
> That's what you get from a simplistic view.
> 
> If you look at the full history, scsi.h and sg.h were exported in 2006
> by the initial commit by David Woodhouse
> 
> commit 8555255f0b426858d8648c6206b70eb906cf4ec7
> Author: David Woodhouse <dwmw2@infradead.org>
> Date:   Sun Jun 18 12:14:01 2006 +0100
> 
>     Add generic Kbuild files for 'make headers_install'
> 
> They were unexported again a year later, apparently on grounds of
> clashing with /usr/include/scsi from glibc:
> 
> commit e629a7ddc0188e1bb9e956e698a9bd00c19c9854
> Author: Olaf Hering <olh@suse.de>
> Date:   Tue Oct 16 23:27:01 2007 -0700
> 
>     do not export /usr/include/scsi in make headers_install
> 
> So perhaps the meta question is how are we supposed to resolve this?
> Glibc ceded it's copy of /usr/include/linux to the kernel headers
> package, so it looks to be an oversight that it still
> retains /usr/include/scsi (there's nothing extra in there in glibc
> beyond what SCSI exports).
> 
> James
> 
> 

Right! so a good strategy might be to first fix up the header for proper
user-mode consumption. And then just let glibc eventually drop their scsi.h
header once enough complains register.

Though out of courtesy someone might send a patch to the glibc maintainers to
remove that header. Andrew do you have any passed experience with this project?

I agree that Kernel should get back control of this header.

Boaz

Re: [PATCH] SCSI: userspace cannot use scsi_command_size_tbl, COMMAND_SIZE and scsi_device_type

[Jaswinder Singh Rajput]

On Sat, 2009-06-27 at 23:26 +0530, Jaswinder Singh Rajput wrote:
> On Sat, 2009-06-27 at 12:27 -0500, James Bottomley wrote:
> > All SCSI patches should be cc'd to the SCSI list
> > 
> > On Sat, 2009-06-27 at 22:35 +0530, Jaswinder Singh Rajput wrote:
> > > userspace cannot use scsi_command_size_tbl, COMMAND_SIZE
> > > and scsi_device_type defined in kernel
> > > 
> > > fix the following 'make headers_check' warnings:
> > > 
> > >   usr/include/scsi/scsi.h:159: userspace cannot call function or variable defined in the kernel
> > >   usr/include/scsi/scsi.h:285: userspace cannot call function or variable defined in the kernel
> > > 
> > > Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput@gmail.com>
> > > ---
> > >  include/scsi/scsi.h |   13 +++++++------
> > >  1 files changed, 7 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
> > > index 084478e..7ba5acf 100644
> > > --- a/include/scsi/scsi.h
> > > +++ b/include/scsi/scsi.h
> > > @@ -156,9 +156,6 @@ scsi_varlen_cdb_length(const void *hdr)
> > >  	return ((struct scsi_varlen_cdb_hdr *)hdr)->additional_cdb_length + 8;
> > >  }
> > >  
> > > -extern const unsigned char scsi_command_size_tbl[8];
> > > -#define COMMAND_SIZE(opcode) scsi_command_size_tbl[((opcode) >> 5) & 7]
> > > -
> > >  static inline unsigned
> > >  scsi_command_size(const unsigned char *cmnd)
> > >  {
> > > @@ -166,6 +163,13 @@ scsi_command_size(const unsigned char *cmnd)
> > >  		scsi_varlen_cdb_length(cmnd) : COMMAND_SIZE(cmnd[0]);
> > 
> > Even a simple eyeball inspection of this patch shows that the removal of
> > COMMAND_SIZE above causes this inline function to fail to compile.
> > 
> > If you can't be bothered even to read your own patches or at the very
> > least compile test them, what makes you think I should bother reading
> > anything you send in?
> > 
> 
> oops I am sorry, actually it should be RFC I want to check whether I
> need to cover more member under __KERNEL__
> 
> [RFC][PATCH] SCSI: userspace cannot use scsi_command_size_tbl, scsi_device_type and friends
> 
> userspace cannot use scsi_command_size_tbl and scsi_device_type defined in kernel
> as well as its friends :
> 
> - SCSI_MAX_VARLEN_CDB_SIZE
> - COMMAND_SIZE
> - struct scsi_varlen_cdb_hdr
> - scsi_varlen_cdb_length()
> - scsi_command_size()
> 
> fix the following 'make headers_check' warnings:
> 
>   usr/include/scsi/scsi.h:159: userspace cannot call function or variable defined in the kernel
>   usr/include/scsi/scsi.h:285: userspace cannot call function or variable defined in the kernel
> 
> Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput@gmail.com>


What is the status of this patch.

Thanks,
--
JSR

Re: [PATCH] SCSI: userspace cannot use scsi_command_size_tbl, COMMAND_SIZE and scsi_device_type

[James Bottomley]

On Sun, 2009-06-28 at 10:32 -0400, James Smart wrote:
> Sorry folks --  the nuance of the clibrary having it's own version of a 
> kernel header for distros was completely unknown to me.
> 
> Christoph mentioned this to me last week, with the recommendation to 
> back out the patch and move the other headers that were exprted into 
> include/linux - thus nothing in include/scsi gets export.  No Problem. 
> and I'll do so shortly.

Actually, I'm not sure that's the best way forwards ... the idea of
include/scsi is to group all the SCSI headers together; having to put
them in include/linux if they need to be exported defeats that purpose
slightly.

I think the first point of business might be to find out why (or even
if) glibc still wants its own SCSI headers.  After all, we now have
quite an anomaly: if you use the old SG_IO ioctl, glibc supplies the
header in include/scsi/sg.h; if you use the new bsg SG_IO then we supply
it in include/linux/bsg.h

> How does a lowly contributor track these kind of nuances ?

If the creator of the kernel header exports missed this problem for a
year, I think it's safe to assume you don't lose brownie points for not
spotting it either.

James