Re: [PATCH] target/arm/arm-semi: fix SYS_OPEN to return nonzero filehandle

[Peter Maydell <peter.maydell@linaro.org>]

On Thu, 9 Jan 2020 at 04:13, Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> According to the specification "Semihosting for AArch32 and Aarch64",
> the SYS_OPEN operation should return:
>
>  - A nonzero handle if the call is successful
>  - -1 if the call is not successful
>
> So, it should never return 0.
>
> Prior to commit 35e9a0a8ce4b ("target/arm/arm-semi: Make semihosting
> code hand out its own file descriptors"), the guest fd matched to the
> host fd. It returned a nonzero handle on success since the fd 0 is
> already used for stdin.

I think this bug existed even prior to that commit, because
in the old implementation we would handle the ":tt" magic
file by returning STDIN_FILENO or STDOUT_FILENO, and
STDIN_FILENO is zero.

So although I agree we should fix this bug, it would probably
be wise if your code using the API treated 0 as a success,
because QEMU's probably not the only implementation that
decided to use "just pass through the host fd"...

> Basically, there are two ways to fix this:
>
>   - Use (guestfd - 1) as the index of guestfs_arrary. We need to insert
>     increment/decrement to convert the guestfd and the array index back
>     and forth.
>
>   - Keep using guestfd as the index of guestfs_array. The first entry
>     of guestfs_array is left unused.
>
> I thought the latter is simpler. We end up with wasting a small piece
> of memory for the unused first entry of guestfd_array, but this is
> probably not a big deal.

Yeah, I guess so.

Applied to target-arm.next.

(This also reminds me that I never got round to fixing a bug where
if the guest does a SYS_OPEN on :tt and then a SYS_CLOSE then
we close the host stdin/stdout, which we should not...)

thanks
-- PMM