[3/5] sysvipc: Consolidate semtimedop s390

Message ID 20190516151249.19029-3-adhemerval.zanella@linaro.org
State New
Headers show
Series
  • [1/5] sysvipc: Fix compat msgctl
Related show

Commit Message

Adhemerval Zanella May 16, 2019, 3:12 p.m.
This patch consolidates the s390-32 semtimedop implementation by defining
a arch-specific SEMTIMEDOP_IPC_ARGS to rearrange the arguments expected
by s390 Linux kABI.  The idea is to avoid have multiples semtimedop
implementation changes for Linux v5.1 change to enable wire-up sysvipc
support.

Checked with a s390-linux-gnu and checking that resulting semtimedop
objects do not change.

	* sysdeps/unix/sysv/linux/ipc_priv.h (SEMTIMEDOP_IPC_ARGS): New
	define.
	* sysdpes/unix/sysv/linux/s390/s390-32/ipc_priv.h: New file.
	* sysdeps/unix/sysv/linux/s390/semtimedop.c: Remove file.
	* sysdeps/unix/sysv/linux/semtimedop.c (semtimedop): Use
	SEMTIMEDOP_IPC_ARGS for calls with __NR_ipc.
---
 sysdeps/unix/sysv/linux/ipc_priv.h            |  3 ++
 .../unix/sysv/linux/s390/s390-32/ipc_priv.h   | 29 +++++++++++++++
 sysdeps/unix/sysv/linux/s390/semtimedop.c     | 36 -------------------
 sysdeps/unix/sysv/linux/semtimedop.c          |  4 +--
 4 files changed, 34 insertions(+), 38 deletions(-)
 create mode 100644 sysdeps/unix/sysv/linux/s390/s390-32/ipc_priv.h
 delete mode 100644 sysdeps/unix/sysv/linux/s390/semtimedop.c

-- 
2.17.1

Comments

Florian Weimer May 16, 2019, 3:41 p.m. | #1
* Adhemerval Zanella:

> diff --git a/sysdeps/unix/sysv/linux/ipc_priv.h b/sysdeps/unix/sysv/linux/ipc_priv.h

> index 65adbb093e..49018c1b28 100644

> --- a/sysdeps/unix/sysv/linux/ipc_priv.h

> +++ b/sysdeps/unix/sysv/linux/ipc_priv.h

> @@ -34,4 +34,7 @@ struct __old_ipc_perm

>  #define MSGRCV_ARGS(__msgp, __msgtyp) \

>    ((long int []){ (long int) __msgp, __msgtyp })

>  

> +#define SEMTIMEDOP_IPC_ARGS(__nsops, __sops, __timeout) \

> +  (__nsops), 0, (__sops), (__timeout)


Maybe add a reference to the s390 version?

> diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/ipc_priv.h b/sysdeps/unix/sysv/linux/s390/s390-32/ipc_priv.h

> new file mode 100644

> index 0000000000..9d74e92289


> +/* The s390 sys_ipc variant has only five parameters instead of six

> +   (as for default variant) and the only difference is the handling of

> +   SEMTIMEDOP where on s390 the third parameter is used as a pointer

> +   to a struct timespec where the generic variant uses fifth parameter.  */

> +#undef SEMTIMEDOP_IPC_ARGS

> +#define SEMTIMEDOP_IPC_ARGS(__nsops, __sops, __timeout) \

> +  (__nsops), (__timeout), (__sops)


I find “and the only difference is” a bit confusing here.  Maybe write
“.  The difference is” instead?

Rest of the patch looks okay to me.

Thanks,
Florian
Adhemerval Zanella May 16, 2019, 7:09 p.m. | #2
On 16/05/2019 12:41, Florian Weimer wrote:
> * Adhemerval Zanella:

> 

>> diff --git a/sysdeps/unix/sysv/linux/ipc_priv.h b/sysdeps/unix/sysv/linux/ipc_priv.h

>> index 65adbb093e..49018c1b28 100644

>> --- a/sysdeps/unix/sysv/linux/ipc_priv.h

>> +++ b/sysdeps/unix/sysv/linux/ipc_priv.h

>> @@ -34,4 +34,7 @@ struct __old_ipc_perm

>>  #define MSGRCV_ARGS(__msgp, __msgtyp) \

>>    ((long int []){ (long int) __msgp, __msgtyp })

>>  

>> +#define SEMTIMEDOP_IPC_ARGS(__nsops, __sops, __timeout) \

>> +  (__nsops), 0, (__sops), (__timeout)

> 

> Maybe add a reference to the s390 version?


What about: 

