Florian Hofhammer <[email protected]> writes: > 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 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).
We don't need to expose internal implementation details to the plugins. If we have length already as part of GByteArray then returning a bool for success/fail makes sense. Its the more modern pattern throughout QEMU anyway. > >> 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! > > Best regards, > Florian -- Alex Bennée Virtualisation Tech Lead @ Linaro
