[Qemu-devel] [PATCH] block.c: fix real cdrom detection

[Qemu-devel] [PATCH] block.c: fix real cdrom detection

[Programmingkid]

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

Fix real cdrom detection so that a real cdrom can actually be used.

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

This patch has been tested on Mac OS X host and guest. 
Command used: qemu-system-ppc -cdrom /dev/cdrom

Note: I was able to view the files using OpenBIOS, but not on 
Mac OS X. The size of the disc is reported correctly but some
error happens that prevents it from mounting in Mac OS X. This
is probably another bug with QEMU.

---
 block.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/block.c b/block.c
index dd4f58d..75ccfad 100644
--- a/block.c
+++ b/block.c
@@ -583,7 +583,8 @@ static int find_image_format(BlockDriverState *bs, const char *filename,
     int ret = 0;
 
     /* Return the raw BlockDriver * to scsi-generic devices or empty drives */
-    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0) {
+    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0 
+               || strcmp("/dev/cdrom", filename) == 0) {
         *pdrv = &bdrv_raw;
         return ret;
     }
-- 
1.7.5.4


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

Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection

[John Snow]

On 06/23/2015 01:56 PM, Programmingkid wrote:
> Fix real cdrom detection so that a real cdrom can actually be used.
> 
> signed-off-by: John Arbuckle <programmingkidx@gmail.com
> <mailto:programmingkidx@gmail.com>>
> 
> This patch has been tested on Mac OS X host and guest. 
> Command used: qemu-system-ppc -cdrom /dev/cdrom
> 
> Note: I was able to view the files using OpenBIOS, but not on 
> Mac OS X. The size of the disc is reported correctly but some
> error happens that prevents it from mounting in Mac OS X. This
> is probably another bug with QEMU.
> 
> ---
>  block.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/block.c b/block.c
> index dd4f58d..75ccfad 100644
> --- a/block.c
> +++ b/block.c
> @@ -583,7 +583,8 @@ static int find_image_format(BlockDriverState *bs,
> const char *filename,
>      int ret = 0;
> 
>  
> 
>      /* Return the raw BlockDriver * to scsi-generic devices or empty
> drives */
> -    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0) {
> +    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0 
> +               || strcmp("/dev/cdrom", filename) == 0) {
>          *pdrv = &bdrv_raw;
>          return ret;
>      }
> -- 
> 1.7.5.4
> 

So what's the issue that this patch attempts to fix and how did you
determine that the fix was needed here? It doesn't look like it respects
proper abstraction at a glance.

--js

Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection

[Programmingkid]

On Jun 23, 2015, at 2:06 PM, John Snow wrote:

> 
> 
> On 06/23/2015 01:56 PM, Programmingkid wrote:
>> Fix real cdrom detection so that a real cdrom can actually be used.
>> 
>> signed-off-by: John Arbuckle <programmingkidx@gmail.com
>> <mailto:programmingkidx@gmail.com>>
>> 
>> This patch has been tested on Mac OS X host and guest. 
>> Command used: qemu-system-ppc -cdrom /dev/cdrom
>> 
>> Note: I was able to view the files using OpenBIOS, but not on 
>> Mac OS X. The size of the disc is reported correctly but some
>> error happens that prevents it from mounting in Mac OS X. This
>> is probably another bug with QEMU.
>> 
>> ---
>> block.c |    3 ++-
>> 1 files changed, 2 insertions(+), 1 deletions(-)
>> 
>> diff --git a/block.c b/block.c
>> index dd4f58d..75ccfad 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -583,7 +583,8 @@ static int find_image_format(BlockDriverState *bs,
>> const char *filename,
>>     int ret = 0;
>> 
>> 
>> 
>>     /* Return the raw BlockDriver * to scsi-generic devices or empty
>> drives */
>> -    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0) {
>> +    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0 
>> +               || strcmp("/dev/cdrom", filename) == 0) {
>>         *pdrv = &bdrv_raw;
>>         return ret;
>>     }
>> -- 
>> 1.7.5.4
>> 
> 
> So what's the issue that this patch attempts to fix and how did you
> determine that the fix was needed here? It doesn't look like it respects
> proper abstraction at a glance.

Without the patch, QEMU would just quit when the "-cdrom /dev/cdrom" option is given.

Before the patch, the bdrv_open_inherit() function would be incorrectly called. Its documentation says "Opens a disk image (raw, qcow2, vmdk, ...)" meaning only for disk image files (not for real media). This patch prevents the bdrv_open_inherit() function from ever being called. It sets the pdrv variable to the raw format. This made sense to me since a real cdrom is read in the raw format.

A quick test does show the patch works. A real cdrom is successfully opened on qemu-system-i386 using a Windows XP guest. 

Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection

[Markus Armbruster]

Programmingkid <programmingkidx@gmail.com> writes:

> On Jun 23, 2015, at 2:06 PM, John Snow wrote:
>
>> 
>> 
>> On 06/23/2015 01:56 PM, Programmingkid wrote:
>>> Fix real cdrom detection so that a real cdrom can actually be used.
>>> 
>>> signed-off-by: John Arbuckle <programmingkidx@gmail.com
>>> <mailto:programmingkidx@gmail.com>>
>>> 
>>> This patch has been tested on Mac OS X host and guest. 
>>> Command used: qemu-system-ppc -cdrom /dev/cdrom
>>> 
>>> Note: I was able to view the files using OpenBIOS, but not on 
>>> Mac OS X. The size of the disc is reported correctly but some
>>> error happens that prevents it from mounting in Mac OS X. This
>>> is probably another bug with QEMU.
>>> 
>>> ---
>>> block.c |    3 ++-
>>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>> 
>>> diff --git a/block.c b/block.c
>>> index dd4f58d..75ccfad 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -583,7 +583,8 @@ static int find_image_format(BlockDriverState *bs,
>>> const char *filename,
>>>     int ret = 0;
>>> 
>>> 
>>> 
>>>     /* Return the raw BlockDriver * to scsi-generic devices or empty
>>> drives */
>>> -    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0) {
>>> +    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0 
>>> +               || strcmp("/dev/cdrom", filename) == 0) {
>>>         *pdrv = &bdrv_raw;
>>>         return ret;
>>>     }
>>> -- 
>>> 1.7.5.4
>>> 
>> 
>> So what's the issue that this patch attempts to fix and how did you
>> determine that the fix was needed here? It doesn't look like it respects
>> proper abstraction at a glance.
>
> Without the patch, QEMU would just quit when the "-cdrom /dev/cdrom"
> option is given.
>
> Before the patch, the bdrv_open_inherit() function would be
> incorrectly called. Its documentation says "Opens a disk image (raw,
> qcow2, vmdk, ...)" meaning only for disk image files (not for real
> media). This patch prevents the bdrv_open_inherit() function from ever
> being called. It sets the pdrv variable to the raw format. This made
> sense to me since a real cdrom is read in the raw format.
>
> A quick test does show the patch works. A real cdrom is successfully
> opened on qemu-system-i386 using a Windows XP guest.

What about /dev/sr0, /dev/sr1, and whatever other names could refer to a
block device without a medium?

Comparing filenames isn't a good way to test "is a block device without
a medium".

Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection

[Programmingkid]

On Jun 25, 2015, at 2:53 AM, Markus Armbruster wrote:

