Patchwork [3/5] sysvipc: Consolidate semtimedop s390

login
register
mail settings
Submitter Adhemerval Zanella Netto
Date May 16, 2019, 3:12 p.m.
Message ID <20190516151249.19029-3-adhemerval.zanella@linaro.org>
Download mbox | patch
Permalink /patch/32713/
State New
Headers show

Comments

Adhemerval Zanella Netto - 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
Florian Weimer - May 16, 2019, 3:41 p.m.
* 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 Netto - May 16, 2019, 7:09 p.m.
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.
* 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 Netto - May 17, 2019, 12:30 p.m.
> 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.
* 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@arndb.de - May 17, 2019, 7:09 p.m.
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 Netto - May 20, 2019, 1:29 p.m.
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
 }