/* This macro is required to handle the s390 variants, which passes the
   arguments in a different order than default.  */

> 

>> diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/ipc_priv.h b/sysdeps/unix/sysv/linux/s390/s390-32/ipc_priv.h

>> new file mode 100644

>> index 0000000000..9d74e92289

> 

>> +/* The s390 sys_ipc variant has only five parameters instead of six

>> +   (as for default variant) and the only difference is the handling of

>> +   SEMTIMEDOP where on s390 the third parameter is used as a pointer

>> +   to a struct timespec where the generic variant uses fifth parameter.  */

>> +#undef SEMTIMEDOP_IPC_ARGS

>> +#define SEMTIMEDOP_IPC_ARGS(__nsops, __sops, __timeout) \

>> +  (__nsops), (__timeout), (__sops)

> 

> I find “and the only difference is” a bit confusing here.  Maybe write

> “.  The difference is” instead?


Ack, I change it locally.

> 

> Rest of the patch looks okay to me.

> 

> Thanks,

> Florian

>
Florian Weimer May 17, 2019, 9:13 a.m. | #3
* Adhemerval Zanella:

> On 16/05/2019 12:41, Florian Weimer wrote:

>> * Adhemerval Zanella:

>> 

>>> diff --git a/sysdeps/unix/sysv/linux/ipc_priv.h b/sysdeps/unix/sysv/linux/ipc_priv.h

>>> index 65adbb093e..49018c1b28 100644

>>> --- a/sysdeps/unix/sysv/linux/ipc_priv.h

>>> +++ b/sysdeps/unix/sysv/linux/ipc_priv.h

>>> @@ -34,4 +34,7 @@ struct __old_ipc_perm

>>>  #define MSGRCV_ARGS(__msgp, __msgtyp) \

>>>    ((long int []){ (long int) __msgp, __msgtyp })

>>>  

>>> +#define SEMTIMEDOP_IPC_ARGS(__nsops, __sops, __timeout) \

>>> +  (__nsops), 0, (__sops), (__timeout)

>> 

>> Maybe add a reference to the s390 version?

>

> What about: 

>

> /* This macro is required to handle the s390 variants, which passes the

>    arguments in a different order than default.  */


Isn't this for s390 (31-bit) only?

Thanks,
Florian
Adhemerval Zanella May 17, 2019, 12:30 p.m. | #4
> Il giorno 17 mag 2019, alle ore 06:13, Florian Weimer <fweimer@redhat.com> ha scritto:

> 

> * Adhemerval Zanella:

> 

>>> On 16/05/2019 12:41, Florian Weimer wrote:

>>> * Adhemerval Zanella:

>>> 

>>>> diff --git a/sysdeps/unix/sysv/linux/ipc_priv.h b/sysdeps/unix/sysv/linux/ipc_priv.h

>>>> index 65adbb093e..49018c1b28 100644

>>>> --- a/sysdeps/unix/sysv/linux/ipc_priv.h

>>>> +++ b/sysdeps/unix/sysv/linux/ipc_priv.h

>>>> @@ -34,4 +34,7 @@ struct __old_ipc_perm

>>>> #define MSGRCV_ARGS(__msgp, __msgtyp) \

>>>>   ((long int []){ (long int) __msgp, __msgtyp })

>>>> 

>>>> +#define SEMTIMEDOP_IPC_ARGS(__nsops, __sops, __timeout) \

>>>> +  (__nsops), 0, (__sops), (__timeout)

>>> 

>>> Maybe add a reference to the s390 version?

>> 

>> What about: 

>> 

>> /* This macro is required to handle the s390 variants, which passes the

>>   arguments in a different order than default.  */

> 

> Isn't this for s390 (31-bit) only?

> 


Indeed, I will change to “s390-32 variant”.

> Thanks,

> Florian
Florian Weimer May 17, 2019, 12:44 p.m. | #5
* Adhemerval Zanella:

>> Il giorno 17 mag 2019, alle ore 06:13, Florian Weimer <fweimer@redhat.com> ha scritto:

>> 

>> * Adhemerval Zanella:

>> 

>>>> On 16/05/2019 12:41, Florian Weimer wrote:

>>>> * Adhemerval Zanella:

>>>> 

>>>>> diff --git a/sysdeps/unix/sysv/linux/ipc_priv.h b/sysdeps/unix/sysv/linux/ipc_priv.h

>>>>> index 65adbb093e..49018c1b28 100644

>>>>> --- a/sysdeps/unix/sysv/linux/ipc_priv.h

>>>>> +++ b/sysdeps/unix/sysv/linux/ipc_priv.h

