From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39869) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z9fti-0007gr-P9 for qemu-devel@nongnu.org; Mon, 29 Jun 2015 16:43:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z9fth-0006zn-Lf for qemu-devel@nongnu.org; Mon, 29 Jun 2015 16:43:22 -0400 Message-ID: <5591ADE0.6030506@redhat.com> Date: Mon, 29 Jun 2015 22:43:12 +0200 From: Laurent Vivier MIME-Version: 1.0 References: <36A983F7-2A97-4130-9179-FA614ACEBAA0@gmail.com> In-Reply-To: <36A983F7-2A97-4130-9179-FA614ACEBAA0@gmail.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] raw-posix.c: cd_is_inserted() implementation for Mac OS X List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Programmingkid Cc: Kevin Wolf , Peter Maydell , qemu-devel qemu-devel , Qemu-block 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 > > wrote: >>> >>> On Jun 29, 2015, at 1:11 PM, Peter Maydell wrote: >>> >>>> On 29 June 2015 at 17:54, Programmingkid >>> > 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 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