Re: [PULL 1/1] linux-user: Support futex_time64

[Alistair Francis]

On Mon, Nov 2, 2020 at 11:29 PM Laurent Vivier <laurent@vivier.eu> wrote:
>
> Le 02/11/2020 à 19:15, Peter Maydell a écrit :
> > On Mon, 30 Mar 2020 at 11:31, Laurent Vivier <laurent@vivier.eu> wrote:
> >>
> >> From: Alistair Francis <alistair.francis@wdc.com>
> >>
> >> Add support for host and target futex_time64. If futex_time64 exists on
> >> the host we try that first before falling back to the standard futex
> >> syscall.
> >
> > Hi; I dunno why Coverity's only just noticed this, but in
> > CID 1432339 it points out:
> >
> >> +#if defined(TARGET_NR_futex_time64)
> >> +static int do_futex_time64(target_ulong uaddr, int op, int val, 
> >> target_ulong timeout,
> >> +                           target_ulong uaddr2, int val3)
> >> +{
> >> +    struct timespec ts, *pts;
> >> +    int base_op;
> >> +
> >> +    /* ??? We assume FUTEX_* constants are the same on both host
> >> +       and target.  */
> >> +#ifdef FUTEX_CMD_MASK
> >> +    base_op = op & FUTEX_CMD_MASK;
> >> +#else
> >> +    base_op = op;
> >> +#endif
> >> +    switch (base_op) {
> >> +    case FUTEX_WAIT:
> >> +    case FUTEX_WAIT_BITSET:
> >> +        if (timeout) {
> >> +            pts = &ts;
> >> +            target_to_host_timespec64(pts, timeout);
> >
> > ...that here we call target_to_host_timespec64(), which can
> > fail with -TARGET_EFAULT, but (unlike all the other times we call
> > the function) we aren't checking its return value.
> > Is there missing error handling code here ?
> >
>
> I think the code is like that because this is a cut&paste of function
> do_futex() witl "s/timespec/timespec64/".
>
> And yes I think we should check for the return value.
> I'm going to fix that.

Thanks! Let me know if you want me to do it and I can send a patch instead.

Alistair

>
> Thanks,
> Laurent
>