> Programmingkid <programmingkidx@gmail.com> writes:
> 
>> On Jun 23, 2015, at 2:06 PM, John Snow wrote:
>> 
>>> 
>>> 
>>> On 06/23/2015 01:56 PM, Programmingkid wrote:
>>>> Fix real cdrom detection so that a real cdrom can actually be used.
>>>> 
>>>> signed-off-by: John Arbuckle <programmingkidx@gmail.com
>>>> <mailto:programmingkidx@gmail.com>>
>>>> 
>>>> This patch has been tested on Mac OS X host and guest. 
>>>> Command used: qemu-system-ppc -cdrom /dev/cdrom
>>>> 
>>>> Note: I was able to view the files using OpenBIOS, but not on 
>>>> Mac OS X. The size of the disc is reported correctly but some
>>>> error happens that prevents it from mounting in Mac OS X. This
>>>> is probably another bug with QEMU.
>>>> 
>>>> ---
>>>> block.c |    3 ++-
>>>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>>> 
>>>> diff --git a/block.c b/block.c
>>>> index dd4f58d..75ccfad 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -583,7 +583,8 @@ static int find_image_format(BlockDriverState *bs,
>>>> const char *filename,
>>>>    int ret = 0;
>>>> 
>>>> 
>>>> 
>>>>    /* Return the raw BlockDriver * to scsi-generic devices or empty
>>>> drives */
>>>> -    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0) {
>>>> +    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0 
>>>> +               || strcmp("/dev/cdrom", filename) == 0) {
>>>>        *pdrv = &bdrv_raw;
>>>>        return ret;
>>>>    }
>>>> -- 
>>>> 1.7.5.4
>>>> 
>>> 
>>> So what's the issue that this patch attempts to fix and how did you
>>> determine that the fix was needed here? It doesn't look like it respects
>>> proper abstraction at a glance.
>> 
>> Without the patch, QEMU would just quit when the "-cdrom /dev/cdrom"
>> option is given.
>> 
>> Before the patch, the bdrv_open_inherit() function would be
>> incorrectly called. Its documentation says "Opens a disk image (raw,
>> qcow2, vmdk, ...)" meaning only for disk image files (not for real
>> media). This patch prevents the bdrv_open_inherit() function from ever
>> being called. It sets the pdrv variable to the raw format. This made
>> sense to me since a real cdrom is read in the raw format.
>> 
>> A quick test does show the patch works. A real cdrom is successfully
>> opened on qemu-system-i386 using a Windows XP guest.
> 
> What about /dev/sr0, /dev/sr1, and whatever other names could refer to a
> block device without a medium?
> 
> Comparing filenames isn't a good way to test "is a block device without
> a medium".

I think we are going to have to agree to disagree. I have never used the /dev/sr(0 | 1) devices and don't see how they would be effected by this patch. Are you trying to say the /dev/sr(0 | 1) devices *should* be handled by this patch? 

Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection

[Programmingkid]

On Jun 25, 2015, at 11:14 AM, Programmingkid wrote:

> 
> On Jun 25, 2015, at 2:53 AM, Markus Armbruster wrote:
> 
>> Programmingkid <programmingkidx@gmail.com> writes:
>> 
>>> On Jun 23, 2015, at 2:06 PM, John Snow wrote:
>>> 
>>>> 
>>>> 
>>>> On 06/23/2015 01:56 PM, Programmingkid wrote:
>>>>> Fix real cdrom detection so that a real cdrom can actually be used.
>>>>> 
>>>>> signed-off-by: John Arbuckle <programmingkidx@gmail.com
>>>>> <mailto:programmingkidx@gmail.com>>
>>>>> 
>>>>> This patch has been tested on Mac OS X host and guest. 
>>>>> Command used: qemu-system-ppc -cdrom /dev/cdrom
>>>>> 
>>>>> Note: I was able to view the files using OpenBIOS, but not on 
>>>>> Mac OS X. The size of the disc is reported correctly but some
>>>>> error happens that prevents it from mounting in Mac OS X. This
>>>>> is probably another bug with QEMU.
>>>>> 
>>>>> ---
>>>>> block.c |    3 ++-
>>>>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>>>> 
>>>>> diff --git a/block.c b/block.c
>>>>> index dd4f58d..75ccfad 100644
>>>>> --- a/block.c
>>>>> +++ b/block.c
>>>>> @@ -583,7 +583,8 @@ static int find_image_format(BlockDriverState *bs,
>>>>> const char *filename,
>>>>>   int ret = 0;
>>>>> 
>>>>> 
>>>>> 
>>>>>   /* Return the raw BlockDriver * to scsi-generic devices or empty
>>>>> drives */
>>>>> -    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0) {
>>>>> +    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0 
>>>>> +               || strcmp("/dev/cdrom", filename) == 0) {
>>>>>       *pdrv = &bdrv_raw;
>>>>>       return ret;
>>>>>   }
>>>>> -- 
>>>>> 1.7.5.4
>>>>> 
>>>> 
>>>> So what's the issue that this patch attempts to fix and how did you
>>>> determine that the fix was needed here? It doesn't look like it respects
>>>> proper abstraction at a glance.
>>> 
>>> Without the patch, QEMU would just quit when the "-cdrom /dev/cdrom"
>>> option is given.
>>> 
>>> Before the patch, the bdrv_open_inherit() function would be
>>> incorrectly called. Its documentation says "Opens a disk image (raw,
>>> qcow2, vmdk, ...)" meaning only for disk image files (not for real
>>> media). This patch prevents the bdrv_open_inherit() function from ever
>>> being called. It sets the pdrv variable to the raw format. This made
>>> sense to me since a real cdrom is read in the raw format.
>>> 
>>> A quick test does show the patch works. A real cdrom is successfully
>>> opened on qemu-system-i386 using a Windows XP guest.
>> 
>> What about /dev/sr0, /dev/sr1, and whatever other names could refer to a
>> block device without a medium?
>> 
>> Comparing filenames isn't a good way to test "is a block device without
>> a medium".
> 
> I think we are going to have to agree to disagree. I have never used the /dev/sr(0 | 1) devices and don't see how they would be effected by this patch. Are you trying to say the /dev/sr(0 | 1) devices *should* be handled by this patch?

Thinking about your question some more, I see what you mean. On Linux /dev/sr0 refers to the cdrom drive. Also on Linux, the /dev/cdrom link refers to the /dev/sr0 device file. So if you just use /dev/cdrom, you are good. 

Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection

[Programmingkid]

On Jun 25, 2015, at 11:32 AM, Programmingkid wrote:

> 
> On Jun 25, 2015, at 11:14 AM, Programmingkid wrote:
> 
>> 
>> On Jun 25, 2015, at 2:53 AM, Markus Armbruster wrote:
>> 
>>> Programmingkid <programmingkidx@gmail.com> writes:
>>> 
>>>> On Jun 23, 2015, at 2:06 PM, John Snow wrote:
>>>> 
>>>>> 
>>>>> 
>>>>> On 06/23/2015 01:56 PM, Programmingkid wrote:
>>>>>> Fix real cdrom detection so that a real cdrom can actually be used.
>>>>>> 
>>>>>> signed-off-by: John Arbuckle <programmingkidx@gmail.com
>>>>>> <mailto:programmingkidx@gmail.com>>
>>>>>> 
>>>>>> This patch has been tested on Mac OS X host and guest. 
>>>>>> Command used: qemu-system-ppc -cdrom /dev/cdrom
>>>>>> 
>>>>>> Note: I was able to view the files using OpenBIOS, but not on 
>>>>>> Mac OS X. The size of the disc is reported correctly but some
>>>>>> error happens that prevents it from mounting in Mac OS X. This
>>>>>> is probably another bug with QEMU.
>>>>>> 
>>>>>> ---
>>>>>> block.c |    3 ++-
>>>>>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>>>>> 
>>>>>> diff --git a/block.c b/block.c
>>>>>> index dd4f58d..75ccfad 100644
>>>>>> --- a/block.c
>>>>>> +++ b/block.c
>>>>>> @@ -583,7 +583,8 @@ static int find_image_format(BlockDriverState *bs,
>>>>>> const char *filename,
>>>>>>  int ret = 0;
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>>  /* Return the raw BlockDriver * to scsi-generic devices or empty
>>>>>> drives */
>>>>>> -    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0) {
>>>>>> +    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0 
>>>>>> +               || strcmp("/dev/cdrom", filename) == 0) {
>>>>>>      *pdrv = &bdrv_raw;
>>>>>>      return ret;
>>>>>>  }
>>>>>> -- 
>>>>>> 1.7.5.4
>>>>>> 
>>>>> 
>>>>> So what's the issue that this patch attempts to fix and how did you
>>>>> determine that the fix was needed here? It doesn't look like it respects
>>>>> proper abstraction at a glance.
>>>> 
>>>> Without the patch, QEMU would just quit when the "-cdrom /dev/cdrom"
>>>> option is given.
>>>> 
>>>> Before the patch, the bdrv_open_inherit() function would be
>>>> incorrectly called. Its documentation says "Opens a disk image (raw,
>>>> qcow2, vmdk, ...)" meaning only for disk image files (not for real
>>>> media). This patch prevents the bdrv_open_inherit() function from ever
>>>> being called. It sets the pdrv variable to the raw format. This made
>>>> sense to me since a real cdrom is read in the raw format.
>>>> 
>>>> A quick test does show the patch works. A real cdrom is successfully
>>>> opened on qemu-system-i386 using a Windows XP guest.
>>> 
>>> What about /dev/sr0, /dev/sr1, and whatever other names could refer to a
>>> block device without a medium?
>>> 
>>> Comparing filenames isn't a good way to test "is a block device without
>>> a medium".
>> 
>> I think we are going to have to agree to disagree. I have never used the /dev/sr(0 | 1) devices and don't see how they would be effected by this patch. Are you trying to say the /dev/sr(0 | 1) devices *should* be handled by this patch?
> 
> Thinking about your question some more, I see what you mean. On Linux /dev/sr0 refers to the cdrom drive. Also on Linux, the /dev/cdrom link refers to the /dev/sr0 device file. So if you just use /dev/cdrom, you are good.

