On 1/17/26 9:51 AM, Florian Hofhammer wrote:
On 16/01/2026 22:39, Pierrick Bouvier wrote:
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.
Sounds like a good idea. This would break existing plugins though, is
this acceptable? (Asking purely out of curiosity, as I'm not familiar
yet with the processes and policies around breaking changes in QEMU
development :))
As long as we bump QEMU_PLUGIN_VERSION, this is acceptable.
Of course, the series will need to modify existing plugins to use the
new signature.
It's a bit tricky as the bool can be implicitly converted to an integer,
but it's worth fixing this for coherency sake with read/memory vaddr.
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.
gdb_read_register already takes a GByteArray, it would make sense to
also use GByteArray for gdb_write_register instead of a uint8_t* and use
the size of the array for sanity checks.
However, I'm not sure changing the return value to bool for those
internal functions makes sense, as other parts of the codebase rely on
the size information. E.g., handle_write_all_regs() in gdbstub/gdbstub.c
relies on the size returned by gdb_write_register() to know how far to
offset into the packet data it received from GDB, as GDB does not send
register size information when writing multiple registers via a 'G'
packet (just a flat byte stream with the register values concatenated).
I don't want to put the burden on you to cleanup all this kind of stuff,
so you can simply let the gdbstub refactoring out of this, and fix API
plugin only.
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.
I'd be happy to handle this!
Thanks for volunteering!
Best regards,
Florian
