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

[Programmingkid]

On Jun 29, 2015, at 4:43 PM, Laurent Vivier wrote:

> 
> 
> 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 <address@hidden
>>> <mailto:address@hidden>> wrote:
>>>> 
>>>> On Jun 29, 2015, at 1:11 PM, Peter Maydell wrote:
>>>> 
>>>>> On 29 June 2015 at 17:54, Programmingkid <address@hidden
>>>>> <mailto:address@hidden>> 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;
> }

Yes, this is probably the correct implementation for cdrom_is_inserted(), but
what I'm trying to do is to make a real cdrom work in QEMU. This
implementation of cdrom_is_inserted() doesn't solve the problem with
find_image_format(). The problem still causes QEMU to quit when using
the option "-cdrom /dev/cdrom". 

My patch I sent you before does fix things, but it is viewed as a hack. I was
hoping the patch might be temporarily accepted until better solution was made.
That is not going to happen :(