Just had another idea. I could change the patch so that if it detects any device file being used, it handles it as raw. That way if you want to use /dev/sr0, you can. 

 /* Return the raw BlockDriver * to scsi-generic devices or empty
drives */
-    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0) {
+    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0 
+                      || strncmp("/dev/", filename, strlen("/dev/")) == 0) {
     *pdrv = &bdrv_raw;
     return ret;
 }

Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection

[Paolo Bonzini]

On 25/06/2015 17:32, Programmingkid wrote:
>> I think we are going to have to agree to disagree. I have never
>> used the /dev/sr(0 | 1) devices and don't see how they would be
>> effected by this patch. Are you trying to say the /dev/sr(0 | 1)
>> devices *should* be handled by this patch?
> 
> Thinking about your question some more, I see what you mean. On Linux
> /dev/sr0 refers to the cdrom drive. Also on Linux, the /dev/cdrom
> link refers to the /dev/sr0 device file. So if you just use
> /dev/cdrom, you are good.

Well, that's not how things work.

If you do things like that, you end up with a bunch of hacks, not with a
decent piece of software.

There is support for CD-ROM passthrough on Linux and FreeBSD in
block/raw-posix.c.  Perhaps the FreeBSD support can be extended to OS X
as well.

Paolo

Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection

[Laurent Vivier]

On 25/06/2015 17:48, Paolo Bonzini wrote:
> 
> On 25/06/2015 17:32, Programmingkid wrote:
>>> I think we are going to have to agree to disagree. I have never
>>> used the /dev/sr(0 | 1) devices and don't see how they would be
>>> effected by this patch. Are you trying to say the /dev/sr(0 | 1)
>>> devices *should* be handled by this patch?
>>
>> Thinking about your question some more, I see what you mean. On Linux
>> /dev/sr0 refers to the cdrom drive. Also on Linux, the /dev/cdrom
>> link refers to the /dev/sr0 device file. So if you just use
>> /dev/cdrom, you are good.
> 
> Well, that's not how things work.
> 
> If you do things like that, you end up with a bunch of hacks, not with a
> decent piece of software.
> 
> There is support for CD-ROM passthrough on Linux and FreeBSD in
> block/raw-posix.c.  Perhaps the FreeBSD support can be extended to OS X
> as well.
> 

In fact, programmingkid, you should fix it in hdev_open() where there is
already a #if __APPLE__ .

Paolo, I agree with you but :

hdev_open()

