Re: [Qemu-devel] [PATCH] ioctl() numbers are unsigned (the man page lies)

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 2279 bytes --]

Looks like the subject would warmly welcome a "kvm:" tag.

On 2012-02-09 05:22, David Gibson wrote:
> In kvm-all.c we store an ioctl cmd number in the irqchip_inject_ioctl field
> of KVMState, which has type 'int'.  This seems to make sense since the
> ioctl() man page says that the cmd parameter has type int.
> 
> However, the kernel treats ioctl numbers as unsigned - sys_ioctl() takes an
> unsigned int, and the macros which generate ioctl numbers expand to
> unsigned expressions.  Furthermore, some ioctls (IOC_READ ioctls on x86
> and IOC_WRITE ioctls on powerpc) have bit 31 set, and so would be negative
> if interpreted as an int. This has the surprising and compile-breaking
> consequence that in kvm_irqchip_set_irq() where we do:
>     return (s->irqchip_inject_ioctl == KVM_IRQ_LINE) ? 1 : event.status;
> We will get a "comparison is always false due to limited range of data
> type" warning from gcc if KVM_IRQ_LINE is one of the bit-31-set ioctls,
> which it is on powerpc.
> 
> So, despite the fact that the man page and posix say ioctl numbers are
> signed, they're actually unsigned.  The kernel uses unsigned, the glibc
> header uses unsigned long, and FreeBSD, NetBSD and OSX also use unsigned
> long ioctl numbers in the code.
> 
> Therefore, this patch changes the variable to be unsigned, fixing the
> compile.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  kvm-all.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index 0b87658..681ad15 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -78,7 +78,10 @@ struct KVMState
>      int pit_in_kernel;
>      int xsave, xcrs;
>      int many_ioeventfds;
> -    int irqchip_inject_ioctl;
> +    /* 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 */

What about naming the problem instead:

/* Comparison with IOCTL macros on 32-bit hosts requires unsigned. */

> +    unsigned irqchip_inject_ioctl;
>  #ifdef KVM_CAP_IRQ_ROUTING
>      struct kvm_irq_routing *irq_routes;
>      int nr_allocated_irq_routes;

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]