Re: [Qemu-devel] [PATCH] linux-user: avoid using glibc internals

[Natanael Copa]

On Wed, 23 Apr 2014 19:00:41 +0100
Peter Maydell <address@hidden> wrote:

> On 23 April 2014 15:59, Natanael Copa <address@hidden> wrote:
> > Avoid using glibc specific internals.
> >
> > Calculate the sigevent pad size is calculated in similar way as kernel
> > does it.
> >
> > This is needed for building with musl libc.
> 
> Thanks for this patch -- is this the only fix that was needed, or
> are there more to come?

There are more patches needed to make it build and run with musl libc.
Those were not mine, but I can try clean them up and submit those if
here is interest for it.

The problem this patch resolves was introduced with qemu 2.0.


> It would be nice to be a little more specific in the patch summary
> line about the change, like:
>    linux-user: avoid using glibc internals in definition of
> target_sigevent struct

Agree.

 
> > Signed-off-by: Natanael Copa <address@hidden>
> > ---
> >  linux-user/syscall.c      | 2 +-
> >  linux-user/syscall_defs.h | 6 +++++-
> >  2 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index 9864813..c8989b6 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -406,7 +406,7 @@ static int sys_inotify_init1(int flags)
> >  #endif
> >  #define __NR_sys_ppoll __NR_ppoll
> >  _syscall5(int, sys_ppoll, struct pollfd *, fds, nfds_t, nfds,
> > -          struct timespec *, timeout, const __sigset_t *, sigmask,
> > +          struct timespec *, timeout, const sigset_t *, sigmask,
> >            size_t, sigsetsize)
> >  #endif
> >
> > diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> > index fdf9a47..450f71b 100644
> > --- a/linux-user/syscall_defs.h
> > +++ b/linux-user/syscall_defs.h
> > @@ -2552,12 +2552,16 @@ struct target_timer_t {
> >      abi_ulong ptr;
> >  };
> >
> > +#define TARGET_SIGEV_MAX_SIZE  64
> > +#define TARGET_SIGEV_PREAMABLE_SIZE (sizeof(int32_t) * 2 + 
> > sizeof(target_sigval_t))
> 
> This is wrong for 64 bit MIPS, I think; compare the kernel's
> MIPS-specific override:
> http://lxr.linux.no/#linux+v3.14.1/arch/mips/include/uapi/asm/siginfo.h#L13
> 
> I suggest
> 
> /* This is architecture-specific but most architectures use the default */
> #ifdef TARGET_MIPS
> #define TARGET_SIGEV_PREAMBLE_SIZE (sizeof(int32_t) * 2 + sizeof(abi_long))
> #else
> TARGET_SIGEV_PREAMBLE_SIZE (sizeof(int32_t) * 2 + sizeof(target_sigval_t))
> #endif
> 
> I'm not entirely sure this is required -- our target_sigval_t looks
> like it ought to be sizeof(abi_long) for MIPS so I don't know
> why the kernel needs this override and we apparently don't.
> Perhaps our target_sigval_t definition is slightly wrong?
> Anyway, putting in the override can't hurt and might avoid subtle
> issues later on if target_sigval_t does turn out to be broken and
> need changing...

Ok. No problem.

> 
> (Note also that 'PREAMBLE' only has one 'A'.)

whoops.

> 
> > +#define TARGET_SIGEV_PAD_SIZE  ((TARGET_SIGEV_MAX_SIZE - 
> > TARGET_SIGEV_PREAMABLE_SIZE) / sizeof(int32_t))
> 
> This line looks like it has more than 80 chars; if so, it should be
> folded. (You can check using scripts/checkpatch.pl.)

I wasn't sure of the folding rules.
 
> >  struct target_sigevent {
> >      target_sigval_t sigev_value;
> >      int32_t sigev_signo;
> >      int32_t sigev_notify;
> >      union {
> > -        int32_t _pad[ARRAY_SIZE(((struct sigevent *)0)->_sigev_un._pad)];
> > +        int32_t _pad[TARGET_SIGEV_PAD_SIZE];
> >          int32_t _tid;
> >
> >          struct {
> > --
> > 1.9.2
> 
> Looks good overall though.
> 
> thanks
> -- PMM

I'll fix and resend. Thanks for feedback.

-nc