#if defined(__linux__)
    {
        char resolved_path[ MAXPATHLEN ], *temp;

        temp = realpath(filename, resolved_path);
        if (temp && strstart(temp, "/dev/sg", NULL)) {
            bs->sg = 1;
        }
#endif

I'm wondering who had this strange idea... :)

Laurent

Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection

[Paolo Bonzini]

On 25/06/2015 18:12, Laurent Vivier wrote:
> 
> 
> On 25/06/2015 17:48, Paolo Bonzini wrote:
>>
>> On 25/06/2015 17:32, Programmingkid wrote:
>>>> I think we are going to have to agree to disagree. I have never
>>>> used the /dev/sr(0 | 1) devices and don't see how they would be
>>>> effected by this patch. Are you trying to say the /dev/sr(0 | 1)
>>>> devices *should* be handled by this patch?
>>>
>>> Thinking about your question some more, I see what you mean. On Linux
>>> /dev/sr0 refers to the cdrom drive. Also on Linux, the /dev/cdrom
>>> link refers to the /dev/sr0 device file. So if you just use
>>> /dev/cdrom, you are good.
>>
>> Well, that's not how things work.
>>
>> If you do things like that, you end up with a bunch of hacks, not with a
>> decent piece of software.
>>
>> There is support for CD-ROM passthrough on Linux and FreeBSD in
>> block/raw-posix.c.  Perhaps the FreeBSD support can be extended to OS X
>> as well.
>>
> 
> In fact, programmingkid, you should fix it in hdev_open() where there is
> already a #if __APPLE__ .
> 
> Paolo, I agree with you but :
> 
> hdev_open()
> 
> #if defined(__linux__)
>     {
>         char resolved_path[ MAXPATHLEN ], *temp;
> 
>         temp = realpath(filename, resolved_path);
>         if (temp && strstart(temp, "/dev/sg", NULL)) {
>             bs->sg = 1;
>         }
> #endif
> 
> I'm wondering who had this strange idea... :)

I was very scared to type "git blame" here. :)  But the question is also
where to put the checks.  Putting it at a random place in block.c is not
a good idea.

But yes, this is also bad.  It should use stat and check the major/minor
numbers.

Paolo

Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection

[Laurent Vivier]

On 25/06/2015 18:16, Paolo Bonzini wrote:
> 
> 
> On 25/06/2015 18:12, Laurent Vivier wrote:
>>
>>
>> On 25/06/2015 17:48, Paolo Bonzini wrote:
>>>
>>> On 25/06/2015 17:32, Programmingkid wrote:
>>>>> I think we are going to have to agree to disagree. I have never
>>>>> used the /dev/sr(0 | 1) devices and don't see how they would be
>>>>> effected by this patch. Are you trying to say the /dev/sr(0 | 1)
>>>>> devices *should* be handled by this patch?
>>>>
>>>> Thinking about your question some more, I see what you mean. On Linux
>>>> /dev/sr0 refers to the cdrom drive. Also on Linux, the /dev/cdrom
>>>> link refers to the /dev/sr0 device file. So if you just use
>>>> /dev/cdrom, you are good.
>>>
>>> Well, that's not how things work.
>>>
>>> If you do things like that, you end up with a bunch of hacks, not with a
>>> decent piece of software.
>>>
>>> There is support for CD-ROM passthrough on Linux and FreeBSD in
>>> block/raw-posix.c.  Perhaps the FreeBSD support can be extended to OS X
>>> as well.
>>>
>>
>> In fact, programmingkid, you should fix it in hdev_open() where there is
>> already a #if __APPLE__ .
>>
>> Paolo, I agree with you but :
>>
>> hdev_open()
>>
>> #if defined(__linux__)
>>     {
>>         char resolved_path[ MAXPATHLEN ], *temp;
>>
>>         temp = realpath(filename, resolved_path);
>>         if (temp && strstart(temp, "/dev/sg", NULL)) {
>>             bs->sg = 1;
>>         }
>> #endif
>>
>> I'm wondering who had this strange idea... :)
> 
> I was very scared to type "git blame" here. :)  But the question is also

http://geek-and-poke.com/2013/11/24/simply-explained

BTW, it is a legacy from 2006:

19cb373 better support of host drives

coming from MacOS X (again!):

3b0d4f6 OS X: support for the built in CD-ROM drive (Mike Kronenberg)

> where to put the checks.  Putting it at a random place in block.c is not
> a good idea.
> 
> But yes, this is also bad.  It should use stat and check the major/minor
> numbers.

Yes, we should check if major is SCSI_GENERIC_MAJOR (21) (on linux).

We can also try to send an SG command like in cdrom_probe_device().
Something like in scsi_generic_realize():

    rc = blk_ioctl(s->conf.blk, SG_GET_VERSION_NUM, &sg_version);
    if (rc < 0) {
        error_setg(errp, "cannot get SG_IO version number: %s.  "
                         "Is this a SCSI device?",
                         strerror(-rc));
        return;
    }

Laurent

Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection

[Laurent Vivier]

On 25/06/2015 19:19, Laurent Vivier wrote:
> 
> 
> On 25/06/2015 18:16, Paolo Bonzini wrote:
>>
>>
>> On 25/06/2015 18:12, Laurent Vivier wrote:
>>>
>>>
>>> On 25/06/2015 17:48, Paolo Bonzini wrote:
>>>>
>>>> On 25/06/2015 17:32, Programmingkid wrote:
>>>>>> I think we are going to have to agree to disagree. I have never
>>>>>> used the /dev/sr(0 | 1) devices and don't see how they would be
>>>>>> effected by this patch. Are you trying to say the /dev/sr(0 | 1)
>>>>>> devices *should* be handled by this patch?
>>>>>
>>>>> Thinking about your question some more, I see what you mean. On Linux
>>>>> /dev/sr0 refers to the cdrom drive. Also on Linux, the /dev/cdrom
>>>>> link refers to the /dev/sr0 device file. So if you just use
>>>>> /dev/cdrom, you are good.
>>>>
>>>> Well, that's not how things work.
>>>>
>>>> If you do things like that, you end up with a bunch of hacks, not with a
>>>> decent piece of software.
>>>>
>>>> There is support for CD-ROM passthrough on Linux and FreeBSD in
>>>> block/raw-posix.c.  Perhaps the FreeBSD support can be extended to OS X
>>>> as well.
>>>>
>>>
>>> In fact, programmingkid, you should fix it in hdev_open() where there is
>>> already a #if __APPLE__ .
>>>
>>> Paolo, I agree with you but :
>>>
>>> hdev_open()
>>>
>>> #if defined(__linux__)
>>>     {
>>>         char resolved_path[ MAXPATHLEN ], *temp;
>>>
>>>         temp = realpath(filename, resolved_path);
>>>         if (temp && strstart(temp, "/dev/sg", NULL)) {
>>>             bs->sg = 1;
>>>         }
>>> #endif
>>>
>>> I'm wondering who had this strange idea... :)
>>
>> I was very scared to type "git blame" here. :)  But the question is also
> 
> http://geek-and-poke.com/2013/11/24/simply-explained
> 
> BTW, it is a legacy from 2006:
> 
> 19cb373 better support of host drives
> 
> coming from MacOS X (again!):
> 
> 3b0d4f6 OS X: support for the built in CD-ROM drive (Mike Kronenberg)
> 
>> where to put the checks.  Putting it at a random place in block.c is not
>> a good idea.
>>
>> But yes, this is also bad.  It should use stat and check the major/minor
>> numbers.
> 
> Yes, we should check if major is SCSI_GENERIC_MAJOR (21) (on linux).
> 
> We can also try to send an SG command like in cdrom_probe_device().
> Something like in scsi_generic_realize():
> 
>     rc = blk_ioctl(s->conf.blk, SG_GET_VERSION_NUM, &sg_version);
>     if (rc < 0) {
>         error_setg(errp, "cannot get SG_IO version number: %s.  "
>                          "Is this a SCSI device?",
>                          strerror(-rc));
>         return;
>     }
> 

BTW, this solution is already queued in Stefan's block tree:

https://github.com/stefanha/qemu/commit/3307ed7b3fac5ba99eb3b84904b0b7cdc3592a61

Laurent

Re: [Qemu-devel] [Qemu-block] [PATCH] block.c: fix real cdrom detection

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2015 at 07:19:14PM +0200, Laurent Vivier wrote:
> 
> 
> On 25/06/2015 18:16, Paolo Bonzini wrote:
> > 
> > 
> > On 25/06/2015 18:12, Laurent Vivier wrote:
> >>
> >>
> >> On 25/06/2015 17:48, Paolo Bonzini wrote:
> >>>
> >>> On 25/06/2015 17:32, Programmingkid wrote:
> >>>>> I think we are going to have to agree to disagree. I have never
> >>>>> used the /dev/sr(0 | 1) devices and don't see how they would be
> >>>>> effected by this patch. Are you trying to say the /dev/sr(0 | 1)
> >>>>> devices *should* be handled by this patch?
> >>>>
> >>>> Thinking about your question some more, I see what you mean. On Linux
> >>>> /dev/sr0 refers to the cdrom drive. Also on Linux, the /dev/cdrom
> >>>> link refers to the /dev/sr0 device file. So if you just use
> >>>> /dev/cdrom, you are good.
> >>>
> >>> Well, that's not how things work.
> >>>
> >>> If you do things like that, you end up with a bunch of hacks, not with a
> >>> decent piece of software.
> >>>
> >>> There is support for CD-ROM passthrough on Linux and FreeBSD in
> >>> block/raw-posix.c.  Perhaps the FreeBSD support can be extended to OS X
> >>> as well.
> >>>
> >>
> >> In fact, programmingkid, you should fix it in hdev_open() where there is
> >> already a #if __APPLE__ .
> >>
> >> Paolo, I agree with you but :
> >>
> >> hdev_open()
> >>
> >> #if defined(__linux__)
> >>     {
> >>         char resolved_path[ MAXPATHLEN ], *temp;
> >>
> >>         temp = realpath(filename, resolved_path);
> >>         if (temp && strstart(temp, "/dev/sg", NULL)) {
> >>             bs->sg = 1;
> >>         }
> >> #endif
> >>
> >> I'm wondering who had this strange idea... :)
> > 
> > I was very scared to type "git blame" here. :)  But the question is also
> 
> http://geek-and-poke.com/2013/11/24/simply-explained
> 
> BTW, it is a legacy from 2006:
> 
> 19cb373 better support of host drives
> 
> coming from MacOS X (again!):
> 
> 3b0d4f6 OS X: support for the built in CD-ROM drive (Mike Kronenberg)
> 
> > where to put the checks.  Putting it at a random place in block.c is not
> > a good idea.
> > 
> > But yes, this is also bad.  It should use stat and check the major/minor
> > numbers.
> 
> Yes, we should check if major is SCSI_GENERIC_MAJOR (21) (on linux).

That would be too specific since there are other drivers that support SG
ioctls, like block/bsg.c.

> We can also try to send an SG command like in cdrom_probe_device().
> Something like in scsi_generic_realize():
> 
>     rc = blk_ioctl(s->conf.blk, SG_GET_VERSION_NUM, &sg_version);
>     if (rc < 0) {
>         error_setg(errp, "cannot get SG_IO version number: %s.  "
>                          "Is this a SCSI device?",
>                          strerror(-rc));
>         return;
>     }

That was recently done in:

commit 3307ed7b3fac5ba99eb3b84904b0b7cdc3592a61
Author: Dimitris Aragiorgis <dimara@arrikto.com>
Date:   Tue Jun 23 13:45:00 2015 +0300

    raw-posix: Introduce hdev_is_sg()

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection

[Programmingkid]

On Jun 25, 2015, at 12:16 PM, Paolo Bonzini wrote:

> 
> 
> On 25/06/2015 18:12, Laurent Vivier wrote:
>> 
>> 
>> On 25/06/2015 17:48, Paolo Bonzini wrote:
>>> 
>>> On 25/06/2015 17:32, Programmingkid wrote:
>>>>> I think we are going to have to agree to disagree. I have never
>>>>> used the /dev/sr(0 | 1) devices and don't see how they would be
>>>>> effected by this patch. Are you trying to say the /dev/sr(0 | 1)
>>>>> devices *should* be handled by this patch?
>>>> 
>>>> Thinking about your question some more, I see what you mean. On Linux
>>>> /dev/sr0 refers to the cdrom drive. Also on Linux, the /dev/cdrom
>>>> link refers to the /dev/sr0 device file. So if you just use
>>>> /dev/cdrom, you are good.
>>> 
>>> Well, that's not how things work.
>>> 
>>> If you do things like that, you end up with a bunch of hacks, not with a
>>> decent piece of software.
>>> 
>>> There is support for CD-ROM passthrough on Linux and FreeBSD in
>>> block/raw-posix.c.  Perhaps the FreeBSD support can be extended to OS X
>>> as well.
>>> 
>> 
>> In fact, programmingkid, you should fix it in hdev_open() where there is
>> already a #if __APPLE__ .
>> 
>> Paolo, I agree with you but :
>> 
>> hdev_open()
>> 
>> #if defined(__linux__)
>>    {
>>        char resolved_path[ MAXPATHLEN ], *temp;
>> 
>>        temp = realpath(filename, resolved_path);
>>        if (temp && strstart(temp, "/dev/sg", NULL)) {
>>            bs->sg = 1;
>>        }
>> #endif
>> 
>> I'm wondering who had this strange idea... :)
> 
> I was very scared to type "git blame" here. :)  But the question is also
> where to put the checks.  Putting it at a random place in block.c is not
> a good idea.

I honestly think it is in the right place. The function find_image_format()
is doing just that - trying to find the format. The image part of the function's name
does bother me. But we could ignore it. Since we know it is a real cdrom drive,
 it would receive the format of raw. It seems as simple as that.

Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection

[Paolo Bonzini]

On 25/06/2015 20:07, Programmingkid wrote:
> I honestly think it is in the right place. The function find_image_format()
> is doing just that - trying to find the format. The image part of the function's name
> does bother me. But we could ignore it. Since we know it is a real cdrom drive,
>  it would receive the format of raw. It seems as simple as that.

But matching against the name in generic block.c code is just wrong.  It
_is_ as simple as that.

Paolo

Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection

[Programmingkid]

On Jun 25, 2015, at 12:12 PM, Laurent Vivier wrote:

> 
> 
> On 25/06/2015 17:48, Paolo Bonzini wrote:
>> 
>> On 25/06/2015 17:32, Programmingkid wrote:
>>>> I think we are going to have to agree to disagree. I have never
>>>> used the /dev/sr(0 | 1) devices and don't see how they would be
>>>> effected by this patch. Are you trying to say the /dev/sr(0 | 1)
>>>> devices *should* be handled by this patch?
>>> 
>>> Thinking about your question some more, I see what you mean. On Linux
>>> /dev/sr0 refers to the cdrom drive. Also on Linux, the /dev/cdrom
>>> link refers to the /dev/sr0 device file. So if you just use
>>> /dev/cdrom, you are good.
>> 
>> Well, that's not how things work.
>> 
>> If you do things like that, you end up with a bunch of hacks, not with a
>> decent piece of software.
>> 
>> There is support for CD-ROM passthrough on Linux and FreeBSD in
>> block/raw-posix.c.  Perhaps the FreeBSD support can be extended to OS X
>> as well.
>> 
> 
> In fact, programmingkid, you should fix it in hdev_open() where there is
> already a #if __APPLE__ .

Nice to hear from you again Laurent. The only way a solution in hdev_open() would work is if it could prevent find_image_format() from executing. Otherwise find_image_format() would just quit QEMU with an error.

Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection

[Paolo Bonzini]

On 25/06/2015 19:56, Programmingkid wrote:
>> In fact, programmingkid, you should fix it in hdev_open() where
>> there is already a #if __APPLE__ .
> 
> Nice to hear from you again Laurent. The only way a solution in
> hdev_open() would work is if it could prevent find_image_format()
> from executing. Otherwise find_image_format() would just quit QEMU
> with an error.

You have to implement an is_inserted function like this one that is
already there for FreeBSD:

static int cdrom_is_inserted(BlockDriverState *bs)
{
    return raw_getlength(bs) > 0;
}

The point you're changing is _already_ testing bdrv_is_inserted.

Look at the block starting like this:

#if defined (__FreeBSD__) || defined(__FreeBSD_kernel__)
static int cdrom_open(BlockDriverState *bs, QDict *options, int flags,
                      Error **errp)

and create a similar one that is #ifdef __APPLE__.  Feel free to remove
pieces that you don't know how to do in OS X / CoreFoundation.

Paolo

Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection

[Peter Maydell]

On 25 June 2015 at 18:56, Programmingkid <programmingkidx@gmail.com> wrote:
> Nice to hear from you again Laurent. The only way a solution in
> hdev_open() would work is if it could prevent find_image_format()
> from executing. Otherwise find_image_format() would just quit QEMU
> with an error.

The question you should be asking is "what is Linux doing for
raw CDROM devices that is different, such that it works there
but doesn't work on OSX?".

It would also be helpful to know which is the case that doesn't
work. Does QEMU fail in all cases, or only if the cdrom drive is
empty, or only if there's a disk in the drive?

My initial suspicion is that we need OSX support in raw-posix.c
for handling the host CDROM specially -- note that Linux and
FreeBSD register a bdrv_host_cdrom with an is_inserted
function.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection

[Programmingkid]

On Jun 25, 2015, at 2:01 PM, Peter Maydell wrote:

> On 25 June 2015 at 18:56, Programmingkid <programmingkidx@gmail.com> wrote:
>> Nice to hear from you again Laurent. The only way a solution in
>> hdev_open() would work is if it could prevent find_image_format()
>> from executing. Otherwise find_image_format() would just quit QEMU
>> with an error.
> 
> The question you should be asking is "what is Linux doing for
> raw CDROM devices that is different, such that it works there
> but doesn't work on OSX?".
> 
> It would also be helpful to know which is the case that doesn't
> work. Does QEMU fail in all cases, or only if the cdrom drive is
> empty, or only if there's a disk in the drive?

QEMU fails if the cdrom is specified "-cdrom /dev/cdrom", and there is no cd in the drive. 

QEMU also fails with a real cdrom in the drive. 

> 
> My initial suspicion is that we need OSX support in raw-posix.c
> for handling the host CDROM specially -- note that Linux and
> FreeBSD register a bdrv_host_cdrom with an is_inserted
> function.

The is_inserted function wouldn't make a difference. 

I did follow the execution of QEMU from find_image_format(). When bdrv_co_io_em() is called, it returns -22. This is where things appear to start to go wrong.

Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection

[Laurent Vivier]

Hi,

On 29/06/2015 01:43, Programmingkid wrote:
> 
> On Jun 25, 2015, at 2:01 PM, Peter Maydell wrote:
> 
>> On 25 June 2015 at 18:56, Programmingkid
>> <programmingkidx@gmail.com> wrote:
>>> Nice to hear from you again Laurent. The only way a solution in 
>>> hdev_open() would work is if it could prevent
>>> find_image_format() from executing. Otherwise find_image_format()
>>> would just quit QEMU with an error.
>> 
>> The question you should be asking is "what is Linux doing for raw
>> CDROM devices that is different, such that it works there but
>> doesn't work on OSX?".
>> 
>> It would also be helpful to know which is the case that doesn't 
>> work. Does QEMU fail in all cases, or only if the cdrom drive is 
>> empty, or only if there's a disk in the drive?
> 
> QEMU fails if the cdrom is specified "-cdrom /dev/cdrom", and there
> is no cd in the drive.
> 
> QEMU also fails with a real cdrom in the drive.
> 
>> 
>> My initial suspicion is that we need OSX support in raw-posix.c for
>> handling the host CDROM specially -- note that Linux and FreeBSD
>> register a bdrv_host_cdrom with an is_inserted function.
> 
> The is_inserted function wouldn't make a difference.

In fact, if your patch fixes the problem, the is_inserted with no
cdrom should too:

with your " strcmp("/dev/cdrom", filename) == 0 ", you force the
selection of bdrv_raw (which is what to do).

without your patch, if "bdrv_is_inserted()" was implemented and no cdrom
in the drive " !bdrv_is_inserted(bs)  " should also select bdrv_raw.

It appears also that bdrv_host_cdrom is not registered in
bdrv_file_init(). I think this is the missing part to have a host cdrom
support on MacOS X.

Laurent

Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection

[Programmingkid]

On Jun 28, 2015, at 8:29 PM, Laurent Vivier wrote:

> Hi,
> 
> On 29/06/2015 01:43, Programmingkid wrote:
>> 
>> On Jun 25, 2015, at 2:01 PM, Peter Maydell wrote:
>> 
>>> On 25 June 2015 at 18:56, Programmingkid
>>> <programmingkidx@gmail.com> wrote:
>>>> Nice to hear from you again Laurent. The only way a solution in 
>>>> hdev_open() would work is if it could prevent
>>>> find_image_format() from executing. Otherwise find_image_format()
>>>> would just quit QEMU with an error.
>>> 
>>> The question you should be asking is "what is Linux doing for raw
>>> CDROM devices that is different, such that it works there but
>>> doesn't work on OSX?".
>>> 
>>> It would also be helpful to know which is the case that doesn't 
>>> work. Does QEMU fail in all cases, or only if the cdrom drive is 
>>> empty, or only if there's a disk in the drive?
>> 
>> QEMU fails if the cdrom is specified "-cdrom /dev/cdrom", and there
>> is no cd in the drive.
>> 
>> QEMU also fails with a real cdrom in the drive.
>> 
>>> 
>>> My initial suspicion is that we need OSX support in raw-posix.c for
>>> handling the host CDROM specially -- note that Linux and FreeBSD
>>> register a bdrv_host_cdrom with an is_inserted function.
>> 
>> The is_inserted function wouldn't make a difference.
> 
> In fact, if your patch fixes the problem, the is_inserted with no
> cdrom should too:
> 
> with your " strcmp("/dev/cdrom", filename) == 0 ", you force the
> selection of bdrv_raw (which is what to do).
> 
> without your patch, if "bdrv_is_inserted()" was implemented and no cdrom
> in the drive " !bdrv_is_inserted(bs)  " should also select bdrv_raw.

The thing is a cdrom would be in the drive, and !bdrv_is_inserted() would return false. 

> 
> It appears also that bdrv_host_cdrom is not registered in
> bdrv_file_init(). I think this is the missing part to have a host cdrom
> support on MacOS X.

I don't think we need a bdrv_host_cdrom registered. If one tiny change could make the real cdrom drive work, why completely implement this structure? 

Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection

[Programmingkid]

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


On Jun 28, 2015, at 8:29 PM, Laurent Vivier wrote:

> Hi,
> 
> On 29/06/2015 01:43, Programmingkid wrote:
>> 
>> On Jun 25, 2015, at 2:01 PM, Peter Maydell wrote:
>> 
>>> On 25 June 2015 at 18:56, Programmingkid
>>> <programmingkidx@gmail.com> wrote:
>>>> Nice to hear from you again Laurent. The only way a solution in 
>>>> hdev_open() would work is if it could prevent
>>>> find_image_format() from executing. Otherwise find_image_format()
>>>> would just quit QEMU with an error.
>>> 
>>> The question you should be asking is "what is Linux doing for raw
>>> CDROM devices that is different, such that it works there but
>>> doesn't work on OSX?".
>>> 
>>> It would also be helpful to know which is the case that doesn't 
>>> work. Does QEMU fail in all cases, or only if the cdrom drive is 
>>> empty, or only if there's a disk in the drive?
>> 
>> QEMU fails if the cdrom is specified "-cdrom /dev/cdrom", and there
>> is no cd in the drive.
>> 
>> QEMU also fails with a real cdrom in the drive.
>> 
>>> 
>>> My initial suspicion is that we need OSX support in raw-posix.c for
>>> handling the host CDROM specially -- note that Linux and FreeBSD
>>> register a bdrv_host_cdrom with an is_inserted function.
>> 
>> The is_inserted function wouldn't make a difference.
> 
> In fact, if your patch fixes the problem, the is_inserted with no
> cdrom should too:
> 
> with your " strcmp("/dev/cdrom", filename) == 0 ", you force the
> selection of bdrv_raw (which is what to do).
> 
> without your patch, if "bdrv_is_inserted()" was implemented and no cdrom
> in the drive " !bdrv_is_inserted(bs)  " should also select bdrv_raw.
> 
> It appears also that bdrv_host_cdrom is not registered in
> bdrv_file_init(). I think this is the missing part to have a host cdrom
> support on MacOS X.
> 
> Laurent


This patch is what I came up with using your idea. It uses the fact that the bdrv_is_inserted() function is called first from find_image_format(). The bdrv_is_inserted() function now points to cdrom_is_inserted(). This new function will return 0 the first time it is called, then 1 after that. So far it works.


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

diff --git a/block/raw-posix.c b/block/raw-posix.c
index a967464..2d35580 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -2324,6 +2324,23 @@ static int hdev_create(const char *filename, QemuOpts *opts,
     return ret;
 }
 
+#ifdef __APPLE__
+
+static int cdrom_is_inserted(BlockDriverState *bs)
+{
+    static int count = 0;
+    int returnValue = 1;
+    
+    if(count == 0) {
+        returnValue = 0; // get around find_image_format() issue
+    }
+    
+    printf("count = %d for %s, returning %d\n", count, bs->filename, returnValue);
+    count++;
+    return returnValue;
+}
+#endif
+
 static BlockDriver bdrv_host_device = {
     .format_name        = "host_device",
     .protocol_name        = "host_device",
@@ -2365,6 +2382,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: 12334 bytes --]

Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection

[Laurent Vivier]

On 29/06/2015 05:01, Programmingkid wrote:
> 
> On Jun 28, 2015, at 8:29 PM, Laurent Vivier wrote:
> 
>> Hi,
>>
>> On 29/06/2015 01:43, Programmingkid wrote:
>>>
>>> On Jun 25, 2015, at 2:01 PM, Peter Maydell wrote:
>>>
>>>> On 25 June 2015 at 18:56, Programmingkid
>>>> <programmingkidx@gmail.com <mailto:programmingkidx@gmail.com>> wrote:
>>>>> Nice to hear from you again Laurent. The only way a solution in
>>>>> hdev_open() would work is if it could prevent
>>>>> find_image_format() from executing. Otherwise find_image_format()
>>>>> would just quit QEMU with an error.
>>>>
>>>> The question you should be asking is "what is Linux doing for raw
>>>> CDROM devices that is different, such that it works there but
>>>> doesn't work on OSX?".
>>>>
>>>> It would also be helpful to know which is the case that doesn't
>>>> work. Does QEMU fail in all cases, or only if the cdrom drive is
>>>> empty, or only if there's a disk in the drive?
>>>
>>> QEMU fails if the cdrom is specified "-cdrom /dev/cdrom", and there
>>> is no cd in the drive.
>>>
>>> QEMU also fails with a real cdrom in the drive.
>>>
>>>>
>>>> My initial suspicion is that we need OSX support in raw-posix.c for
>>>> handling the host CDROM specially -- note that Linux and FreeBSD
>>>> register a bdrv_host_cdrom with an is_inserted function.
>>>
>>> The is_inserted function wouldn't make a difference.
>>
>> In fact, if your patch fixes the problem, the is_inserted with no
>> cdrom should too:
>>
>> with your " strcmp("/dev/cdrom", filename) == 0 ", you force the
>> selection of bdrv_raw (which is what to do).
>>
>> without your patch, if "bdrv_is_inserted()" was implemented and no cdrom
>> in the drive " !bdrv_is_inserted(bs)  " should also select bdrv_raw.
>>
>> It appears also that bdrv_host_cdrom is not registered in
>> bdrv_file_init(). I think this is the missing part to have a host cdrom
>> support on MacOS X.
>>
>> Laurent
> 
> This patch is what I came up with using your idea. It uses the fact that
> the bdrv_is_inserted() function is called first from
> find_image_format(). The bdrv_is_inserted() function now points
> to cdrom_is_inserted(). This new function will return 0 the first time
> it is called, then 1 after that. So far it works.

Great.

> 
> ---
>  block/raw-posix.c |   21 +++++++++++++++++++++
>  1 files changed, 21 insertions(+), 0 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index a967464..2d35580 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -2324,6 +2324,23 @@ static int hdev_create(const char *filename,
> QemuOpts *opts,
>      return ret;
>  }
> 
>  
> 
> +#ifdef __APPLE__
> +
> +static int cdrom_is_inserted(BlockDriverState *bs)
> +{
> +    static int count = 0;
> +    int returnValue = 1;
> +    
> +    if(count == 0) {
> +        returnValue = 0; // get around find_image_format() issue
> +    }
> +    
> +    printf("count = %d for %s, returning %d\n", count, bs->filename,
> returnValue);
> +    count++;
> +    return returnValue;
> +}
> +#endif

So instead, read the size of the device, if it is 0, return false.

Something like for FreeBSD.

raw_getlength(bs), and

raw_getlength() should use an ioctl() to get the size.

I think you can use something like DKIOCGETBLOCKCOUNT.

http://www.opensource.apple.com/source/xnu/xnu-1456.1.26/bsd/sys/disk.h
https://developer.apple.com/library/mac/samplecode/CDROMSample/Listings/CDROMSample_CDROMSample_c.html

>  static BlockDriver bdrv_host_device = {
>      .format_name        = "host_device",
>      .protocol_name        = "host_device",
> @@ -2365,6 +2382,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
>  };

We need also cdrom_eject, and cdrom_lock_medium.

This example can help:

http://www.opensource.apple.com/source/DiskArbitration/DiskArbitration-156/disktool/disktool.c

Laurent

Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection

[Programmingkid]

On Jun 25, 2015, at 11:48 AM, Paolo Bonzini wrote:

> 
> On 25/06/2015 17:32, Programmingkid wrote:
>>> I think we are going to have to agree to disagree. I have never
>>> used the /dev/sr(0 | 1) devices and don't see how they would be
>>> effected by this patch. Are you trying to say the /dev/sr(0 | 1)
>>> devices *should* be handled by this patch?
>> 
>> Thinking about your question some more, I see what you mean. On Linux
>> /dev/sr0 refers to the cdrom drive. Also on Linux, the /dev/cdrom
>> link refers to the /dev/sr0 device file. So if you just use
>> /dev/cdrom, you are good.
> 
> Well, that's not how things work.
> 
> If you do things like that, you end up with a bunch of hacks, not with a
> decent piece of software.
> 
> There is support for CD-ROM passthrough on Linux and FreeBSD in
> block/raw-posix.c.  Perhaps the FreeBSD support can be extended to OS X
> as well.

Could you be more specific? A name of a function would be great.

Re: [Qemu-devel] [Qemu-block] [PATCH] block.c: fix real cdrom detection

[Stefan Hajnoczi]

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

On Tue, Jun 23, 2015 at 02:26:51PM -0400, Programmingkid wrote:
> 
> On Jun 23, 2015, at 2:06 PM, John Snow wrote:
> 
> > 
> > 
> > On 06/23/2015 01:56 PM, Programmingkid wrote:
> >> Fix real cdrom detection so that a real cdrom can actually be used.
> >> 
> >> signed-off-by: John Arbuckle <programmingkidx@gmail.com
> >> <mailto:programmingkidx@gmail.com>>
> >> 
> >> This patch has been tested on Mac OS X host and guest. 
> >> Command used: qemu-system-ppc -cdrom /dev/cdrom
> >> 
> >> Note: I was able to view the files using OpenBIOS, but not on 
> >> Mac OS X. The size of the disc is reported correctly but some
> >> error happens that prevents it from mounting in Mac OS X. This
> >> is probably another bug with QEMU.
> >> 
> >> ---
> >> block.c |    3 ++-
> >> 1 files changed, 2 insertions(+), 1 deletions(-)
> >> 
> >> diff --git a/block.c b/block.c
> >> index dd4f58d..75ccfad 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -583,7 +583,8 @@ static int find_image_format(BlockDriverState *bs,
> >> const char *filename,
> >>     int ret = 0;
> >> 
> >> 
> >> 
> >>     /* Return the raw BlockDriver * to scsi-generic devices or empty
> >> drives */
> >> -    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0) {
> >> +    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0 
> >> +               || strcmp("/dev/cdrom", filename) == 0) {
> >>         *pdrv = &bdrv_raw;
> >>         return ret;
> >>     }
> >> -- 
> >> 1.7.5.4
> >> 
> > 
> > So what's the issue that this patch attempts to fix and how did you
> > determine that the fix was needed here? It doesn't look like it respects
> > proper abstraction at a glance.
> 
> Without the patch, QEMU would just quit when the "-cdrom /dev/cdrom" option is given.

Why does it quit?

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] block.c: fix real cdrom detection

