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

[Peter Maydell]

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

thanks
-- PMM