On 12/19/2012 02:12 AM, Dmitry Antipov wrote:
On 12/19/2012 04:55 AM, John Stultz wrote:
I was playing with your current ashmem unit test and needed the following trivial patch to get it all (seemingly) working against Linus' head kernel w/ ubuntu userland.
Ashmem unit test currently targets ashmem kernel driver from:
http://linux-arm.org/git?p=linux-2.6-armdroid.git%3Ba=shortlog%3Bh=refs/head...
which is a bit different from k.org version (we use armdroid version for juice-aosp because 64-bit developement goes there). Note that armdroid version has bug: ashmem.c expects that _IOC_SIZE (ioctl_number) == sizeof (struct ashmem_pin) for ASHMEM_GET_PIN_STATUS, but ashmem.h encodes ASHMEM_GET_PIN_STATUS with _IO(XXX), e.g. with zero _IOC_SIZE.
Since ASHMEM_GET_PIN_STATUS ioctl (a.k.a. ASHMEM_ISPINNED in user-space Android headers) uses struct ashmem_pin as 3rd arg, I believe that the correct definition for the user space is:
#define ASHMEM_ISPINNED _IOW(__ASHMEMIOC, 9, struct ashmem_pin)
and for the kernel, respectively:
#define ASHMEM_GET_PIN_STATUS _IOW(__ASHMEMIOC, 9, struct ashmem_pin)
This is the way I make it work in juice-aosp:
http://git.linaro.org/gitweb?p=people/antipov/linux-aarch64.git%3Ba=commit%3...
So looking at this after having coffee, and while I think you're technically right, the problem is that your change introduce an ABI break with the existing kernel interface. That's a big no-no.
The _IOW() macro just generates a number. Its useful so that the commands are unique, given the size of the structure may be different on different arches. This allows the same constant to get different ioctl command numbers for 32bit and 64bit systems, if the structure size differs.
Luckily the ashmem_pin structure is explicit in size and constant across arches, so its "safe".
So fixing this would do nothing but break the existing abi.
A similar issue was recently reverted, so I don't think we want to propose such a change. http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git%3Ba=commitd...
So I'd suggest instead to make your test consistent with the kernel, rather then the other way around.
As an aside, if we were unlucky and the structure being passed was not constant between arches, the proper solution would probably be to correct the ioctl cmd definitions as you have, but also to provide a compatability definition "_GET_PIN_STATUS_OLD" or that used the same broken definition. Then wire in the _OLD command in the ioctl assuming the existing users are 32bits. This way you fix properly it for future applications, and avoid breaking old existing applications. If binder has similar problems, this approach may be more useful.
thanks -john