Messages in this thread | | | Date | Tue, 8 Nov 2005 00:02:50 +0800 (WST) | From | Ian Kent <> | Subject | Re: [PATCH 15/25] autofs: move ioctl32 to autofs{,4}/root.c |
| |
On Mon, 7 Nov 2005, Arnd Bergmann wrote:
> On Sünndag 06 November 2005 07:22, Ian Kent wrote: > > On Sat, 5 Nov 2005, Arnd Bergmann wrote: > > > > 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? > > I think the compat_ptr() macro is not defined on architectures that don't > have 32 bit compat code, but we could change that. > > > 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 point here is that the two are different on 64 bit platforms, since > sizeof (int) != sizeof (long). You also can't do > > switch (cmd) { > case AUTOFS_IOC_SETTIMEOUT32: > case AUTOFS_IOC_SETTIMEOUT: > return do_stuff(); > } > > because then gcc would complain about duplicate case targets on 32 bit > targets.
I was thinking that if the module was compiled for 64bit then the 64bit definition would prevail and visa versa.
eg. In the include file.
#ifdef COMPAT_IOCTL #define AUTOFS_IOC_SETTIMEOUT(..., unsigned int) #else #define AUTOFS_IOC_SETTIMEOUT(...,unsigned long) #endif
I think I'm going to have to investigate further following the implementation.
> > > 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). > > I used the BKL here in order to maintain the current semantics, because > ioctl is always called with BKL held, and compat_ioctl is called without > it.
Of course a sensible approach.
> > If you are sure you don't need the BKL, then you should also replace > ".ioctl = ..." with ".unlocked_ioctl = ...".
Yep. I'll check and amend it later. After all it will be part of the module then.
Thanks Ian
| |