Re: [Qemu-devel] [PATCH] raw-posix.c: cd_is_inserted() implementation for Mac OS X

[Laurent Vivier <lvivier@redhat.com>]

On 29/06/2015 20:37, Programmingkid wrote:
> 
> On Jun 29, 2015, at 2:16 PM, Peter Maydell wrote:
> 
>> On 29 June 2015 at 19:04, Programmingkid <programmingkidx@gmail.com
>> <mailto:programmingkidx@gmail.com>> wrote:
>>>
>>> On Jun 29, 2015, at 1:11 PM, Peter Maydell wrote:
>>>
>>>> On 29 June 2015 at 17:54, Programmingkid <programmingkidx@gmail.com
>>>> <mailto:programmingkidx@gmail.com>> wrote:
>>>>> @@ -2365,6 +2384,10 @@ static BlockDriver bdrv_host_device = {
>>>>>    .bdrv_ioctl         = hdev_ioctl,
>>>>>    .bdrv_aio_ioctl     = hdev_aio_ioctl,
>>>>> #endif
>>>>> +
>>>>> +#ifdef __APPLE__
>>>>> +    .bdrv_is_inserted   = cdrom_is_inserted,
>>>>> +#endif
>>>>
>>>> Why isn't this handled by having a bdrv_host_cdrom,
>>>> like Linux and FreeBSD do for their CDROM support?
>>>
>>> That would involve a lot of unnecessary work and modifications. This
>>> small change is all that is needed.
>>
>> Yes, but it's obviously wrong, because this:
>>
>> +    if (count == 0) {
>> +        count++;
>> +        returnValue = 0; /* get around find_image_format() issue */
>> +    }
>>
>> makes no sense at all -- this means that we'll always report "drive
>> empty" the first time this function is called. We should always
>> report the correct answer, regardless of who's calling us.
>>
>> If you find yourself writing this kind of weird workaround, it
>> generally suggests that the change is a "this happens to make it
>> work" patch, not the correct fix for the problem. We need clean
>> fixes in QEMU, because if we allow "happens to make it work"
>> patches to pile up then the whole system becomes unmaintainable.
>> Yes, this often means that the amount of work required to
>> fix a bug is more than a handful of lines. That doesn't mean
>> that the work is unnecessary.
>>
>> (For instance, what happens if somebody changes some other
>> part of QEMU so that it happens that find_image_format() is not
>> the first thing to call this function?)
>>
>> We know the correct way to support host cdrom drives, because
>> we're already doing that on Linux. We should consistently
>> support host cdrom drives the same way for all hosts.
> 
> I have really tried to find out what was wrong. It is a asynchronous,
> multi-threaded mess. Trying to follow where QEMU messes up 
> was hard. The closest I came to was to a function called 
> bdrv_co_io_em(). It was returning a value of -22. 
> 
> If some change does happen to make this patch to 
> not work anymore, I can easily fix it. 

Frankly, I don't understand you.

The only thing you have to do is to write:

static int cdrom_is_inserted(BlockDriverState *bs)
{
    return raw_getlength(bs) > 0;
}

You have introduced yourself the support for raw_getlength() for MacOS X:

commit 728dacbda817b2ca259e9d337fab06bcf14e94a6
Author: Programmingkid <programmingkidx@gmail.com>
Date:   Mon Jan 19 17:12:55 2015 -0500

    block/raw-posix.c: Fix raw_getlength() on Mac OS X block devices

    This patch replaces the dummy code in raw_getlength() for block devices
    on OS X, which always returned LLONG_MAX, with a real implementation
    that returns the actual block device size.

Then, just "#ifdef CONFIG_BSD" around the existing bdrv_host_cdrom of
FreeBSD (minus cdrom_eject and cdrom_lock_medium) and bdrv_register().

Laurent