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

[Peter Maydell]

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?

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

> 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...

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

> +#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.)

>  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