The return value in the docstring did not match the return value in the
code (see plugins/api.c).

Signed-off-by: Florian Hofhammer <florian.hofhammer@epfl.ch>
---
 include/qemu/qemu-plugin.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 1f25fb2b40..def6693a17 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -1002,7 +1002,7 @@ int qemu_plugin_read_register(struct qemu_plugin_register *handle,
  * Attempting to write a register with @buf smaller than the register size
  * will result in a crash or other undesired behavior.
  *
- * Returns the number of bytes written. On failure returns 0.
+ * Returns the number of bytes written. On failure returns -1.
  */
 QEMU_PLUGIN_API
 int qemu_plugin_write_register(struct qemu_plugin_register *handle,
-- 
2.51.0

On 1/16/26 9:38 AM, Florian Hofhammer wrote:
> The return value in the docstring did not match the return value in the
> code (see plugins/api.c).
> 
> Signed-off-by: Florian Hofhammer <florian.hofhammer@epfl.ch>
> ---
>   include/qemu/qemu-plugin.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index 1f25fb2b40..def6693a17 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -1002,7 +1002,7 @@ int qemu_plugin_read_register(struct qemu_plugin_register *handle,
>    * Attempting to write a register with @buf smaller than the register size
>    * will result in a crash or other undesired behavior.
>    *
> - * Returns the number of bytes written. On failure returns 0.
> + * Returns the number of bytes written. On failure returns -1.
>    */
>   QEMU_PLUGIN_API
>   int qemu_plugin_write_register(struct qemu_plugin_register *handle,

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.

Regards,
Pierrick

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!

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.

Best regards,
Florian

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

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).

> 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

Florian Hofhammer <florian.hofhammer@epfl.ch> 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

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

On 1/16/26 1:39 PM, Pierrick Bouvier wrote:
> 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);

Meant:

bool qemu_plugin_read_register(struct qemu_plugin_register *handle,  	 

                                GByteArray *data);
bool qemu_plugin_write_register(struct qemu_plugin_register *handle,
                                 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

On 1/16/26 1:39 PM, Pierrick Bouvier wrote:
> 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.
>

I reviewed that code by the way, so definitely my fault I let it through.

On 16/1/26 18:38, Florian Hofhammer wrote:
> The return value in the docstring did not match the return value in the
> code (see plugins/api.c).
> 
> Signed-off-by: Florian Hofhammer <florian.hofhammer@epfl.ch>
> ---
>   include/qemu/qemu-plugin.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>