Re: [PATCH v4 27/53] semihosting: Split out semihost_sys_open

[Richard Henderson]

On 6/22/22 02:35, Luc Michel wrote:
> On 13:45 Tue 07 Jun     , Richard Henderson wrote:
> > Split out the non-ARM specific portions of SYS_OPEN to a
> > reusable function.  This handles gdb and host file i/o.
> >
> > Add helpers to validate the length of the filename string.
> > Prepare for usage by other semihosting by allowing the
> > filename length parameter to be 0, and calling strlen.
> >
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> >   include/semihosting/syscalls.h |  25 ++++++
> >   semihosting/arm-compat-semi.c  |  52 ++---------
> >   semihosting/guestfd.c          |   5 ++
> >   semihosting/syscalls.c         | 156 +++++++++++++++++++++++++++++++++
> >   semihosting/meson.build        |   1 +
> >   5 files changed, 193 insertions(+), 46 deletions(-)
> >   create mode 100644 include/semihosting/syscalls.h
> >   create mode 100644 semihosting/syscalls.c
>
> [...]
>
> >           } else {
> > -            hostfd = open(s, open_modeflags[arg1], 0644);
> > -            if (hostfd < 0) {
> > -                ret = -1;
> > -                err = errno;
> > -            } else {
> > -                ret = alloc_guestfd();
> > -                associate_guestfd(ret, hostfd);
> > -            }
> > +            semihost_sys_open(cs, common_semi_cb, arg0, arg2 + 1,
> > +                              gdb_open_modeflags[arg1], 0644);
>
> You're missing a unlock_user(s, arg0, 0); here

Good catch.  Fixed.


r~