On Fri, Aug 08, 2025 at 10:08:12AM +0200, Markus Armbruster wrote:
> HMP command gdbserver used to emit two error messages for certain
> errors. For instance, with -M none:
>
> (qemu) gdbserver
> gdbstub: meaningless to attach gdb to a machine without any CPU.
> Could not open gdbserver on device 'tcp::1234'
>
> The first message is the specific error, and the second one a generic
> additional message that feels superfluous to me.
>
> Commit c0e6b8b798b (system: propagate Error to gdbserver_start (and
> other device setups)) turned the first message into a warning:
>
> warning: gdbstub: meaningless to attach gdb to a machine without any CPU.
> Could not open gdbserver on device 'tcp::1234'
>
> This is arguably worse.
>
> hmp_gdbserver() passes &error_warn to gdbserver_start(), so that
> failure gets reported as warning, and then additionally emits the
> generic error on failure. This is a misuse of &error_warn.
>
> Instead, receive the error in &err and report it, as usual. With
> this, gdbserver reports just the error:
>
> gdbstub: meaningless to attach gdb to a machine without any CPU.
What do you think about an alternative of removing "gdbstub: "
from all the errors raised in 'gdbserver_start' & similar, and
then using
error_prepend(err, "Could not open gdbserver on device '%s'", device);
in hmp_gdbserver ?
I don't feel too strongly, so in any case for the patch as is,
Reviewed-by: Daniel P. Berrangé <berra...@redhat.com>
>
> Cc: Alex Bennée <alex.ben...@linaro.org>
> Signed-off-by: Markus Armbruster <arm...@redhat.com>
> ---
> include/exec/gdbstub.h | 3 ---
> monitor/hmp-cmds.c | 7 ++++---
> 2 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> index a16c0051ce..bd7182c4d3 100644
> --- a/include/exec/gdbstub.h
> +++ b/include/exec/gdbstub.h
> @@ -55,9 +55,6 @@ void gdb_unregister_coprocessor_all(CPUState *cpu);
> * system emulation you can use a full chardev spec for your gdbserver
> * port.
> *
> - * The error handle should be either &error_fatal (for start-up) or
> - * &error_warn (for QMP/HMP initiated sessions).
> - *
> * Returns true when server successfully started.
> */
> bool gdbserver_start(const char *port_or_device, Error **errp);
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 74a0f56566..33a88ce205 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -280,14 +280,15 @@ void hmp_log(Monitor *mon, const QDict *qdict)
>
> void hmp_gdbserver(Monitor *mon, const QDict *qdict)
> {
> + Error *err = NULL;
> const char *device = qdict_get_try_str(qdict, "device");
> +
> if (!device) {
> device = "tcp::" DEFAULT_GDBSTUB_PORT;
> }
>
> - if (!gdbserver_start(device, &error_warn)) {
> - monitor_printf(mon, "Could not open gdbserver on device '%s'\n",
> - device);
> + if (!gdbserver_start(device, &err)) {
> + error_report_err(err);
> } else if (strcmp(device, "none") == 0) {
> monitor_printf(mon, "Disabled gdbserver\n");
> } else {
> --
> 2.49.0
>
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