[Programmingkid]

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


On Jun 25, 2015, at 9:31 AM, Stefan Hajnoczi wrote:

> On Tue, Jun 23, 2015 at 02:26:51PM -0400, Programmingkid wrote:
>> 
>> On Jun 23, 2015, at 2:06 PM, John Snow wrote:
>> 
>>> 
>>> 
>>> On 06/23/2015 01:56 PM, Programmingkid wrote:
>>>> Fix real cdrom detection so that a real cdrom can actually be used.
>>>> 
>>>> signed-off-by: John Arbuckle <programmingkidx@gmail.com
>>>> <mailto:programmingkidx@gmail.com>>
>>>> 
>>>> This patch has been tested on Mac OS X host and guest. 
>>>> Command used: qemu-system-ppc -cdrom /dev/cdrom
>>>> 
>>>> Note: I was able to view the files using OpenBIOS, but not on 
>>>> Mac OS X. The size of the disc is reported correctly but some
>>>> error happens that prevents it from mounting in Mac OS X. This
>>>> is probably another bug with QEMU.
>>>> 
>>>> ---
>>>> block.c |    3 ++-
>>>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>>> 
>>>> diff --git a/block.c b/block.c
>>>> index dd4f58d..75ccfad 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -583,7 +583,8 @@ static int find_image_format(BlockDriverState *bs,
>>>> const char *filename,
>>>>    int ret = 0;
>>>> 
>>>> 
>>>> 
>>>>    /* Return the raw BlockDriver * to scsi-generic devices or empty
>>>> drives */
>>>> -    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0) {
>>>> +    if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0 
>>>> +               || strcmp("/dev/cdrom", filename) == 0) {
>>>>        *pdrv = &bdrv_raw;
>>>>        return ret;
>>>>    }
>>>> -- 
>>>> 1.7.5.4
>>>> 
>>> 
>>> So what's the issue that this patch attempts to fix and how did you
>>> determine that the fix was needed here? It doesn't look like it respects
>>> proper abstraction at a glance.
>> 
>> Without the patch, QEMU would just quit when the "-cdrom /dev/cdrom" option is given.
> 
> Why does it quit?

