Re: [PATCH v4 28/53] semihosting: Split out semihost_sys_close

[Luc Michel]

On 13:45 Tue 07 Jun     , Richard Henderson wrote:
> Split out the non-ARM specific portions of SYS_CLOSE to a
> reusable function.  This handles all GuestFD.
> 
> Note that gdb_do_syscall %x reads target_ulong, not int.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Luc Michel <lmichel@kalray.eu>

> ---
>  include/semihosting/syscalls.h |  3 +++
>  semihosting/arm-compat-semi.c  | 41 +----------------------------
>  semihosting/guestfd.c          |  7 ++++-
>  semihosting/syscalls.c         | 47 ++++++++++++++++++++++++++++++++++
>  4 files changed, 57 insertions(+), 41 deletions(-)
> 
> diff --git a/include/semihosting/syscalls.h b/include/semihosting/syscalls.h
> index 991658bf79..00e718f11d 100644
> --- a/include/semihosting/syscalls.h
> +++ b/include/semihosting/syscalls.h
> @@ -22,4 +22,7 @@ void semihost_sys_open(CPUState *cs, 
> gdb_syscall_complete_cb complete,
>                         target_ulong fname, target_ulong fname_len,
>                         int gdb_flags, int mode);
>  
> +void semihost_sys_close(CPUState *cs, gdb_syscall_complete_cb complete,
> +                        int fd);
> +
>  #endif /* SEMIHOSTING_SYSCALLS_H */
> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
> index 3239eda513..a6dddb2aa2 100644
> --- a/semihosting/arm-compat-semi.c
> +++ b/semihosting/arm-compat-semi.c
> @@ -276,7 +276,6 @@ common_semi_flen_cb(CPUState *cs, target_ulong ret, 
> target_ulong err)
>   * do the work and return the required return value to the guest
>   * via common_semi_cb.
>   */
> -typedef void sys_closefn(CPUState *cs, GuestFD *gf);
>  typedef void sys_writefn(CPUState *cs, GuestFD *gf,
>                           target_ulong buf, uint32_t len);
>  typedef void sys_readfn(CPUState *cs, GuestFD *gf,
> @@ -285,23 +284,6 @@ typedef void sys_isattyfn(CPUState *cs, GuestFD *gf);
>  typedef void sys_seekfn(CPUState *cs, GuestFD *gf, target_ulong offset);
>  typedef void sys_flenfn(CPUState *cs, GuestFD *gf);
>  
> -static void host_closefn(CPUState *cs, GuestFD *gf)
> -{
> -    int ret;
> -    /*
> -     * Only close the underlying host fd if it's one we opened on behalf
> -     * of the guest in SYS_OPEN.
> -     */
> -    if (gf->hostfd == STDIN_FILENO ||
> -        gf->hostfd == STDOUT_FILENO ||
> -        gf->hostfd == STDERR_FILENO) {
> -        ret = 0;
> -    } else {
> -        ret = close(gf->hostfd);
> -    }
> -    common_semi_cb(cs, ret, ret ? errno : 0);
> -}
> -
>  static void host_writefn(CPUState *cs, GuestFD *gf,
>                           target_ulong buf, uint32_t len)
>  {
> @@ -362,11 +344,6 @@ static void host_flenfn(CPUState *cs, GuestFD *gf)
>      }
>  }
>  
> -static void gdb_closefn(CPUState *cs, GuestFD *gf)
> -{
> -    gdb_do_syscall(common_semi_cb, "close,%x", gf->hostfd);
> -}
> -
>  static void gdb_writefn(CPUState *cs, GuestFD *gf,
>                          target_ulong buf, uint32_t len)
>  {
> @@ -414,12 +391,6 @@ static const uint8_t featurefile_data[] = {
>      SH_EXT_EXIT_EXTENDED | SH_EXT_STDOUT_STDERR, /* Feature byte 0 */
>  };
>  
> -static void staticfile_closefn(CPUState *cs, GuestFD *gf)
> -{
> -    /* Nothing to do */
> -    common_semi_cb(cs, 0, 0);
> -}
> -
>  static void staticfile_writefn(CPUState *cs, GuestFD *gf,
>                                 target_ulong buf, uint32_t len)
>  {
> @@ -468,7 +439,6 @@ static void staticfile_flenfn(CPUState *cs, GuestFD *gf)
>  }
>  
>  typedef struct GuestFDFunctions {
> -    sys_closefn *closefn;
>      sys_writefn *writefn;
>      sys_readfn *readfn;
>      sys_isattyfn *isattyfn;
> @@ -478,7 +448,6 @@ typedef struct GuestFDFunctions {
>  
>  static const GuestFDFunctions guestfd_fns[] = {
>      [GuestFDHost] = {
> -        .closefn = host_closefn,
>          .writefn = host_writefn,
>          .readfn = host_readfn,
>          .isattyfn = host_isattyfn,
> @@ -486,7 +455,6 @@ static const GuestFDFunctions guestfd_fns[] = {
>          .flenfn = host_flenfn,
>      },
>      [GuestFDGDB] = {
> -        .closefn = gdb_closefn,
>          .writefn = gdb_writefn,
>          .readfn = gdb_readfn,
>          .isattyfn = gdb_isattyfn,
> @@ -494,7 +462,6 @@ static const GuestFDFunctions guestfd_fns[] = {
>          .flenfn = gdb_flenfn,
>      },
>      [GuestFDStatic] = {
> -        .closefn = staticfile_closefn,
>          .writefn = staticfile_writefn,
>          .readfn = staticfile_readfn,
>          .isattyfn = staticfile_isattyfn,
> @@ -585,13 +552,7 @@ void do_common_semihosting(CPUState *cs)
>  
>      case TARGET_SYS_CLOSE:
>          GET_ARG(0);
> -
> -        gf = get_guestfd(arg0);
> -        if (!gf) {
> -            goto do_badf;
> -        }
> -        guestfd_fns[gf->type].closefn(cs, gf);
> -        dealloc_guestfd(arg0);
> +        semihost_sys_close(cs, common_semi_cb, arg0);
>          break;
>  
>      case TARGET_SYS_WRITEC:
> diff --git a/semihosting/guestfd.c b/semihosting/guestfd.c
> index 7ac2e147a8..e3122ebba9 100644
> --- a/semihosting/guestfd.c
> +++ b/semihosting/guestfd.c
> @@ -49,6 +49,11 @@ int alloc_guestfd(void)
>      return i;
>  }
>  
> +static void do_dealloc_guestfd(GuestFD *gf)
> +{
> +    gf->type = GuestFDUnused;
> +}
> +
>  /*
>   * Look up the guestfd in the data structure; return NULL
>   * for out of bounds, but don't check whether the slot is unused.
> @@ -119,5 +124,5 @@ void dealloc_guestfd(int guestfd)
>      GuestFD *gf = do_get_guestfd(guestfd);
>  
>      assert(gf);
> -    gf->type = GuestFDUnused;
> +    do_dealloc_guestfd(gf);
>  }
> diff --git a/semihosting/syscalls.c b/semihosting/syscalls.c
> index 9f9d19a59a..3648b9dd49 100644
> --- a/semihosting/syscalls.c
> +++ b/semihosting/syscalls.c
> @@ -94,6 +94,12 @@ static void gdb_open(CPUState *cs, gdb_syscall_complete_cb 
> complete,
>                     fname, len, (target_ulong)gdb_flags, (target_ulong)mode);
>  }
>  
> +static void gdb_close(CPUState *cs, gdb_syscall_complete_cb complete,
> +                      GuestFD *gf)
> +{
> +    gdb_do_syscall(complete, "close,%x", (target_ulong)gf->hostfd);
> +}
> +
>  /*
>   * Host semihosting syscall implementations.
>   */
> @@ -140,6 +146,23 @@ static void host_open(CPUState *cs, 
> gdb_syscall_complete_cb complete,
>      unlock_user(p, fname, 0);
>  }
>  
> +static void host_close(CPUState *cs, gdb_syscall_complete_cb complete,
> +                       GuestFD *gf)
> +{
> +    /*
> +     * Only close the underlying host fd if it's one we opened on behalf
> +     * of the guest in SYS_OPEN.
> +     */
> +    if (gf->hostfd != STDIN_FILENO &&
> +        gf->hostfd != STDOUT_FILENO &&
> +        gf->hostfd != STDERR_FILENO &&
> +        close(gf->hostfd) < 0) {
> +        complete(cs, -1, errno);
> +    } else {
> +        complete(cs, 0, 0);
> +    }
> +}
> +
>  /*
>   * Syscall entry points.
>   */
> @@ -154,3 +177,27 @@ void semihost_sys_open(CPUState *cs, 
> gdb_syscall_complete_cb complete,
>          host_open(cs, complete, fname, fname_len, gdb_flags, mode);
>      }
>  }
> +
> +void semihost_sys_close(CPUState *cs, gdb_syscall_complete_cb complete, int 
> fd)
> +{
> +    GuestFD *gf = get_guestfd(fd);
> +
> +    if (!gf) {
> +        complete(cs, -1, EBADF);
> +        return;
> +    }
> +    switch (gf->type) {
> +    case GuestFDGDB:
> +        gdb_close(cs, complete, gf);
> +        break;
> +    case GuestFDHost:
> +        host_close(cs, complete, gf);
> +        break;
> +    case GuestFDStatic:
> +        complete(cs, 0, 0);
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +    dealloc_guestfd(fd);
> +}
> -- 
> 2.34.1
>