Hi Ziyang, On 6/12/2026 5:48 AM, Ziyang Zhang wrote: > On Fri, 12 Jun 2026 11:43:57 +0100, Alex Bennée wrote: >> All of the arguments here are abi_long which go to int32_t or >> target_long->int64_t so perhaps we should be using that for all args to >> ensure signedness is correct? > You're right that abi_long is signed. I only changed the return value to > keep this callback consistent with the two syscall callbacks that > existed from the first version of the plugin syscall API: > qemu_plugin_vcpu_syscall_cb_t and qemu_plugin_vcpu_syscall_ret_cb_t. > Both use int64_t for num and int64_t for the return value, while the > entry callback passes a1..a8 as uint64_t. This patch matches that. > > I'd prefer to keep the arguments as uint64_t. They don't share a single > signedness: pointers and size_t are unsigned, while offsets and fds are > signed. At this boundary they are just register-width words that the > handler casts per syscall, so the correct extension is per-argument-type > (glibc handles them that way too [1]), and forcing them all to int64_t > would be wrong for pointer arguments in particular. > > Changing the whole argument vector would also touch several APIs and all > their callers, which is well beyond the scope of this small signedness > fix. So I'd rather keep it to the return value, the one part of this > interface with a well-defined signed meaning. > > [1] https://github.com/bminor/glibc/blob/master/sysdeps/unix/sysv/linux/ > x86_64/x32/sysdep.h#L50 > > Thanks, > Ziyang Zhang >
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
