Patchwork [3/3] raw-posix: Re-open host CD-ROM after media change

login
register
mail settings
Submitter Stefan Hajnoczi
Date March 23, 2011, 5:40 p.m.
Message ID <1300902055-25850-4-git-send-email-stefanha@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/88106/
State New
Headers show

Comments

Stefan Hajnoczi - March 23, 2011, 5:40 p.m.
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(-)
Juan Quintela - March 23, 2011, 8:27 p.m.
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.
Stefan Hajnoczi - March 23, 2011, 8:50 p.m.
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
Kevin Wolf - March 24, 2011, 12:42 p.m.
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
Stefan Hajnoczi - March 24, 2011, 7 p.m.
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
Juan Quintela - March 24, 2011, 9:23 p.m.
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.

Patch

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)