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

[Stefan Hajnoczi]

On Wed, Mar 23, 2011 at 8:27 PM, Juan Quintela <address@hidden> wrote:
> Stefan Hajnoczi <address@hidden> 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