Re: [PATCH v0] kvm: unsigned datatype in ioctl wrapper

[Peter Maydell]

On Thu, 5 Aug 2021 at 21:34, johannst <johannes.stoelp@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.stoelp@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