Re: [Qemu-devel] [PATCH] block/raw-posix: Linux compat-ioctl warning workaround

[Kevin Wolf]

Am 29.06.2011 15:36, schrieb Johannes Stezenbach:
> On Linux x86_64 host with 32bit userspace, running
> qemu or even just "qemu-img create -f qcow2 some.img 1G"
> causes a kernel warning:
> 
> ioctl32(qemu-img:5296): Unknown cmd fd(3) cmd(00005326){t:'S';sz:0} 
> arg(7fffffff) on some.img
> ioctl32(qemu-img:5296): Unknown cmd fd(3) cmd(801c0204){t:02;sz:28} 
> arg(fff77350) on some.img
> 
> ioctl 00005326 is CDROM_DRIVE_STATUS,
> ioctl 801c0204 is FDGETPRM.
> 
> The warning appears because the Linux compat-ioctl handler for these
> ioctls only applies to block devices, while qemu also uses the ioctls on
> plain files.  Work around by calling fstat() the ensure the ioctls are
> only used on block devices.
> 
> Signed-off-by: Johannes Stezenbach <address@hidden>
> ---
> discussed in http://lkml.kernel.org/r/address@hidden
> 
> I'll also send a Linux kernel patch but this is needed to
> fix the warning on old kernels.

I probably wouldn't have bothered to work around it for older kernels,
but anyway it shouldn't hurt to do so.

Generally the patch looks good to me, just some minor comments:

> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 4cd7d7a..10d56f0 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -46,6 +46,8 @@
>  #include <sys/dkio.h>
>  #endif
>  #ifdef __linux__
> +#include <sys/types.h>
> +#include <sys/stat.h>
>  #include <sys/ioctl.h>
>  #include <sys/param.h>
>  #include <linux/cdrom.h>
> @@ -1188,6 +1190,7 @@ static int floppy_probe_device(const char *filename)
>      int fd, ret;
>      int prio = 0;
>      struct floppy_struct fdparam;
> +    struct stat st;
>  
>      if (strstart(filename, "/dev/fd", NULL))
>          prio = 50;
> @@ -1196,12 +1199,20 @@ static int floppy_probe_device(const char *filename)
>      if (fd < 0) {
>          goto out;
>      }
> +    ret = fstat(fd, &st);
> +    if (ret == -1) {
> +        goto outc;
> +    }
> +    if (!S_ISBLK(st.st_mode)) {
> +        goto outc;
> +    }

Why not a single if (ret == -1 || !S_ISBLK(st.st_mode))?

>      /* Attempt to detect via a floppy specific ioctl */
>      ret = ioctl(fd, FDGETPRM, &fdparam);
>      if (ret >= 0)
>          prio = 100;
>  
> +outc:
>      close(fd);
>  out:
>      return prio;
> @@ -1290,17 +1301,26 @@ static int cdrom_probe_device(const char *filename)
>  {
>      int fd, ret;
>      int prio = 0;
> +    struct stat st;
>  
>      fd = open(filename, O_RDONLY | O_NONBLOCK);
>      if (fd < 0) {
>          goto out;
>      }
> +    ret = fstat(fd, &st);
> +    if (ret == -1) {
> +        goto outc;
> +    }
> +    if (!S_ISBLK(st.st_mode)) {
> +        goto outc;
> +    }

Same here.

>      /* Attempt to detect via a CDROM specific ioctl */
>      ret = ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT);
>      if (ret >= 0)
>          prio = 100;
>  
> +outc:
>      close(fd);
>  out:
>      return prio;
> @@ -1309,11 +1329,15 @@ out:
>  static int cdrom_is_inserted(BlockDriverState *bs)
>  {
>      BDRVRawState *s = bs->opaque;
> +    struct stat st;
>      int ret;
>  
> -    ret = ioctl(s->fd, CDROM_DRIVE_STATUS, CDSL_CURRENT);
> -    if (ret == CDS_DISC_OK)
> -        return 1;
> +    ret = fstat(s->fd, &st);
> +    if (!ret && S_ISBLK(st.st_mode)) {
> +        ret = ioctl(s->fd, CDROM_DRIVE_STATUS, CDSL_CURRENT);
> +        if (ret == CDS_DISC_OK)
> +            return 1;
> +    }
>      return 0;
>  }

I think this last hunk is unnecessary. cdrom_is_inserted should only be
called if cdrom_probe_device returned success before, so we have already
tested this.

Kevin