On Jun 29, 2015, at 2:16 PM, Peter Maydell wrote: > On 29 June 2015 at 19:04, Programmingkid <[email protected]> wrote: >> >> On Jun 29, 2015, at 1:11 PM, Peter Maydell wrote: >> >>> On 29 June 2015 at 17:54, Programmingkid <[email protected]> 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.
