Re: [Qemu-devel] [PATCH 5/5] linux-user/signal.c: define __SIGRTMIN/MAX for non-GNU platforms

Re: [Qemu-devel] [PATCH 5/5] linux-user/signal.c: define __SIGRTMIN/MAX for non-GNU platforms

[Riku Voipio]

On Tue, Apr 29, 2014 at 10:06:31PM +0200, Natanael Copa wrote:
> On Tue, 29 Apr 2014 09:02:13 -0600
> Eric Blake <eblake@redhat.com> wrote:
> 
> > On 04/29/2014 08:53 AM, Natanael Copa wrote:
> > > On Tue, 29 Apr 2014 08:28:29 -0600
> > > Eric Blake <eblake@redhat.com> wrote:
> > > 
> > >> On 04/29/2014 08:17 AM, Natanael Copa wrote:
> > >>> The __SIGRTMIN and __SIGRTMAX are glibc internals and are not available
> > >>> on all platforms, so we define those if they are missing.
> > 
> > >>> +#define __SIGRTMIN 32
> > >>
> > >> Rather than defining the implementation-specific __SIGRTMIN to a magic
> > >> number that is liable to be wrong, why not instead fix the code to use
> > >> the POSIX-mandated SIGRTMIN and SIGRTMAX public defines instead?
> > >>
> > > 
> > > Those seems to be runtime values:
> > > /usr/include/signal.h:#define SIGRTMIN  (__libc_current_sigrtmin())
> > 
> > Oh right - POSIX allows them to be runtime variable.  But we are
> > interacting with a given kernel, where the values will be fixed.  Maybe
> > you have to define __SIGRTMIN after all, but can we at least have an
> > assert() that the value you picked matches SIGRTMIN at runtime?
 
> Yeah, that might be an idea.

If you can update the patch with asserts, I'd be fine with the patch.
I don't we want to go into changing to use NSIG-1 unless there is some
compelling advantage on it. 

> > > /usr/include/signal.h:#define SIGRTMAX  (__libc_current_sigrtmax())
> > > 
> > > so it gives:
> > > /home/ncopa/src/qemu/linux-user/signal.c:93:5: error: nonconstant array index in initializer
> > >      [SIGRTMIN] = __SIGRTMAX,
> > > 
> > > I could have used (NSIG-1) but are not sure if NSIG is a runtime macro
> > > in glibc. The array itself is using _NSIG instead of NSIG for some
> > > reason.
> > 
> > NSIG is not any more portable; nor does POSIX require that the RT
> > signals occur at the tail end of NSIG (in other words, NSIG-1 need not
> > be SIGRTMAX).  Unless someone knows of a kernel define, it sounds like
> > we're stuck hard-coding in some knowledge of Linux.
> > 
> 
> Since we already use _NSIG to define the size of the array, and we want
> to use the last element of the array, maybe we should just use _NSIG-1?
> 
> -nc

Re: [Qemu-devel] [PATCH 0/5] fix building with musl libc

[Paolo Bonzini]

Il 29/04/2014 16:17, Natanael Copa ha scritto:
> In addition to the previoiusly sent "linux-user: avoid using glibc
> internals in _syscall5 and in definition of target_sigevent struct",
> those are needed for making qemu build with musl libc on Alpine Linux.
>
> There is still 2 missing fcntl.h definitions (F_EXLCK and F_SHLCK) but
> those should probably be defined in libc so patch for that is not
> included here.
>
> Natanael Copa (5):
>   util/qemu-openpty: fix build with musl libc by include termios.h as
>     fallback
>   xen: replace ffsl with ctzl
>   vhost: replace ffsl with ctzl
>   exec: replace ffsl with ctzl
>   linux-user/signal.c: define __SIGRTMIN/MAX for non-GNU platforms
>
>  hw/virtio/vhost.c       | 6 ++----
>  include/exec/ram_addr.h | 2 +-
>  linux-user/signal.c     | 7 +++++++
>  util/qemu-openpty.c     | 2 ++
>  xen-all.c               | 2 +-
>  5 files changed, 13 insertions(+), 6 deletions(-)
>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 0/5] fix building with musl libc

[Peter Maydell]