>>>>> @@ -34,4 +34,7 @@ struct __old_ipc_perm

>>>>> #define MSGRCV_ARGS(__msgp, __msgtyp) \

>>>>>   ((long int []){ (long int) __msgp, __msgtyp })

>>>>> 

>>>>> +#define SEMTIMEDOP_IPC_ARGS(__nsops, __sops, __timeout) \

>>>>> +  (__nsops), 0, (__sops), (__timeout)

>>>> 

>>>> Maybe add a reference to the s390 version?

>>> 

>>> What about: 

>>> 

>>> /* This macro is required to handle the s390 variants, which passes the

>>>   arguments in a different order than default.  */

>> 

>> Isn't this for s390 (31-bit) only?

>> 

>

> Indeed, I will change to “s390-32 variant”.


Thanks, looks good to me.

Florian
Arnd Bergmann May 17, 2019, 7:09 p.m. | #6
On Fri, May 17, 2019 at 2:30 PM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
> > Il giorno 17 mag 2019, alle ore 06:13, Florian Weimer <fweimer@redhat.com> ha scritto:

> >

> > * Adhemerval Zanella:

> >

> >>> On 16/05/2019 12:41, Florian Weimer wrote:

> >>> * Adhemerval Zanella:

> >>>

> >>>> diff --git a/sysdeps/unix/sysv/linux/ipc_priv.h b/sysdeps/unix/sysv/linux/ipc_priv.h

> >>>> index 65adbb093e..49018c1b28 100644

> >>>> --- a/sysdeps/unix/sysv/linux/ipc_priv.h

> >>>> +++ b/sysdeps/unix/sysv/linux/ipc_priv.h

> >>>> @@ -34,4 +34,7 @@ struct __old_ipc_perm

> >>>> #define MSGRCV_ARGS(__msgp, __msgtyp) \

> >>>>   ((long int []){ (long int) __msgp, __msgtyp })

> >>>>

> >>>> +#define SEMTIMEDOP_IPC_ARGS(__nsops, __sops, __timeout) \

> >>>> +  (__nsops), 0, (__sops), (__timeout)

> >>>

> >>> Maybe add a reference to the s390 version?

> >>

> >> What about:

> >>

> >> /* This macro is required to handle the s390 variants, which passes the

> >>   arguments in a different order than default.  */

> >

> > Isn't this for s390 (31-bit) only?

> >

>

> Indeed, I will change to “s390-32 variant”.


According to the kernel sources, the odd sys_s390_ipc function is
used in both native 64-bit and compat 32-bit variants. The 64-bit
s390 ABI also started out with a maximum of five arguments,
so the wrapper was kept there, see:

arch/s390/kernel/syscalls/syscall.tbl:117  common       ipc
sys_s390_ipc compat_sys_s390_ipc

       Arnd
Adhemerval Zanella May 20, 2019, 1:29 p.m. | #7
On 17/05/2019 16:09, Arnd Bergmann wrote:
> On Fri, May 17, 2019 at 2:30 PM Adhemerval Zanella

> <adhemerval.zanella@linaro.org> wrote:

>>> Il giorno 17 mag 2019, alle ore 06:13, Florian Weimer <fweimer@redhat.com> ha scritto:

>>>

>>> * Adhemerval Zanella:

>>>

>>>>> On 16/05/2019 12:41, Florian Weimer wrote:

>>>>> * Adhemerval Zanella:

>>>>>

>>>>>> diff --git a/sysdeps/unix/sysv/linux/ipc_priv.h b/sysdeps/unix/sysv/linux/ipc_priv.h

>>>>>> index 65adbb093e..49018c1b28 100644

>>>>>> --- a/sysdeps/unix/sysv/linux/ipc_priv.h

>>>>>> +++ b/sysdeps/unix/sysv/linux/ipc_priv.h

>>>>>> @@ -34,4 +34,7 @@ struct __old_ipc_perm

>>>>>> #define MSGRCV_ARGS(__msgp, __msgtyp) \

>>>>>>   ((long int []){ (long int) __msgp, __msgtyp })

>>>>>>

>>>>>> +#define SEMTIMEDOP_IPC_ARGS(__nsops, __sops, __timeout) \

>>>>>> +  (__nsops), 0, (__sops), (__timeout)

>>>>>

>>>>> Maybe add a reference to the s390 version?

>>>>

>>>> What about:

>>>>

>>>> /* This macro is required to handle the s390 variants, which passes the

>>>>   arguments in a different order than default.  */

>>>

>>> Isn't this for s390 (31-bit) only?

>>>

>>

>> Indeed, I will change to “s390-32 variant”.

