Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection
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
programmingk...@gmail.com mailto:programmingk...@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
On Jun 25, 2015, at 2:01 PM, Peter Maydell wrote: On 25 June 2015 at 18:56, Programmingkid programmingk...@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
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 programmingk...@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
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 programmingk...@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
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
programmingk...@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
Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection
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] [PATCH] block.c: fix real cdrom detection
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
On Jun 25, 2015, at 2:53 AM, Markus Armbruster wrote:
Programmingkid programmingk...@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 programmingk...@gmail.com
mailto:programmingk...@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
On Jun 25, 2015, at 11:14 AM, Programmingkid wrote:
On Jun 25, 2015, at 2:53 AM, Markus Armbruster wrote:
Programmingkid programmingk...@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 programmingk...@gmail.com
mailto:programmingk...@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
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 programmingk...@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 programmingk...@gmail.com
mailto:programmingk...@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
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
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
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
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
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
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] [PATCH] block.c: fix real cdrom detection
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
On 25 June 2015 at 18:56, Programmingkid programmingk...@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
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
Programmingkid programmingk...@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 programmingk...@gmail.com
mailto:programmingk...@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.
[Qemu-devel] [PATCH] block.c: fix real cdrom detection
Fix real cdrom detection so that a real cdrom can actually be used.
signed-off-by: John Arbuckle programmingk...@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
Re: [Qemu-devel] [PATCH] block.c: fix real cdrom detection
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 programmingk...@gmail.com
mailto:programmingk...@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
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 programmingk...@gmail.com
mailto:programmingk...@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.
