Re: [Qemu-devel] [PATCH v2 02/15] target/arm/arm-semi: Always set some kind of errno for failed calls

[Philippe Mathieu-Daudé]

On 9/16/19 4:15 PM, Peter Maydell wrote:
> If we fail a semihosting call we should always set the
> semihosting errno to something; we were failing to do
> this for some of the "check inputs for sanity" cases.
>
> Signed-off-by: Peter Maydell <address@hidden>
> Reviewed-by: Alex Bennée <address@hidden>

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

> ---
>   target/arm/arm-semi.c | 45 ++++++++++++++++++++++++++-----------------
>   1 file changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index 03e60105c05..51b55816faf 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -232,11 +232,13 @@ static target_ulong arm_gdb_syscall(ARMCPU *cpu, 
> gdb_syscall_complete_cb cb,
>   #define GET_ARG(n) do {                                 \
>       if (is_a64(env)) {                                  \
>           if (get_user_u64(arg ## n, args + (n) * 8)) {   \
> -            return -1;                                  \
> +            errno = EFAULT;                             \
> +            return set_swi_errno(ts, -1);               \
>           }                                               \
>       } else {                                            \
>           if (get_user_u32(arg ## n, args + (n) * 4)) {   \
> -            return -1;                                  \
> +            errno = EFAULT;                             \
> +            return set_swi_errno(ts, -1);               \
>           }                                               \
>       }                                                   \
>   } while (0)
> @@ -287,12 +289,13 @@ target_ulong do_arm_semihosting(CPUARMState *env)
>           GET_ARG(2);
>           s = lock_user_string(arg0);
>           if (!s) {
> -            /* FIXME - should this error code be -TARGET_EFAULT ? */
> -            return (uint32_t)-1;
> +            errno = EFAULT;
> +            return set_swi_errno(ts, -1);
>           }
>           if (arg1 >= 12) {
>               unlock_user(s, arg0, 0);
> -            return (uint32_t)-1;
> +            errno = EINVAL;
> +            return set_swi_errno(ts, -1);
>           }
>           if (strcmp(s, ":tt") == 0) {
>               int result_fileno = arg1 < 4 ? STDIN_FILENO : STDOUT_FILENO;
> @@ -413,8 +416,8 @@ target_ulong do_arm_semihosting(CPUARMState *env)
>           } else {
>               s = lock_user_string(arg0);
>               if (!s) {
> -                /* FIXME - should this error code be -TARGET_EFAULT ? */
> -                return (uint32_t)-1;
> +                errno = EFAULT;
> +                return set_swi_errno(ts, -1);
>               }
>               ret =  set_swi_errno(ts, remove(s));
>               unlock_user(s, arg0, 0);
> @@ -432,11 +435,12 @@ target_ulong do_arm_semihosting(CPUARMState *env)
>               char *s2;
>               s = lock_user_string(arg0);
>               s2 = lock_user_string(arg2);
> -            if (!s || !s2)
> -                /* FIXME - should this error code be -TARGET_EFAULT ? */
> -                ret = (uint32_t)-1;
> -            else
> +            if (!s || !s2) {
> +                errno = EFAULT;
> +                ret = set_swi_errno(ts, -1);
> +            } else {
>                   ret = set_swi_errno(ts, rename(s, s2));
> +            }
>               if (s2)
>                   unlock_user(s2, arg2, 0);
>               if (s)
> @@ -456,8 +460,8 @@ target_ulong do_arm_semihosting(CPUARMState *env)
>           } else {
>               s = lock_user_string(arg0);
>               if (!s) {
> -                /* FIXME - should this error code be -TARGET_EFAULT ? */
> -                return (uint32_t)-1;
> +                errno = EFAULT;
> +                return set_swi_errno(ts, -1);
>               }
>               ret = set_swi_errno(ts, system(s));
>               unlock_user(s, arg0, 0);
> @@ -517,19 +521,22 @@ target_ulong do_arm_semihosting(CPUARMState *env)
>   
>               if (output_size > input_size) {
>                   /* Not enough space to store command-line arguments.  */
> -                return -1;
> +                errno = E2BIG;
> +                return set_swi_errno(ts, -1);
>               }
>   
>               /* Adjust the command-line length.  */
>               if (SET_ARG(1, output_size - 1)) {
>                   /* Couldn't write back to argument block */
> -                return -1;
> +                errno = EFAULT;
> +                return set_swi_errno(ts, -1);
>               }
>   
>               /* Lock the buffer on the ARM side.  */
>               output_buffer = lock_user(VERIFY_WRITE, arg0, output_size, 0);
>               if (!output_buffer) {
> -                return -1;
> +                errno = EFAULT;
> +                return set_swi_errno(ts, -1);
>               }
>   
>               /* Copy the command-line arguments.  */
> @@ -544,7 +551,8 @@ target_ulong do_arm_semihosting(CPUARMState *env)
>   
>               if (copy_from_user(output_buffer, ts->info->arg_start,
>                                  output_size)) {
> -                status = -1;
> +                errno = EFAULT;
> +                status = set_swi_errno(ts, -1);
>                   goto out;
>               }
>   
> @@ -614,7 +622,8 @@ target_ulong do_arm_semihosting(CPUARMState *env)
>   
>                   if (fail) {
>                       /* Couldn't write back to argument block */
> -                    return -1;
> +                    errno = EFAULT;
> +                    return set_swi_errno(ts, -1);
>                   }
>               }
>               return 0;