On Thu, 5 Aug 2021 at 21:34, johannst <johannes.sto...@googlemail.com> wrote:
>
> Dear all,
>
> in my opinion the `type` argument in the kvm ioctl wrappers should be of
> type unsigned. Please correct me if I am wrong.
(Ccing Eric as our resident POSIX expert.)
> Due to the same reason as explained in the comment on the
> `irq_set_ioctl` field in `struct KVMState` (accel/kvm/kvm-all.c),
> the kvm ioctl wrapper should take `type` as an unsigned.
The reason in that comment:
/* The man page (and posix) say ioctl numbers are signed int, but
* they're not. Linux, glibc and *BSD all treat ioctl numbers as
* unsigned, and treating them as signed here can break things */
It would be more helpful to readers to state the reason directly
in the commit message, rather than requiring them to go and look
up a comment in some other file.
(That comment, incidentally, seems to be no longer completely
true: on my system the ioctl manpage says 'unsigned long', though
the glibc info docs say 'int', in contradiction to the ioctl.h
glibc actually ships...)
Of the various KVM_* ioctls we use via these functions, do
any actually have values that would result in invalid sign
extension here ? That is, is this fixing an existing bug, or is
it merely avoiding a potential future bug?
> Signed-off-by: johannst <johannes.sto...@gmail.com>
> ---
> accel/kvm/kvm-all.c | 8 ++++----
> accel/kvm/trace-events | 8 ++++----
> include/sysemu/kvm.h | 8 ++++----
> 3 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 0125c17edb..45cd6edce3 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2967,7 +2967,7 @@ int kvm_cpu_exec(CPUState *cpu)
> return ret;
> }
>
> -int kvm_ioctl(KVMState *s, int type, ...)
> +int kvm_ioctl(KVMState *s, unsigned type, ...)
The underlying ioctl() prototype (at least in my Linux /usr/include/sys/ioctl.h
and as documented in the ioctl(2) manpage) uses "unsigned long" for the
request argument; should we do the same ?
-- PMM