On 29 April 2014 15:17, Natanael Copa <ncopa@alpinelinux.org> wrote:
> In addition to the previoiusly sent "linux-user: avoid using glibc
> internals in _syscall5 and in definition of target_sigevent struct",
> those are needed for making qemu build with musl libc on Alpine Linux.
>
> There is still 2 missing fcntl.h definitions (F_EXLCK and F_SHLCK) but
> those should probably be defined in libc so patch for that is not
> included here.
>
> Natanael Copa (5):
>   util/qemu-openpty: fix build with musl libc by include termios.h as
>     fallback
>   xen: replace ffsl with ctzl
>   vhost: replace ffsl with ctzl
>   exec: replace ffsl with ctzl
>   linux-user/signal.c: define __SIGRTMIN/MAX for non-GNU platforms

Hmm. I just noticed these ffsl patches didn't get committed (the BSDs
don't like ffsl either). There were issues with patch 5( the signal.c one)
but 1..4 are good (and get my Reviewed-by: as well as Paolo's).

I'm currently building up a tree with various bsd-user fixes in it,
so I'll take these through that, I think. (Patch 1 is arguably slightly
out of scope for that but better to keep it together with 2..4.)

Natanael: you might want to update the signal.c patch to accommodate
review comments and resubmit it.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/5] fix building with musl libc

[Natanael Copa]

On Wed, 4 Jun 2014 00:37:44 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 29 April 2014 15:17, Natanael Copa <ncopa@alpinelinux.org> wrote:
> > In addition to the previoiusly sent "linux-user: avoid using glibc
> > internals in _syscall5 and in definition of target_sigevent struct",
> > those are needed for making qemu build with musl libc on Alpine Linux.
> >
> > There is still 2 missing fcntl.h definitions (F_EXLCK and F_SHLCK) but
> > those should probably be defined in libc so patch for that is not
> > included here.
> >
> > Natanael Copa (5):
> >   util/qemu-openpty: fix build with musl libc by include termios.h as
> >     fallback
> >   xen: replace ffsl with ctzl
> >   vhost: replace ffsl with ctzl
> >   exec: replace ffsl with ctzl
> >   linux-user/signal.c: define __SIGRTMIN/MAX for non-GNU platforms
> 
> Hmm. I just noticed these ffsl patches didn't get committed (the BSDs
> don't like ffsl either). There were issues with patch 5( the signal.c one)
> but 1..4 are good (and get my Reviewed-by: as well as Paolo's).
> 
> I'm currently building up a tree with various bsd-user fixes in it,
> so I'll take these through that, I think. (Patch 1 is arguably slightly
> out of scope for that but better to keep it together with 2..4.)
> 
> Natanael: you might want to update the signal.c patch to accommodate
> review comments and resubmit it.
> 
> thanks

I will try get it done today. Thanks!

-nc

> -- PMM

[Qemu-devel] [PATCH v2] linux-user/signal.c: define __SIGRTMIN/MAX for non-GNU platforms

[Natanael Copa]

The __SIGRTMIN and __SIGRTMAX are glibc internals and are not available
on all platforms, so we define those if they are missing. We also check
that those corresponds with the posix variables SIGRTMIN/SIGRTMAX which
may only be available during runtime.

This is needed for musl libc.

Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
---
Changes v1 -> v2:
 - replace NSIG with _NSIG since thats use everywhere else in the code.
 - add runtime asserts.

 linux-user/signal.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 5b8a01f..67771ad 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -32,6 +32,13 @@
 
 //#define DEBUG_SIGNAL
 
