[Qemu-devel] Re: [PATCH v2 3/3] raw-posix: Re-open host CD-ROM after media change

[Kevin Wolf]

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 <address@hidden>

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