| Message ID | 1300902055-25850-4-git-send-email-stefanha@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote: > 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 <stefanha@linux.vnet.ibm.com> > --- > block/raw-posix.c | 25 +++++++++++++++++++++---- > 1 files changed, 21 insertions(+), 4 deletions(-) > > diff --git a/block/raw-posix.c b/block/raw-posix.c > index a95c8d4..ac95467 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -1240,10 +1240,27 @@ 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; > + /* > + * Opening and closing on each drive status check 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. > + */ Comment confused me :p we are not opening and closing at each status check. We are opening if the cdrom is _not_ opened. And we are closing if there is one error. If comment 2 above is right, you need to do insteand something like: if (s->fd != -1) { close(s->fd); } s->fd = open( ....); That is really reopening all the times. > + > + if (s->fd == -1) { > + s->fd = qemu_open(bs->filename, s->open_flags, 0644); Everything else on that file uses plain "open" not "qemu_open". diference is basically that qemu_open() adds flag O_CLOEXEC. I don't know if this one should be vanilla open or the other ones qemu_open(). What do you think? > + if (s->fd < 0) { > + return 0; > + } > + } > + > + ret = (ioctl(s->fd, CDROM_DRIVE_STATUS, CDSL_CURRENT) == CDS_DISC_OK); parens are not needed around ==. > + > + if (!ret) { > + close(s->fd); > + s->fd = -1; > + } > + return ret; > } > > static int cdrom_eject(BlockDriverState *bs, int eject_flag) Later, Juan.
On Wed, Mar 23, 2011 at 8:27 PM, Juan Quintela <quintela@redhat.com> wrote: > Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote: >> + /* >> + * Opening and closing on each drive status check 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. >> + */ > > Comment confused me :p > > we are not opening and closing at each status check. > We are opening if the cdrom is _not_ opened. And we are closing if > there is one error. If comment 2 above is right, you need to do > insteand something like: > > if (s->fd != -1) { > close(s->fd); > } > > s->fd = open( ....); > > That is really reopening all the times. You're right, I'll fix the comment. It should only close/reopen if there is no medium present. If there is already a medium then we're good. >> + >> + if (s->fd == -1) { >> + s->fd = qemu_open(bs->filename, s->open_flags, 0644); > > Everything else on that file uses plain "open" not "qemu_open". > diference is basically that qemu_open() adds flag O_CLOEXEC. > > I don't know if this one should be vanilla open or the other ones > qemu_open(). > > What do you think? raw_open_common() uses qemu_open(). That's why I used it. >> + if (s->fd < 0) { >> + return 0; >> + } >> + } >> + >> + ret = (ioctl(s->fd, CDROM_DRIVE_STATUS, CDSL_CURRENT) == CDS_DISC_OK); > > parens are not needed around ==. Yes, if you want I'll remove them. I just did it for readability. Stefan
Am 23.03.2011 21:50, schrieb Stefan Hajnoczi: > On Wed, Mar 23, 2011 at 8:27 PM, Juan Quintela <quintela@redhat.com> wrote: >> Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote: >>> + >>> + if (s->fd == -1) { >>> + s->fd = qemu_open(bs->filename, s->open_flags, 0644); >> >> Everything else on that file uses plain "open" not "qemu_open". >> diference is basically that qemu_open() adds flag O_CLOEXEC. >> >> I don't know if this one should be vanilla open or the other ones >> qemu_open(). >> >> What do you think? > > raw_open_common() uses qemu_open(). That's why I used it. And I think it's correct. There's no reason not to set O_CLOEXEC here. Maybe some of the open() users need to be fixed. >>> + if (s->fd < 0) { >>> + return 0; >>> + } >>> + } >>> + >>> + ret = (ioctl(s->fd, CDROM_DRIVE_STATUS, CDSL_CURRENT) == CDS_DISC_OK); >> >> parens are not needed around ==. > > Yes, if you want I'll remove them. I just did it for readability. I like them. Kevin
On Thu, Mar 24, 2011 at 12:42 PM, Kevin Wolf <kwolf@redhat.com> wrote: > Am 23.03.2011 21:50, schrieb Stefan Hajnoczi: >> On Wed, Mar 23, 2011 at 8:27 PM, Juan Quintela <quintela@redhat.com> wrote: >>> Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote: >>>> + >>>> + if (s->fd == -1) { >>>> + s->fd = qemu_open(bs->filename, s->open_flags, 0644); >>> >>> Everything else on that file uses plain "open" not "qemu_open". >>> diference is basically that qemu_open() adds flag O_CLOEXEC. >>> >>> I don't know if this one should be vanilla open or the other ones >>> qemu_open(). >>> >>> What do you think? >> >> raw_open_common() uses qemu_open(). That's why I used it. > > And I think it's correct. There's no reason not to set O_CLOEXEC here. > Maybe some of the open() users need to be fixed. > >>>> + if (s->fd < 0) { >>>> + return 0; >>>> + } >>>> + } >>>> + >>>> + ret = (ioctl(s->fd, CDROM_DRIVE_STATUS, CDSL_CURRENT) == CDS_DISC_OK); >>> >>> parens are not needed around ==. >> >> Yes, if you want I'll remove them. I just did it for readability. > > I like them. I will have to #ifdef QUINTELA and #ifdef KWOLF :). Seriously, I'll leave the braces unless anyone feels really strongly either way. This passes checkpatch.pl BTW. Stefan
Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Thu, Mar 24, 2011 at 12:42 PM, Kevin Wolf <kwolf@redhat.com> wrote: >> Am 23.03.2011 21:50, schrieb Stefan Hajnoczi: >>> On Wed, Mar 23, 2011 at 8:27 PM, Juan Quintela <quintela@redhat.com> wrote: >>>> Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote: >>>>> + >>>>> + if (s->fd == -1) { >>>>> + s->fd = qemu_open(bs->filename, s->open_flags, 0644); >>>> >>>> Everything else on that file uses plain "open" not "qemu_open". >>>> diference is basically that qemu_open() adds flag O_CLOEXEC. >>>> >>>> I don't know if this one should be vanilla open or the other ones >>>> qemu_open(). >>>> >>>> What do you think? >>> >>> raw_open_common() uses qemu_open(). That's why I used it. >> >> And I think it's correct. There's no reason not to set O_CLOEXEC here. >> Maybe some of the open() users need to be fixed. I didn't doubt that. What I tried to point is that there are three opens for cdrom/floppy on that file. It makes sense that all are the same. I guessed that proper fix was to change all others to qemu_open(), but just wanted to point that it was inconsistent, and should be done one or other way. >>>>> + if (s->fd < 0) { >>>>> + return 0; >>>>> + } >>>>> + } >>>>> + >>>>> + ret = (ioctl(s->fd, CDROM_DRIVE_STATUS, CDSL_CURRENT) == CDS_DISC_OK); >>>> >>>> parens are not needed around ==. >>> >>> Yes, if you want I'll remove them. I just did it for readability. >> >> I like them. > > I will have to #ifdef QUINTELA and #ifdef KWOLF :). Seriously, I'll > leave the braces unless anyone feels really strongly either way. This > passes checkpatch.pl BTW. In x = (a == b); braces are useless. But my preference is not 'strong' enough to try to force it on other people. It appears that other people feel strong about it, so let it be. Later, Juan.
diff --git a/block/raw-posix.c b/block/raw-posix.c index a95c8d4..ac95467 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -1240,10 +1240,27 @@ 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; + /* + * Opening and closing on each drive status check 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; } static int cdrom_eject(BlockDriverState *bs, int eject_flag)
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 <stefanha@linux.vnet.ibm.com> --- block/raw-posix.c | 25 +++++++++++++++++++++---- 1 files changed, 21 insertions(+), 4 deletions(-)