Because of a bug. This is what it prints: "Could not read image for determining its format: Invalid argument". This message seems to indicate that QEMU thinks the real cdrom drive is an image file. 

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

Re: [Qemu-devel] [Qemu-block] [PATCH] block.c: fix real cdrom detection

[Stefan Hajnoczi]

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

On Thu, Jun 25, 2015 at 11:11:24AM -0400, Programmingkid wrote:
> On Jun 25, 2015, at 9:31 AM, Stefan Hajnoczi wrote:
> > On Tue, Jun 23, 2015 at 02:26:51PM -0400, Programmingkid wrote:
> >> On Jun 23, 2015, at 2:06 PM, John Snow wrote:
> >>> So what's the issue that this patch attempts to fix and how did you
> >>> determine that the fix was needed here? It doesn't look like it respects
> >>> proper abstraction at a glance.
> >> 
> >> Without the patch, QEMU would just quit when the "-cdrom /dev/cdrom" option is given.
> > 
> > Why does it quit?
> 
> Because of a bug. This is what it prints: "Could not read image for determining its format: Invalid argument".

The bdrv_pread() failure is what need you need to investigate.  In the
other sub-thread there have been hints about adding CD-ROM passthrough
support on Mac OS X by filling in the missing parts in
block/raw-posix.c.  That should help you get to the bottom of the
problem.

