Re: [PATCH 01/12] monitor: Clean up HMP gdbserver error reporting

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> 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 ?

This would change the error message from

    (qemu) gdbserver 
    gdbstub: meaningless to attach gdb to a machine without any CPU.

to

    (qemu) gdbserver 
    Could not open gdbserver on device 'tcp::1234': meaningless to attach gdb to a machine without any CPU.

Longer, but doesn't tell me anything new beyond reminding me that
gdbserver's argument defaults to "tcp::1234".

This instance of gdbserver_start() is additionally called by
qemu_machine_creation_done() in system/vl.c on behalf of command line
option gdb.  The error message would change from

    $ qemu-system-x86_64 -M none -gdb ""
    qemu-system-x86_64: -gdb : gdbstub: meaningless to attach gdb to a machine without any CPU.

to

    $ qemu-system-x86_64 -M none -gdb ""
    qemu-system-x86_64: -gdb : meaningless to attach gdb to a machine without any CPU.

Improvement.  Howver, we better drop the prefix from all error messages
emitted here, not just this one.

There's another instance of gdbserver_start() in gdbstub/user.c, called
by main() in bsduser/main.c and in linux-user/main.c on behalf of
command line option -g / -gdb.  Same silly prefix:

    $ qemu-x86_64 -g x,y /bin/ls
    qemu-x86_64: gdbstub: unknown option "y"
    Usage: -g {port|path}[,suspend={y|n}]

> I don't feel too strongly, so in any case for the patch as is,

Separate patch to strip "gdbstub: " from all error messages?

> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Thanks!