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

[Stefan Hajnoczi]

On Wed, Jul 1, 2015 at 4:35 PM, Programmingkid
 wrote:
> On Jun 26, 2015, at 4:01 PM, Stefan Hajnoczi wrote:
>> On Fri, Jun 26, 2015 at 4:50 PM, Programmingkid
>>  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

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

[Stefan Hajnoczi]

On Fri, Jun 26, 2015 at 4:50 PM, Programmingkid
 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

[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 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).


pgpSNAzWyGHZq.pgp
Description: PGP signature

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

[Stefan Hajnoczi]

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 
Date:   Tue Jun 23 13:45:00 2015 +0300

raw-posix: Introduce hdev_is_sg()


pgp_jTnsrpmSA.pgp
Description: PGP signature

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

[Programmingkid]

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 >>> >
 
 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. 

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

[Stefan Hajnoczi]

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  >> >
> >> 
> >> 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?


pgpMaLuar6Dml.pgp
Description: PGP signature