From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:50598) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QbvEW-0002Su-Va for qemu-devel@nongnu.org; Wed, 29 Jun 2011 09:55:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QbvEU-0006Ib-Ue for qemu-devel@nongnu.org; Wed, 29 Jun 2011 09:55:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:18036) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QbvET-0006I6-SV for qemu-devel@nongnu.org; Wed, 29 Jun 2011 09:55:10 -0400 Message-ID: <4E0B2F67.5070702@redhat.com> Date: Wed, 29 Jun 2011 15:57:59 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <20110629133601.GA8067@sig21.net> In-Reply-To: <20110629133601.GA8067@sig21.net> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] block/raw-posix: Linux compat-ioctl warning workaround List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Johannes Stezenbach Cc: Arnd Bergmann , qemu-devel@nongnu.org, kvm@vger.kernel.org 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 > --- > discussed in http://lkml.kernel.org/r/20110617090424.GA19345@sig21.net > > 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 > #endif > #ifdef __linux__ > +#include > +#include > #include > #include > #include > @@ -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