Re: [build fix] Re: [GIT PATCH] SCSI part 1

[James Bottomley]

On Wed, 2008-07-16 at 16:18 +0200, Ingo Molnar wrote:
> * James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote:
> 
> > On Wed, 2008-07-16 at 15:15 +0200, Ingo Molnar wrote:
> > > * Ingo Molnar <mingo@xxxxxxx> wrote:
> > > 
> > > > > scsi_cmnd.h depends on symbols defined in blkdev.h. The fix is to 
> > > > > include blkdev.h as well.
> > > > 
> > > > that wont work - a better replacement fix is the one below. The 
> > > > problem is that scsi.h is included even on !CONFIG_BLOCK and then the 
> > > > BLK_MAX_CDB symbol is meaningless.
> > > 
> > > -v3 .. the new methods need to be under #ifdef CONFIG_BLOCK as well. 
> > > Note my patch is just a quick RFC, this can probably be done 
> > > cleaner.
> > 
> > Erm, Ingo, if you'd just follow linux-next instead of your own tree, 
> > you'd see there's already a fix for this.
> 
> Erm, no. In the merge window i follow upstream -git, not "my tree", and 
> i searched lkml for the build failure signature and it had nothing 
> there. Then i looked at the commit and it said that it was created just 
> 1 day before the merge window started:
> 
>  commit feac6a07c4a3578bffd6769bb4927e8a7e1f3ffe
>  Author:     Martin Petermann <martin@xxxxxxxxxxxxxxxxxx>
>  AuthorDate: Wed Jul 2 10:56:35 2008 +0200
>  Commit:     James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
>  CommitDate: Sat Jul 12 08:22:34 2008 -0500
>                  ^^^^^^
> 
> So i didnt even think of it having hit linux-next so i didnt look into 
> the linux-next archives. lkml should have been Cc:-ed in this case, 

It was, that would be this email:

http://marc.info/?l=linux-kernel&m=121555252007662

Perhaps now you might see how even you can miss important stuff on such
a high volume, low signal to noise ratio list ...

> that's where people look for in case of upstream breakages. You would 
> have saved me some effort via that - please try to do it in the future, 
> it's very helpful to testers.

Not if they miss the email, as you apparently did.

> btw., about the technical aspects of the solution, i'm not sure i like 
> these big #ifdef blocks:
> 
> > --- linux-next-20080708.orig/fs/compat_ioctl.c
> > +++ linux-next-20080708/fs/compat_ioctl.c
> > @@ -68,9 +68,11 @@
> >  #include <linux/capi.h>
> >  #include <linux/gigaset_dev.h>
> >  
> > +#ifdef CONFIG_BLOCK
> >  #include <scsi/scsi.h>
> >  #include <scsi/scsi_ioctl.h>
> >  #include <scsi/sg.h>
> > +#endif
> >  
> >  #include <asm/uaccess.h>
> >  #include <linux/ethtool.h>
> > @@ -1965,6 +1967,7 @@ COMPATIBLE_IOCTL(GIO_UNISCRNMAP)
> >  COMPATIBLE_IOCTL(PIO_UNISCRNMAP)
> >  COMPATIBLE_IOCTL(PIO_FONTRESET)
> >  COMPATIBLE_IOCTL(PIO_UNIMAPCLR)
> > +#ifdef CONFIG_BLOCK
> >  /* Big S */
> >  COMPATIBLE_IOCTL(SCSI_IOCTL_GET_IDLUN)
> >  COMPATIBLE_IOCTL(SCSI_IOCTL_DOORLOCK)
> > @@ -1974,6 +1977,7 @@ COMPATIBLE_IOCTL(SCSI_IOCTL_GET_BUS_NUMB
> >  COMPATIBLE_IOCTL(SCSI_IOCTL_SEND_COMMAND)
> >  COMPATIBLE_IOCTL(SCSI_IOCTL_PROBE_HOST)
> >  COMPATIBLE_IOCTL(SCSI_IOCTL_GET_PCI)
> > +#endif
> >  /* Big T */
> >  COMPATIBLE_IOCTL(TUNSETNOCSUM)
> >  COMPATIBLE_IOCTL(TUNSETDEBUG)
> > @@ -2044,6 +2048,7 @@ COMPATIBLE_IOCTL(SIOCGIFVLAN)
> >  COMPATIBLE_IOCTL(SIOCSIFVLAN)
> >  COMPATIBLE_IOCTL(SIOCBRADDBR)
> >  COMPATIBLE_IOCTL(SIOCBRDELBR)
> > +#ifdef CONFIG_BLOCK
> >  /* SG stuff */
> >  COMPATIBLE_IOCTL(SG_SET_TIMEOUT)
> >  COMPATIBLE_IOCTL(SG_GET_TIMEOUT)
> > @@ -2068,6 +2073,7 @@ COMPATIBLE_IOCTL(SG_SCSI_RESET)
> >  COMPATIBLE_IOCTL(SG_GET_REQUEST_TABLE)
> >  COMPATIBLE_IOCTL(SG_SET_KEEP_ORPHAN)
> >  COMPATIBLE_IOCTL(SG_GET_KEEP_ORPHAN)
> > +#endif
> >  /* PPP stuff */
> >  COMPATIBLE_IOCTL(PPPIOCGFLAGS)
> >  COMPATIBLE_IOCTL(PPPIOCSFLAGS)
> 
> the clean solution we use everywhere else is to push such #ifdefs into 
> the headers, to make them generally includable. For example you can 
> include lockdep.h even if you dont have lockdep enabled, you can include 
> smp.h even on UP-only files, etc. etc.

The problem being that compat stuff only needs to exist for block ioctls
if block actually exists.  If we're going to have a single giant file
for all compat ioctls, then I think we have to fix it the way randy has
done.  If you want to separate it into say compat_block.h and simply
#include that into compat.h then we can fix it more cleanly.

James


--
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/