> 

> According to the kernel sources, the odd sys_s390_ipc function is

> used in both native 64-bit and compat 32-bit variants. The 64-bit

> s390 ABI also started out with a maximum of five arguments,

> so the wrapper was kept there, see:

> 

> arch/s390/kernel/syscalls/syscall.tbl:117  common       ipc

> sys_s390_ipc compat_sys_s390_ipc

> 


Thanks for remind me this is not for 31-bit only. I moved the
sysdeps/unix/sysv/linux/s390/s390-32/ipc_priv.h to 
sysdeps/unix/sysv/linux/s390/ipc_priv.h.

Patch

diff --git a/sysdeps/unix/sysv/linux/ipc_priv.h b/sysdeps/unix/sysv/linux/ipc_priv.h
index 65adbb093e..49018c1b28 100644
--- a/sysdeps/unix/sysv/linux/ipc_priv.h
+++ b/sysdeps/unix/sysv/linux/ipc_priv.h
@@ -34,4 +34,7 @@  struct __old_ipc_perm
 #define MSGRCV_ARGS(__msgp, __msgtyp) \
   ((long int []){ (long int) __msgp, __msgtyp })
 
+#define SEMTIMEDOP_IPC_ARGS(__nsops, __sops, __timeout) \
+  (__nsops), 0, (__sops), (__timeout)
+
 #include <ipc_ops.h>
diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/ipc_priv.h b/sysdeps/unix/sysv/linux/s390/s390-32/ipc_priv.h
new file mode 100644
index 0000000000..9d74e92289
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/s390/s390-32/ipc_priv.h
@@ -0,0 +1,29 @@ 
+/* Arch-specific SysV IPC definitions for Linux.  s390 version.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <sysdeps/unix/sysv/linux/ipc_priv.h>
+
+/* The s390 sys_ipc variant has only five parameters instead of six
+   (as for default variant) and the only difference is the handling of
+   SEMTIMEDOP where on s390 the third parameter is used as a pointer
+   to a struct timespec where the generic variant uses fifth parameter.  */
+#undef SEMTIMEDOP_IPC_ARGS
+#define SEMTIMEDOP_IPC_ARGS(__nsops, __sops, __timeout) \
+  (__nsops), (__timeout), (__sops)
+
+#include <ipc_ops.h>
diff --git a/sysdeps/unix/sysv/linux/s390/semtimedop.c b/sysdeps/unix/sysv/linux/s390/semtimedop.c
deleted file mode 100644
index 772ee432a9..0000000000
--- a/sysdeps/unix/sysv/linux/s390/semtimedop.c
+++ /dev/null
@@ -1,36 +0,0 @@ 
-/* Copyright (C) 2003-2019 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-   Contributed by Martin Schwidefsky <schwidefsky@de.ibm.com>, 2003.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <http://www.gnu.org/licenses/>.  */
-
-#include <sys/sem.h>
-#include <ipc_priv.h>
-#include <sysdep.h>
-#include <errno.h>
-
-/* Perform user-defined atomical operation of array of semaphores.  */
-
-int
-semtimedop (int semid, struct sembuf *sops, size_t nsops,
-	    const struct timespec *timeout)
-{
-  /* The s390 sys_ipc variant has only five parameters instead of six
-     (as for default variant) and the only difference is the handling of
-     SEMTIMEDOP where on s390 the third parameter is used as a pointer
-     to a struct timespec where the generic variant uses fifth parameter.  */
-  return INLINE_SYSCALL_CALL (ipc, IPCOP_semtimedop, semid, nsops, timeout,
-			      sops);
-}
diff --git a/sysdeps/unix/sysv/linux/semtimedop.c b/sysdeps/unix/sysv/linux/semtimedop.c
index 961c6d1f0b..1d746c4117 100644
--- a/sysdeps/unix/sysv/linux/semtimedop.c
+++ b/sysdeps/unix/sysv/linux/semtimedop.c
@@ -30,7 +30,7 @@  semtimedop (int semid, struct sembuf *sops, size_t nsops,
 #ifdef __ASSUME_DIRECT_SYSVIPC_SYSCALLS
   return INLINE_SYSCALL_CALL (semtimedop, semid, sops, nsops, timeout);
 #else
-  return INLINE_SYSCALL_CALL (ipc, IPCOP_semtimedop, semid, nsops, 0, sops,
-			      timeout);
+  return INLINE_SYSCALL_CALL (ipc, IPCOP_semtimedop, semid,
+			      SEMTIMEDOP_IPC_ARGS (nsops, sops, timeout));
 #endif
 }