Re: [PATCH 02/17] Refactor Linux ipc_priv header

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 12/12/2016 11:25, Arnd Bergmann wrote:
> On Monday, December 12, 2016 10:57:51 AM CET Adhemerval Zanella wrote:
>>
>> On 12/12/2016 10:47, Arnd Bergmann wrote:
>>> On Monday, December 12, 2016 10:32:55 AM CET Adhemerval Zanella wrote:
>>>
>>>> Since current IPC_64 default now on kernel is 0x100, some architectures
>>>> require it to 0 (for instance x86_64) while others continue to use a
>>>> non zero default regardless (powerpc).
>>>
>>> I find the explanation is a bit confusing, it's not that the kernel
>>> requires IPC_64 to be 0x100, it's that some architectures support
>>> the the old-style IPC and require IPC_64 to be passed, while new
>>> architectures should default to the new version.
>>
>> Right, I did not want to expose some kernel internal implementation,
>> but I think your explanation should be better.  I will change to
>>
>> "
>> Some architectures support the old-style IPC and require IPC_64
>> equal to 0x100 to be passed along SysV IPC syscalls, while new
>> architectures should default to new IPC version (without the
>> flags being set).
>>
>> This patch refactor current ipc_priv.h Linux headers in two directions:
>>
>>   - Remove cross platform references (for instance alpha including powerpc
>>     definition) and add required definition for each required port.  The
>>     idea is to avoid tie one architecture definition with another and make
>>     platform change independent.
>>
>>   - Move all common definitions (the ipc syscall commands) on a common
>>     header, ipc_ops.h.
>> "
>>
>> Do you think it is less confusing?
> 
> Sounds good, thanks!
> 
>>>
>>>> index 0000000..1a31396
>>>> --- /dev/null
>>>> +++ b/sysdeps/unix/sysv/linux/aarch64/ipc_priv.h
>>> ...
>>>> +
>>>> +#include <sys/ipc.h>  /* For __key_t  */
>>>> +
>>>> +#define __IPC_64	0x0
>>>> +
>>>> +struct __old_ipc_perm
>>>> +{
>>>> +  __key_t __key;		/* Key.  */
>>>> +  unsigned int uid;		/* Owner's user ID.  */
>>>> +  unsigned int gid;		/* Owner's group ID.  */
>>>> +  unsigned int cuid;		/* Creator's user ID.  */
>>>> +  unsigned int cgid;		/* Creator's group ID.  */
>>>> +  unsigned int mode;		/* Read/write permission.  */
>>>> +  unsigned short int __seq;	/* Sequence number.  */
>>>> +};
>>>
>>> I don't understand this part at all: the #define line first says
>>> that the old IPC is not supported, but then you define the structure
>>> anyway?
>>>
>>> What is the structure actually used for in glibc? My interpretation
>>> is that it's only needed to implement the SHLIB_COMPAT version
>>> of the library calls that don't exist on ARM64 either.
>>
>> You are right and it is mainly a limitation on how my code defines the
>> 'union semun' in semctl.c.  I will change it to use an opaque type
>> instead of a pointer to __old_ipc_perm.
> 
> I guess we can even remove the entire definition of
> struct __old_semid_ds etc on all architectures since we
> just pass a pointer from the compat entry point to the
> kernel without looking at the contents?

Yeap, this is what I ended up doing in my local branch (remove aarch64
ipc_priv.h ununsed definitions and cleanup semctl old ipc struct
definitions).