On 1/16/26 10:48 AM, Florian Hofhammer wrote:
On 16/01/2026 19:24, Pierrick Bouvier wrote:
In practice, it may return anything else than 0 (see arm_cpu_gdb_write_register
for instance).
So the right (vague) description should be:
On success returns 0.
Hmm, it seems to me as if the code is a bit inconsistent here: the
plugin API in plugins/api.c returns -1 if it detects an error directly,
and the arm_cpu_gdb_write_register() (but it's similar for other archs,
e.g., x86_cpu_gdb_write_register()) returns 0 if the register is unknown
and the number of bytes written otherwise (in the arm example: 4 for the
general-purpose registers).
That means that currently, both -1 and 0 as return value indicate an
error.
Thanks for the catch, that made me dig into the actual gdbstub code a
bit more!
Indeed, same for me. I've been reading too quick when answering through
your first email, and missed the nuance.
In order to make this consistent, there are two options I see:
1) Change the plugin API function to return 0 on error (but then it's
inconsistent with the qemu_plugin_read_register() function which returns
-1 on error), or
2) Change the arch-specific gdbstub functions to return -1 on error
instead of 0.
What do you think? I'd be happy to prepare a patch for either option.>
For sake of consistency, we should make this use the same interface than
{read,write}_memory_vaddr, minus the len param.
bool qemu_plugin_read_memory_vaddr(uint64_t addr,
GByteArray *data, size_t len);
bool qemu_plugin_write_memory_vaddr(uint64_t addr,
GByteArray *data);
So it would be:
bool qemu_plugin_read_register(uint64_t addr, GByteArray *data);
bool qemu_plugin_write_register(uint64_t addr, GByteArray *data);
This is better and unambiguous, as no one needs a documentation to know
what a bool return is, and data already holds the size information.
As well, writing this, I realized that existing write_register is broken
by design: we never check the size of data array (except > 1) and
blindly an arbitrary amount of memory from it, which is wrong.
Even though the doc mentions it, we should just fix it, detect when size
< reg_size, and return false.
This comes from the fact gdb_write_register itself has no notion of size
and trust the pointer. so it would need another refactor also. And while
at it, change gdb_{read,write}_register definition to return bool also.
Best regards,
Florian
I think it pushes a lot of changes just for a simple comment change, so
I would understand if you don't want to do this whole refactoring beyond
plugins interface. I can pick it up, or let you work on it if you have
time/interest. Feel free to let us know.
In all cases, thank you for pointing this.
Regards,
Pierrick
