Re: compat ioctl32 for /dev/snapshot?

[Arnd Bergmann <arnd@arndb.de>]

On Tuesday 05 May 2009, Michael Tokarev wrote:
> Arnd Bergmann wrote:
> > On Monday 04 May 2009, Michael Tokarev wrote:
> >> Is the following patch ok?  I just pulled all the SNAPSHOT_* stuff from
> >> include/linux/suspend_ioctls.h and added them into fs/compat_ioctl.c.
> []
> 
> > You are however missing support for deprecated ioctls in your
> > code: SNAPSHOT_SET_SWAP_FILE and SNAPSHOT_PMOPS are trivial
> > (COMPATIBLE_IOCTL), but you also need to add support for
> > these
> 
> Well, as comments in that file (kernel/power/user.c) states, those
> ioctls are obsolete and will be removed and are only preserved for
> compatibility etc.  Since no one complained so far (well, no one
> complained about whole /dev/snapshot thing at all ;), maybe there's
> no need to define those and the following ones too?
> 
> > #define SNAPSHOT_ATOMIC_SNAPSHOT32 _IOW(SNAPSHOT_IOC_MAGIC, 3, compat_uptr_t)
> > #define SNAPSHOT_SET_IMAGE_SIZE32 _IOW(SNAPSHOT_IOC_MAGIC, 6, compat_ulong)
> > #define SNAPSHOT_AVAIL_SWAP32 _IOR(SNAPSHOT_IOC_MAGIC, 7, compat_uptr_t)
> > #define SNAPSHOT_GET_SWAP_PAGE32 _IOR(SNAPSHOT_IOC_MAGIC, 8, compat_uptr_t)

The point is that they are meant for compatibility with existing binaries,
which is also the reason for having the compat_ioctl in the first place.
The kernel should behave in identical ways for both 32 and 64 bits, so
either we add all of the ioctl numbers in compat mode, or we remove support
for the obsolete calls in native mode as well.

> > Please try this patch instead:
> 
> Ok.  Your patch was garbled by your MUA (inserting =3D =20 and breaking
> lines), but that's ok.

sorry about that, I'll try to find out how that happened.

> After de-garbling it now complains about 
> undefined compat_uptr_t.  For that, I've added
> 
> #ifdef CONFIG_COMPAT
> #include <linux/compat.h>
> #endif

This one does not need to be enclosed in #ifdef CONFIG_COMPAT,
the file can always be included. The reason for the #ifdef inside
of the switch() statement is that you otherwise get warnings
about duplicate case statements on 32 bit kernels.
 
	Arnd <><