+#ifndef __SIGRTMIN
+#define __SIGRTMIN 32
+#endif
+#ifndef __SIGRTMAX
+#define __SIGRTMAX (_NSIG-1)
+#endif
+
 static struct target_sigaltstack target_sigaltstack_used = {
     .ss_sp = 0,
     .ss_size = 0,
@@ -379,6 +386,13 @@ void signal_init(void)
     int i, j;
     int host_sig;
 
+    /* SIGRTMIN/SIGRTMAX might be runtime variables so we cannot use them
+       to declare the host_to_target_signal table. But we are interacting
+       with a given kernel where the values will be fixed. Check that the
+       runtime values actually corresponds. */
+    assert(__SIGRTMIN == SIGRTMIN);
+    assert(__SIGRTMAX == SIGRTMAX);
+
     /* generate signal conversion tables */
     for(i = 1; i < _NSIG; i++) {
         if (host_to_target_signal_table[i] == 0)
-- 
2.0.0

Re: [Qemu-devel] [PATCH v2] linux-user/signal.c: define __SIGRTMIN/MAX for non-GNU platforms

[Riku Voipio]

Hi,

On Wed, Jun 04, 2014 at 09:49:00AM +0200, Natanael Copa wrote:
> The __SIGRTMIN and __SIGRTMAX are glibc internals and are not available
> on all platforms, so we define those if they are missing. We also check
> that those corresponds with the posix variables SIGRTMIN/SIGRTMAX which
> may only be available during runtime.
> 
> This is needed for musl libc.

After all, the idea of asserts doesn't work on glibc it seems:

qemu-arm qemu-smoke/armel/busybox ls -ld .
qemu-arm: linux-user/signal.c:393: signal_init: Assertion `32 == (__libc_current_sigrtmin ())' failed.
Aborted

Quick test on my amd64/glibc 2.18 system:

printf("RTMIN: %d RTMAX: %d\n", SIGRTMIN, SIGRTMAX);
RTMIN: 34 RTMAX: 64

While: /usr/include/bits/signum.h
#define __SIGRTMIN  32


> Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
> ---
> Changes v1 -> v2:
>  - replace NSIG with _NSIG since thats use everywhere else in the code.
>  - add runtime asserts.
> 
>  linux-user/signal.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 5b8a01f..67771ad 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -32,6 +32,13 @@
>  
>  //#define DEBUG_SIGNAL
>  
> +#ifndef __SIGRTMIN
> +#define __SIGRTMIN 32
> +#endif
> +#ifndef __SIGRTMAX
> +#define __SIGRTMAX (_NSIG-1)
> +#endif
> +
>  static struct target_sigaltstack target_sigaltstack_used = {
>      .ss_sp = 0,
>      .ss_size = 0,
> @@ -379,6 +386,13 @@ void signal_init(void)
>      int i, j;
>      int host_sig;
>  
> +    /* SIGRTMIN/SIGRTMAX might be runtime variables so we cannot use them
> +       to declare the host_to_target_signal table. But we are interacting
> +       with a given kernel where the values will be fixed. Check that the
> +       runtime values actually corresponds. */
> +    assert(__SIGRTMIN == SIGRTMIN);
> +    assert(__SIGRTMAX == SIGRTMAX);
> +
>      /* generate signal conversion tables */
>      for(i = 1; i < _NSIG; i++) {
>          if (host_to_target_signal_table[i] == 0)
> -- 
> 2.0.0
> 

Re: [Qemu-devel] [PATCH v2] linux-user/signal.c: define __SIGRTMIN/MAX for non-GNU platforms

[Paolo Bonzini]

Il 06/06/2014 10:27, Riku Voipio ha scritto:
> Hi,
>
> On Wed, Jun 04, 2014 at 09:49:00AM +0200, Natanael Copa wrote:
>> The __SIGRTMIN and __SIGRTMAX are glibc internals and are not available
>> on all platforms, so we define those if they are missing. We also check
>> that those corresponds with the posix variables SIGRTMIN/SIGRTMAX which
>> may only be available during runtime.
>>
>> This is needed for musl libc.
>
> After all, the idea of asserts doesn't work on glibc it seems:
>
> qemu-arm qemu-smoke/armel/busybox ls -ld .
> qemu-arm: linux-user/signal.c:393: signal_init: Assertion `32 == (__libc_current_sigrtmin ())' failed.
> Aborted
>
> Quick test on my amd64/glibc 2.18 system:
>
> printf("RTMIN: %d RTMAX: %d\n", SIGRTMIN, SIGRTMAX);
> RTMIN: 34 RTMAX: 64
>
> While: /usr/include/bits/signum.h
> #define __SIGRTMIN  32

That's because glibc reserves two signals (one for cancellation, the 
other to implement set*id system calls).  Basically you'd need to extend 
the hack of host_to_target_signal_table to all signals in the 
[__SIGRTMIN, SIGRTMIN) range, computing the table at run-time.

Paolo