On 6/15/2026 9:10 PM, Ziyang Zhang wrote: > Hi Pierrick, > > On Mon, 15 Jun 2026 10:46:16 -0700, Pierrick Bouvier wrote: >> We can't expose target dependent types, so having unsigned types for >> args and ret was the correct thing to do. This way, a plugin can decide >> to use any bit pattern, and upper part is discarded if needed. >> >> For args, we explicitly clamp values depending on target: >> https://gitlab.com/qemu-project/qemu/-/blob/master/plugins/core.c? >> ref_type=heads#L554 >> >> For ret, the type conversion is handled by writing ret to a temporary >> variable, before sending it back: >> https://gitlab.com/qemu-project/qemu/-/blob/master/linux-user/ >> syscall.c?ref_type=heads#L14622 >> >> Indeed, there is an inconsistency between syscall_ret and syscall_filter >> on that. However, they do different things: >> - syscall_ret will read the value, so having a signed is mandatory. >> `if ret < 0` needs to work. >> - syscall_filter will only set it, so having a signed in not mandatory. >> `*ret = -1` still does what is expected. >> >> From that, I'm not sure any change is needed, but feel free to point a >> use case I missed where this matter for syscall filter. >> >> Regards, >> Pierrick > > You're right, and thanks for the detailed explanation. There is no > correctness issue here: the filter only sets sysret, and *sysret = -1 > behaves the same whether the pointer is signed or unsigned, so nothing > is functionally broken. > > My concern is purely about consistency. The syscall return value appears > in both callbacks: qemu_plugin_vcpu_syscall_filter_cb_t passes it as a > writable sysret and qemu_plugin_vcpu_syscall_ret_cb_t passes it as a > readable value, but in both it is the same kind of thing, a syscall > return value, whose semantics are inherently signed (the negative range > encodes errno). Having the write side use uint64_t while the read side > uses int64_t looks surprising to a plugin user, since it is the same > signed quantity in both places. Keeping the two consistent makes the > intent clearer, and as a bonus lets a filter write *sysret = -EINVAL > without an implicit signed to unsigned conversion. > > As you say this is not mandatory, so I am happy to drop it if you would > rather leave it as is. It just seemed like a small readability > improvement given the two callbacks describe the same value. >
That's convincing enough for me, and yes, consistency matters :). I'll pull today this other series: https://lore.kernel.org/qemu-devel/[email protected]/ Would that be ok for you to wait for it to come down (hopefully before end of the week), and then publish a new version of yours rebased on top? I can ping you once it's merged if you want. > Regards, > Ziyang Zhang > Regards, Pierrick