> This message seems to indicate that QEMU thinks the real cdrom drive is an image file. 

If the -drive format= option is not given, QEMU will try to detect the
image format.  That's the purpose of the find_image_format() function.

QEMU does not make a distinction between image files and block devices
because there are valid use cases where a block device uses an image
format.  For example, a disk or partition can contain qcow2 data (there
is no partition table or file system, just qcow2).

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] block.c: fix real cdrom detection

[Programmingkid]

On Jun 26, 2015, at 5:34 AM, Stefan Hajnoczi wrote:

> On Thu, Jun 25, 2015 at 11:11:24AM -0400, Programmingkid wrote:
>> On Jun 25, 2015, at 9:31 AM, Stefan Hajnoczi wrote:
>>> On Tue, Jun 23, 2015 at 02:26:51PM -0400, Programmingkid wrote:
>>>> On Jun 23, 2015, at 2:06 PM, John Snow wrote:
>>>>> So what's the issue that this patch attempts to fix and how did you
>>>>> determine that the fix was needed here? It doesn't look like it respects
>>>>> proper abstraction at a glance.
>>>> 
>>>> Without the patch, QEMU would just quit when the "-cdrom /dev/cdrom" option is given.
>>> 
>>> Why does it quit?
>> 
>> Because of a bug. This is what it prints: "Could not read image for determining its format: Invalid argument".
> 
> The bdrv_pread() failure is what need you need to investigate.  In the
> other sub-thread there have been hints about adding CD-ROM passthrough
> support on Mac OS X by filling in the missing parts in
> block/raw-posix.c.  That should help you get to the bottom of the
> problem.
> 
>> This message seems to indicate that QEMU thinks the real cdrom drive is an image file. 
> 
> If the -drive format= option is not given, QEMU will try to detect the
> image format.  That's the purpose of the find_image_format() function.
> 
> QEMU does not make a distinction between image files and block devices
> because there are valid use cases where a block device uses an image
> format.  For example, a disk or partition can contain qcow2 data (there
> is no partition table or file system, just qcow2).

