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

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

[Programmingkid]

[-- Attachment #1: Type: text/plain, Size: 1493 bytes --]

Fix a problem with QEMU not being able to use real cd's on
Mac OS X hosts. Implements a function called
cd_is_inserted().

Signed-off-by: John Arbuckle <programmingkidx@gmail.com>

---
 block/raw-posix.c |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index a967464..9420602 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -2018,7 +2018,26 @@ kern_return_t GetBSDPath( io_iterator_t mediaIterator, char *bsdPath, CFIndex ma
     return kernResult;
 }
 
-#endif
+/*
+ * Determines if a real cdrom is inserted into the host computer's optical
+ * drive. Uses the fact that find_image_format() calls this function first
+ * in order to go around a bug involving trying to determine a real cd's
+ * format.
+ */
+static int cdrom_is_inserted(BlockDriverState *bs)
+{
+    static int count;
+    int returnValue = (raw_getlength(bs) > 0) ? 1 : 0;
+
+    if (count == 0) {
+        count++;
+        returnValue = 0; /* get around find_image_format() issue */
+    }
+
+    return returnValue;
+}
+
+#endif  /* __APPLE__ */
 
 static int hdev_probe_device(const char *filename)
 {
@@ -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
 };
 
 #ifdef __linux__
-- 
1.7.5.4


[-- Attachment #2: Type: text/html, Size: 9695 bytes --]

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

[Peter Maydell]

On 29 June 2015 at 17:54, Programmingkid <programmingkidx@gmail.com> 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?

-- PMM

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

[Programmingkid]

On Jun 29, 2015, at 1:11 PM, Peter Maydell wrote:

> On 29 June 2015 at 17:54, Programmingkid <programmingkidx@gmail.com> 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.

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 <programmingkidx@gmail.com> wrote:
>
> On Jun 29, 2015, at 1:11 PM, Peter Maydell wrote:
>
>> On 29 June 2015 at 17:54, Programmingkid <programmingkidx@gmail.com> 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

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

[Programmingkid]

[-- Attachment #1: Type: text/plain, Size: 2370 bytes --]


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

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

[-- Attachment #2: Type: text/html, Size: 4539 bytes --]

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

[Laurent Vivier]

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 <programmingkidx@gmail.com
>> <mailto:programmingkidx@gmail.com>> wrote:
>>>
>>> On Jun 29, 2015, at 1:11 PM, Peter Maydell wrote:
>>>
>>>> On 29 June 2015 at 17:54, Programmingkid <programmingkidx@gmail.com
>>>> <mailto:programmingkidx@gmail.com>> 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 <programmingkidx@gmail.com>
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

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 <programmingkidx@gmail.com
>>> <mailto:programmingkidx@gmail.com>> wrote:
>>>> 
>>>> On Jun 29, 2015, at 1:11 PM, Peter Maydell wrote:
>>>> 
>>>>> On 29 June 2015 at 17:54, Programmingkid <programmingkidx@gmail.com
>>>>> <mailto:programmingkidx@gmail.com>> 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 :(