Re: [PATCH 1/3] ipc: Refactor sysvipc internal definitions

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

On 15/10/2019 08:45, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> This patch refactor the internal sysvipc in two main points:
>>
>>   1. Add a new __ASSUME_SYSVIPC_DEFAULT_IPC_64 to infer the __IPC_64
>>      value in wire-up or ipc syscall.  The defaut value assumed for
> 
> Sorry, would you please re-phrase?  I don't understand the wire-up
> terminology in this context.

The 'wire-up' here means the syscall is done by its own entrypoint defined
by a __NR_ number instead of a multiplexed one, in this case the __NR_ipc.
Maybe the following is more clear:

  1. Add a new __ASSUME_SYSVIPC_DEFAULT_IPC_64 to infer the __IPC_64
     value to be used along either the multiplexed __NR_ipc or wired-up
     syscall.


> 
>>      __IPC_64 is also changed from 0x100 to 0x0, aligning with Linux
>>      generic UAPI.  The idea is to simplify the Linux 5.1 wire-up for
>>      sysvipc syscalls for some 32-bit ABIs (which expectes __IPC_64
>>      being 0x0) and simplify new ports (which would not require add
>>      a ipc_priv.h to override the __IPC_64 value).
> 
> Replace “would not required add a ipc_priv.h” with “will no longer need
> to add ipc_priv.h”?

Ack.

> 
> 
>>   2. It removes some duplicated definition from sysvipc compat code
>>      at ipc_priv.h.  The idea is also to make it simpler to enable
>>      the new wireup sysvipc on Linux v5.1.
> 
> Again, I don't understand this terminology.

This idea is to consolidate the __old_ipc_perm and other arch-specific 
definitions that are duplicated over the architectures. Maybe:

  2. It also removed some duplicated internal definition used on compat
     sysvipc symbols defined at ipc_priv.h (more specifically the
     __old_ipc_perm, SEMCTL_ARG_ADDRESS, MSGRCV_ARGS, and SEMTIMEDOP_IPC_ARGS).
     The idea is also to make it simpler to enable the new wire-up sysvipc
     syscall provided by Linux v5.1.

> 
>> diff --git a/sysdeps/unix/sysv/linux/alpha/kernel-features.h b/sysdeps/unix/sysv/linux/alpha/kernel-features.h
>> index 3928b4b62c..ac127adff7 100644
>> --- a/sysdeps/unix/sysv/linux/alpha/kernel-features.h
>> +++ b/sysdeps/unix/sysv/linux/alpha/kernel-features.h
>> @@ -52,4 +52,7 @@
>>  # undef __ASSUME_STATX
>>  #endif
>>  
>> +/* Alpha support old sysvipc even being a 64-bit architecture.  */
>> +#undef __ASSUME_SYSVIPC_DEFAULT_IPC_64
> 
> s/support/requires/?

Ack.

> 
>> diff --git a/sysdeps/unix/sysv/linux/alpha/ipc_priv.h b/sysdeps/unix/sysv/linux/alpha/ipc_priv.h
>> index 881d943063..0bf9da5694 100644
>> --- a/sysdeps/unix/sysv/linux/alpha/ipc_priv.h
>> +++ b/sysdeps/unix/sysv/linux/alpha/ipc_priv.h
> 
>> +#define __OLD_IPC_SEQ_TYPE  unsigned short int
>> +#include <sysdeps/unix/sysv/linux/ipc_priv.h>
> 
> Isn't that the __OLD_IPC_SEQ_TYPE default?  (This applies to other
> architectures as well.
> 
> In fact, __OLD_IPC_SEQ_TYPE appears to be unsighed short int always, so
> I think it shouldn't be parameterized.

Indeed, I will remove. 

> 
>> diff --git a/sysdeps/unix/sysv/linux/mips/mips64/ipc_priv.h b/sysdeps/unix/sysv/linux/mips/mips64/ipc_priv.h
>> index 10d5c52dd0..459ee3d2e7 100644
>> --- a/sysdeps/unix/sysv/linux/mips/mips64/ipc_priv.h
>> +++ b/sysdeps/unix/sysv/linux/mips/mips64/ipc_priv.h
> 
>> +#define __OLD_IPC_SEQ_TYPE   unsigned short int
>> +#include <sysdeps/unix/sysv/linux/ipc_priv.h>
> 
> See above.

Ack.

> 
> Rest looks okay to me.
> 
> Thanks,
> Florian
>