So what you are saying is if the user enters "-cdrom /dev/cdrom", QEMU is suppose to call find_image_format. If everything goes right, the first if statement should be skipped. Then the bdrv_pread() function should succeed and so should the bdrv_probe_all() function. Is that how it is suppose to work?

Re: [Qemu-devel] [Qemu-block] [PATCH] block.c: fix real cdrom detection

[Stefan Hajnoczi]

On Fri, Jun 26, 2015 at 4:50 PM, Programmingkid
<programmingkidx@gmail.com> wrote:
>
> On Jun 26, 2015, at 5:34 AM, Stefan Hajnoczi wrote:
>
>> On Thu, Jun 25, 2015 at 11:11:24AM -0400, Programmingkid wrote:
>>> On Jun 25, 2015, at 9:31 AM, Stefan Hajnoczi wrote:
>>>> On Tue, Jun 23, 2015 at 02:26:51PM -0400, Programmingkid wrote:
>>>>> On Jun 23, 2015, at 2:06 PM, John Snow wrote:
>>>>>> So what's the issue that this patch attempts to fix and how did you
>>>>>> determine that the fix was needed here? It doesn't look like it respects
>>>>>> proper abstraction at a glance.
>>>>>
>>>>> Without the patch, QEMU would just quit when the "-cdrom /dev/cdrom" option is given.
>>>>
>>>> Why does it quit?
>>>
>>> Because of a bug. This is what it prints: "Could not read image for determining its format: Invalid argument".
>>
>> The bdrv_pread() failure is what need you need to investigate.  In the
>> other sub-thread there have been hints about adding CD-ROM passthrough
>> support on Mac OS X by filling in the missing parts in
>> block/raw-posix.c.  That should help you get to the bottom of the
>> problem.
>>
>>> This message seems to indicate that QEMU thinks the real cdrom drive is an image file.
>>
>> If the -drive format= option is not given, QEMU will try to detect the
>> image format.  That's the purpose of the find_image_format() function.
>>
>> QEMU does not make a distinction between image files and block devices
>> because there are valid use cases where a block device uses an image
>> format.  For example, a disk or partition can contain qcow2 data (there
>> is no partition table or file system, just qcow2).
>
> So what you are saying is if the user enters "-cdrom /dev/cdrom", QEMU is suppose to call find_image_format. If everything goes right, the first if statement should be skipped. Then the bdrv_pread() function should succeed and so should the bdrv_probe_all() function. Is that how it is suppose to work?

Exactly.  bdrv_pread() will grab some data from the start of the
CD-ROM.  No qcow2, vmdk, etc header will be found (there is probably
an ISO file system there), so QEMU will default to the raw format and
everything will work as expected.  This is also how CD-ROM passthrough
works on Linux and FreeBSD.

Re: [Qemu-devel] [Qemu-block] [PATCH] block.c: fix real cdrom detection

[Stefan Hajnoczi]

On Wed, Jul 1, 2015 at 4:35 PM, Programmingkid
<programmingkidx@gmail.com> wrote:
> On Jun 26, 2015, at 4:01 PM, Stefan Hajnoczi wrote:
>> On Fri, Jun 26, 2015 at 4:50 PM, Programmingkid
>> <programmingkidx@gmail.com> wrote:
>>> On Jun 26, 2015, at 5:34 AM, Stefan Hajnoczi wrote:
>>>> On Thu, Jun 25, 2015 at 11:11:24AM -0400, Programmingkid wrote:
>>>>> On Jun 25, 2015, at 9:31 AM, Stefan Hajnoczi wrote:
>>>>>> On Tue, Jun 23, 2015 at 02:26:51PM -0400, Programmingkid wrote:
>>>>>>> On Jun 23, 2015, at 2:06 PM, John Snow wrote:
>>>>>>>> So what's the issue that this patch attempts to fix and how did you
>>>>>>>> determine that the fix was needed here? It doesn't look like it respects
>>>>>>>> proper abstraction at a glance.
>>>>>>>
>>>>>>> Without the patch, QEMU would just quit when the "-cdrom /dev/cdrom" option is given.
>>>>>>
>>>>>> Why does it quit?
>>>>>
>>>>> Because of a bug. This is what it prints: "Could not read image for determining its format: Invalid argument".
>>>>
>>>> The bdrv_pread() failure is what need you need to investigate.  In the
>>>> other sub-thread there have been hints about adding CD-ROM passthrough
>>>> support on Mac OS X by filling in the missing parts in
>>>> block/raw-posix.c.  That should help you get to the bottom of the
>>>> problem.
>>>>
>>>>> This message seems to indicate that QEMU thinks the real cdrom drive is an image file.
>>>>
>>>> If the -drive format= option is not given, QEMU will try to detect the
>>>> image format.  That's the purpose of the find_image_format() function.
>>>>
>>>> QEMU does not make a distinction between image files and block devices
>>>> because there are valid use cases where a block device uses an image
>>>> format.  For example, a disk or partition can contain qcow2 data (there
>>>> is no partition table or file system, just qcow2).
>>>
>>> So what you are saying is if the user enters "-cdrom /dev/cdrom", QEMU is suppose to call find_image_format. If everything goes right, the first if statement should be skipped. Then the bdrv_pread() function should succeed and so should the bdrv_probe_all() function. Is that how it is suppose to work?
>>
>> Exactly.  bdrv_pread() will grab some data from the start of the
>> CD-ROM.  No qcow2, vmdk, etc header will be found (there is probably
>> an ISO file system there), so QEMU will default to the raw format and
>> everything will work as expected.  This is also how CD-ROM passthrough
>> works on Linux and FreeBSD.
>
>
> I need some help. In the function bdrv_co_io_em(), the co argument's ret field is changed to -22. Would you know where exactly it is set? That question again is where is the co.ret variable set? I have tried stepping thru the function, but the change always escapes me.

Please keep qemu-devel@nongnu.org CCed so others can participate in
the discussion.

The return value comes from:

        acb = bs->drv->bdrv_aio_readv(bs, sector_num, iov, nb_sectors,
                                      bdrv_co_io_em_complete, &co);

When the read request completes it invokes the callback.  In this case
it's bdrv_co_io_em_complete().  That function fills in co.ret.

You'll probably find that ->bdrv_aio_readv() points to
block/raw-posix.c:raw_aio_readv().  This is the block driver for raw
image files on POSIX systems (Mac OS, Linux, etc).

The function doing the actual preadv(2) system call is
handle_aiocb_rw() and its children.

Hope this helps.

Stefan