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

[Arnd Bergmann <arnd at arndb dot de>]

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?

	Arnd