Re: [Qemu-devel] [RFC PATCH 08/11] target/arm: correct return values for WRITE/READ in arm-semi

[Philippe Mathieu-Daudé]

On 5/14/19 5:52 PM, Alex Bennée wrote:
> The documentation says the write should return the number of bytes not
> written on an error (0 means everything was written). Read provides a
> buffer length and the return value should be the buffer length - bytes
> actually read. Remove the incorrect FIXME's and return the correct
> values.
> 
> Signed-off-by: Alex Bennée 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  target/arm/arm-semi.c | 20 
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
> index a3bbef18ef7..3ae8f05d51b 100644
> --- a/target/arm/arm-semi.c
> +++ b/target/arm/arm-semi.c
> @@ -334,13 +334,15 @@ target_ulong do_arm_semihosting(CPUARMState *env)
>  } else {
>  s = lock_user(VERIFY_READ, arg1, len, 1);
>  if (!s) {
> -/* FIXME - should this error code be -TARGET_EFAULT ? */
> -return (uint32_t)-1;
> +/* Return bytes not written on error */
> +return len;
>  }
>  ret = set_swi_errno(ts, write(arg0, s, len));
>  unlock_user(s, arg1, 0);
> -if (ret == (uint32_t)-1)
> -return -1;
> +if (ret == (uint32_t)-1) {
> +ret = 0;
> +}
> +/* Return bytes not written */
>  return len - ret;
>  }
>  case TARGET_SYS_READ:
> @@ -355,15 +357,17 @@ target_ulong do_arm_semihosting(CPUARMState *env)
>  } else {
>  s = lock_user(VERIFY_WRITE, arg1, len, 0);
>  if (!s) {
> -/* FIXME - should this error code be -TARGET_EFAULT ? */
> -return (uint32_t)-1;
> +/* return bytes not read */
> +return len;
>  }
>  do {
>  ret = set_swi_errno(ts, read(arg0, s, len));
>  } while (ret == -1 && errno == EINTR);
>  unlock_user(s, arg1, len);
> -if (ret == (uint32_t)-1)
> -return -1;
> +if (ret == (uint32_t)-1) {
> +ret = 0;
> +}
> +/* Return bytes not read */
>  return len - ret;
>  }
>  case TARGET_SYS_READC:
> 

[Qemu-devel] [RFC PATCH 08/11] target/arm: correct return values for WRITE/READ in arm-semi

[Alex Bennée]

The documentation says the write should return the number of bytes not
written on an error (0 means everything was written). Read provides a
buffer length and the return value should be the buffer length - bytes
actually read. Remove the incorrect FIXME's and return the correct
values.

Signed-off-by: Alex Bennée 
---
 target/arm/arm-semi.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c
index a3bbef18ef7..3ae8f05d51b 100644
--- a/target/arm/arm-semi.c
+++ b/target/arm/arm-semi.c
@@ -334,13 +334,15 @@ target_ulong do_arm_semihosting(CPUARMState *env)
 } else {
 s = lock_user(VERIFY_READ, arg1, len, 1);
 if (!s) {
-/* FIXME - should this error code be -TARGET_EFAULT ? */
-return (uint32_t)-1;
+/* Return bytes not written on error */
+return len;
 }
 ret = set_swi_errno(ts, write(arg0, s, len));
 unlock_user(s, arg1, 0);
-if (ret == (uint32_t)-1)
-return -1;
+if (ret == (uint32_t)-1) {
+ret = 0;
+}
+/* Return bytes not written */
 return len - ret;
 }
 case TARGET_SYS_READ:
@@ -355,15 +357,17 @@ target_ulong do_arm_semihosting(CPUARMState *env)
 } else {
 s = lock_user(VERIFY_WRITE, arg1, len, 0);
 if (!s) {
-/* FIXME - should this error code be -TARGET_EFAULT ? */
-return (uint32_t)-1;
+/* return bytes not read */
+return len;
 }
 do {
 ret = set_swi_errno(ts, read(arg0, s, len));
 } while (ret == -1 && errno == EINTR);
 unlock_user(s, arg1, len);
-if (ret == (uint32_t)-1)
-return -1;
+if (ret == (uint32_t)-1) {
+ret = 0;
+}
+/* Return bytes not read */
 return len - ret;
 }
 case TARGET_SYS_READC:
-- 
2.20.1