From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=50740 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q5Eja-0001bR-Oo for qemu-devel@nongnu.org; Thu, 31 Mar 2011 06:04:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q5EjU-0008NH-Nc for qemu-devel@nongnu.org; Thu, 31 Mar 2011 06:04:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:26051) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q5EjU-0008N1-Bh for qemu-devel@nongnu.org; Thu, 31 Mar 2011 06:04:04 -0400 Message-ID: <4D945206.6090204@redhat.com> Date: Thu, 31 Mar 2011 12:05:58 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1301425482-8722-1-git-send-email-stefanha@linux.vnet.ibm.com> <1301425482-8722-4-git-send-email-stefanha@linux.vnet.ibm.com> In-Reply-To: <1301425482-8722-4-git-send-email-stefanha@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH v2 3/3] raw-posix: Re-open host CD-ROM after media change List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Amit Shah , Anthony Liguori , Ryan Harper , qemu-devel@nongnu.org, Juan Quintela Am 29.03.2011 21:04, schrieb Stefan Hajnoczi: > Piggy-back on the guest CD-ROM polling to poll on the host. Open and > close the host CD-ROM file descriptor to ensure we read the new size and > not a stale size. > > Two things are going on here: > > 1. If hald/udisks is not already polling CD-ROMs on the host then > re-opening the CD-ROM causes the host to read the new medium's size. > > 2. There is a bug in Linux which means the CD-ROM file descriptor must > be re-opened in order for lseek(2) to see the new size. The > inode size gets out of sync with the underlying device (which you can > confirm by checking that /sys/block/sr0/size and lseek(2) do not > match after media change). I have raised this with the > maintainers but we need a workaround for the foreseeable future. > > Note that these changes are all in a #ifdef __linux__ section. > > Signed-off-by: Stefan Hajnoczi Applied the first two patches of the series. I have some comments on this one. > --- > block/raw-posix.c | 26 ++++++++++++++++++++++---- > 1 files changed, 22 insertions(+), 4 deletions(-) > > diff --git a/block/raw-posix.c b/block/raw-posix.c > index 6b72470..8b5205c 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -1238,10 +1238,28 @@ static int cdrom_is_inserted(BlockDriverState *bs) > BDRVRawState *s = bs->opaque; > int ret; > > - ret = ioctl(s->fd, CDROM_DRIVE_STATUS, CDSL_CURRENT); > - if (ret == CDS_DISC_OK) > - return 1; > - return 0; > + /* > + * Close the file descriptor if no medium is present and open it to poll > + * again. This ensures the medium size is refreshed. If the file > + * descriptor is kept open the size can become stale. This is essentially > + * replicating CD-ROM polling but is driven by the guest. As the guest > + * polls, we poll the host. > + */ > + > + if (s->fd == -1) { > + s->fd = qemu_open(bs->filename, s->open_flags, 0644); > + if (s->fd < 0) { > + return 0; > + } > + } > + > + ret = (ioctl(s->fd, CDROM_DRIVE_STATUS, CDSL_CURRENT) == CDS_DISC_OK); > + > + if (!ret) { > + close(s->fd); > + s->fd = -1; > + } > + return ret; > } We have this code in raw_close: if (s->fd >= 0) { close(s->fd); s->fd = -1; if (s->aligned_buf != NULL) qemu_vfree(s->aligned_buf); } So now that we set s->fd = -1, this part won't be run and we leak s->aligned_buf. This problem exists in other places in raw-posix, too. The other thing is that I'm not sure if everything in raw-posix is prepared to deal with a -1 fd. At the very least, I think we'll get -EBADF errors instead of the expected -ENOMEDIUM. The general approach looks good to me. Kevin