Re: [Qemu-devel] [PATCH] add semihosting-config option

[Peter Maydell]

On 18 November 2014 19:00, Liviu Ionescu <address@hidden> wrote:
> - allow to define where the semihosting calls will be addressed
>
> Signed-off-by: Liviu Ionescu <address@hidden>
> ---
>  gdbstub.c               | 12 ++++++++++++
>  include/sysemu/sysemu.h |  6 ++++++
>  qemu-options.hx         | 12 +++++++++++-
>  vl.c                    | 49 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 78 insertions(+), 1 deletion(-)

Thanks for sending this as a standalone patch. It looks pretty good,
there are just a few nits:

>
> diff --git a/gdbstub.c b/gdbstub.c
> index 0faca56..372aa67 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -355,6 +355,18 @@ static enum {
>     remote gdb syscalls.  Otherwise use native file IO.  */

The comment here ^^ is now out of date...

>  int use_gdb_syscalls(void)
>  {
> +    if (semihosting_target == SEMIHOSTING_TARGET_NATIVE) {
> +        if (gdb_syscall_mode == GDB_SYS_UNKNOWN) {
> +            gdb_syscall_mode = GDB_SYS_DISABLED;
> +        }
> +        return FALSE;
> +    } else if (semihosting_target == SEMIHOSTING_TARGET_GDB) {
> +        if (gdb_syscall_mode == GDB_SYS_UNKNOWN) {
> +            gdb_syscall_mode = GDB_SYS_ENABLED;
> +        }

...and there isn't much point in these if()s I think
since we will never look at gdb_syscall_mode if
the semihosting_target is not auto; so we needn't set it.

> +        return TRUE;

Also we prefer 'true' and 'false' to TRUE and FALSE.

If you like I can just fix these minor things and put the
patch into my target-arm queue, or you could reroll the patch
yourself if you prefer that.
(The change won't hit master until after we release 2.2 in early
December, so there's no immediate hurry either way.)

(PS: if you're sending a single standalone patch you don't
need to send a cover letter mail as well; the cover letter
is really for providing an overall description of a
multi-patch email series.)

-- PMM