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

James Bottomley
2008-07-16T14:28:23Z

On Wed, 2008-07-16 at 16:18 +0200, Ingo Molnar wrote:
> * James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
>
> > On Wed, 2008-07-16 at 15:15 +0200, Ingo Molnar wrote:
> > > * Ingo Molnar <mingo@elte.hu> 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@linux.vnet.ibm.com>
> AuthorDate: Wed Jul 2 10:56:35 2008 +0200
> Commit: James Bottomley <James.Bottomley@HansenPartnership.com>
> 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