qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] raw-posix.c: cd_is_inserted() implementation fo


From: Programmingkid
Subject: Re: [Qemu-devel] [PATCH] raw-posix.c: cd_is_inserted() implementation for Mac OS X
Date: Mon, 29 Jun 2015 14:37:07 -0400


On Jun 29, 2015, at 2:16 PM, Peter Maydell wrote:

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.

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. 

reply via email to

[Prev in Thread] Current Thread [Next in Thread]