Re: [Qemu-block] [Qemu-devel] [PATCH] raw-posix.c: cd_is_inserted() implementation for Mac OS X
On 29 June 2015 at 17:54, Programmingkid programmingk...@gmail.com wrote:
@@ -2365,6 +2384,10 @@ static BlockDriver bdrv_host_device = {
.bdrv_ioctl = hdev_ioctl,
.bdrv_aio_ioctl = hdev_aio_ioctl,
#endif
+
+#ifdef __APPLE__
+.bdrv_is_inserted = cdrom_is_inserted,
+#endif
Why isn't this handled by having a bdrv_host_cdrom,
like Linux and FreeBSD do for their CDROM support?
-- PMM
Re: [Qemu-block] [Qemu-devel] [PATCH] raw-posix.c: cd_is_inserted() implementation for Mac OS X
On Jun 29, 2015, at 1:11 PM, Peter Maydell wrote:
On 29 June 2015 at 17:54, Programmingkid programmingk...@gmail.com wrote:
@@ -2365,6 +2384,10 @@ static BlockDriver bdrv_host_device = {
.bdrv_ioctl = hdev_ioctl,
.bdrv_aio_ioctl = hdev_aio_ioctl,
#endif
+
+#ifdef __APPLE__
+.bdrv_is_inserted = cdrom_is_inserted,
+#endif
Why isn't this handled by having a bdrv_host_cdrom,
like Linux and FreeBSD do for their CDROM support?
That would involve a lot of unnecessary work and modifications. This small
change is all that is needed.
Re: [Qemu-block] [Qemu-devel] [PATCH] raw-posix.c: cd_is_inserted() implementation for Mac OS X
On Jun 29, 2015, at 2:16 PM, Peter Maydell wrote:
On 29 June 2015 at 19:04, Programmingkid programmingk...@gmail.com wrote:
On Jun 29, 2015, at 1:11 PM, Peter Maydell wrote:
On 29 June 2015 at 17:54, Programmingkid programmingk...@gmail.com wrote:
@@ -2365,6 +2384,10 @@ static BlockDriver bdrv_host_device = {
.bdrv_ioctl = hdev_ioctl,
.bdrv_aio_ioctl = hdev_aio_ioctl,
#endif
+
+#ifdef __APPLE__
+.bdrv_is_inserted = cdrom_is_inserted,
+#endif
Why isn't this handled by having a bdrv_host_cdrom,
like Linux and FreeBSD do for their CDROM support?
That would involve a lot of unnecessary work and modifications. This
small change is all that is needed.
Yes, but it's obviously wrong, because this:
+if (count == 0) {
+count++;
+returnValue = 0; /* get around find_image_format() issue */
+}
makes no sense at all -- this means that we'll always report drive
empty the first time this function is called. We should always
report the correct answer, regardless of who's calling us.
If you find yourself writing this kind of weird workaround, it
generally suggests that the change is a this happens to make it
work patch, not the correct fix for the problem. We need clean
fixes in QEMU, because if we allow happens to make it work
patches to pile up then the whole system becomes unmaintainable.
Yes, this often means that the amount of work required to
fix a bug is more than a handful of lines. That doesn't mean
that the work is unnecessary.
(For instance, what happens if somebody changes some other
part of QEMU so that it happens that find_image_format() is not
the first thing to call this function?)
We know the correct way to support host cdrom drives, because
we're already doing that on Linux. We should consistently
support host cdrom drives the same way for all hosts.
I have really tried to find out what was wrong. It is a asynchronous,
multi-threaded mess. Trying to follow where QEMU messes up
was hard. The closest I came to was to a function called
bdrv_co_io_em(). It was returning a value of -22.
If some change does happen to make this patch to
not work anymore, I can easily fix it.
Re: [Qemu-block] [Qemu-devel] [PATCH] raw-posix.c: cd_is_inserted() implementation for Mac OS X
On 29 June 2015 at 19:04, Programmingkid programmingk...@gmail.com wrote:
On Jun 29, 2015, at 1:11 PM, Peter Maydell wrote:
On 29 June 2015 at 17:54, Programmingkid programmingk...@gmail.com wrote:
@@ -2365,6 +2384,10 @@ static BlockDriver bdrv_host_device = {
.bdrv_ioctl = hdev_ioctl,
.bdrv_aio_ioctl = hdev_aio_ioctl,
#endif
+
+#ifdef __APPLE__
+.bdrv_is_inserted = cdrom_is_inserted,
+#endif
Why isn't this handled by having a bdrv_host_cdrom,
like Linux and FreeBSD do for their CDROM support?
That would involve a lot of unnecessary work and modifications. This
small change is all that is needed.
Yes, but it's obviously wrong, because this:
+if (count == 0) {
+count++;
+returnValue = 0; /* get around find_image_format() issue */
+}
makes no sense at all -- this means that we'll always report drive
empty the first time this function is called. We should always
report the correct answer, regardless of who's calling us.
If you find yourself writing this kind of weird workaround, it
generally suggests that the change is a this happens to make it
work patch, not the correct fix for the problem. We need clean
fixes in QEMU, because if we allow happens to make it work
patches to pile up then the whole system becomes unmaintainable.
Yes, this often means that the amount of work required to
fix a bug is more than a handful of lines. That doesn't mean
that the work is unnecessary.
(For instance, what happens if somebody changes some other
part of QEMU so that it happens that find_image_format() is not
the first thing to call this function?)
We know the correct way to support host cdrom drives, because
we're already doing that on Linux. We should consistently
support host cdrom drives the same way for all hosts.
thanks
-- PMM
Re: [Qemu-block] [Qemu-devel] [PATCH] raw-posix.c: cd_is_inserted() implementation for Mac OS X
On Jun 29, 2015, at 4:43 PM, Laurent Vivier wrote:
On 29/06/2015 20:37, Programmingkid wrote:
On Jun 29, 2015, at 2:16 PM, Peter Maydell wrote:
On 29 June 2015 at 19:04, Programmingkid programmingk...@gmail.com
mailto:programmingk...@gmail.com wrote:
On Jun 29, 2015, at 1:11 PM, Peter Maydell wrote:
On 29 June 2015 at 17:54, Programmingkid programmingk...@gmail.com
mailto:programmingk...@gmail.com wrote:
@@ -2365,6 +2384,10 @@ static BlockDriver bdrv_host_device = {
.bdrv_ioctl = hdev_ioctl,
.bdrv_aio_ioctl = hdev_aio_ioctl,
#endif
+
+#ifdef __APPLE__
+.bdrv_is_inserted = cdrom_is_inserted,
+#endif
Why isn't this handled by having a bdrv_host_cdrom,
like Linux and FreeBSD do for their CDROM support?
That would involve a lot of unnecessary work and modifications. This
small change is all that is needed.
Yes, but it's obviously wrong, because this:
+if (count == 0) {
+count++;
+returnValue = 0; /* get around find_image_format() issue */
+}
makes no sense at all -- this means that we'll always report drive
empty the first time this function is called. We should always
report the correct answer, regardless of who's calling us.
If you find yourself writing this kind of weird workaround, it
generally suggests that the change is a this happens to make it
work patch, not the correct fix for the problem. We need clean
fixes in QEMU, because if we allow happens to make it work
patches to pile up then the whole system becomes unmaintainable.
Yes, this often means that the amount of work required to
fix a bug is more than a handful of lines. That doesn't mean
that the work is unnecessary.
(For instance, what happens if somebody changes some other
part of QEMU so that it happens that find_image_format() is not
the first thing to call this function?)
We know the correct way to support host cdrom drives, because
we're already doing that on Linux. We should consistently
support host cdrom drives the same way for all hosts.
I have really tried to find out what was wrong. It is a asynchronous,
multi-threaded mess. Trying to follow where QEMU messes up
was hard. The closest I came to was to a function called
bdrv_co_io_em(). It was returning a value of -22.
If some change does happen to make this patch to
not work anymore, I can easily fix it.
Frankly, I don't understand you.
The only thing you have to do is to write:
static int cdrom_is_inserted(BlockDriverState *bs)
{
return raw_getlength(bs) 0;
}
Yes, this is probably the correct implementation for cdrom_is_inserted(), but
what I'm trying to do is to make a real cdrom work in QEMU. This
implementation of cdrom_is_inserted() doesn't solve the problem with
find_image_format(). The problem still causes QEMU to quit when using
the option -cdrom /dev/cdrom.
My patch I sent you before does fix things, but it is viewed as a hack. I was
hoping the patch might be temporarily accepted until better solution was made.
That is not going to happen :(
Re: [Qemu-block] [Qemu-devel] [PATCH] raw-posix.c: cd_is_inserted() implementation for Mac OS X
On 29/06/2015 20:37, Programmingkid wrote:
On Jun 29, 2015, at 2:16 PM, Peter Maydell wrote:
On 29 June 2015 at 19:04, Programmingkid programmingk...@gmail.com
mailto:programmingk...@gmail.com wrote:
On Jun 29, 2015, at 1:11 PM, Peter Maydell wrote:
On 29 June 2015 at 17:54, Programmingkid programmingk...@gmail.com
mailto:programmingk...@gmail.com wrote:
@@ -2365,6 +2384,10 @@ static BlockDriver bdrv_host_device = {
.bdrv_ioctl = hdev_ioctl,
.bdrv_aio_ioctl = hdev_aio_ioctl,
#endif
+
+#ifdef __APPLE__
+.bdrv_is_inserted = cdrom_is_inserted,
+#endif
Why isn't this handled by having a bdrv_host_cdrom,
like Linux and FreeBSD do for their CDROM support?
That would involve a lot of unnecessary work and modifications. This
small change is all that is needed.
Yes, but it's obviously wrong, because this:
+if (count == 0) {
+count++;
+returnValue = 0; /* get around find_image_format() issue */
+}
makes no sense at all -- this means that we'll always report drive
empty the first time this function is called. We should always
report the correct answer, regardless of who's calling us.
If you find yourself writing this kind of weird workaround, it
generally suggests that the change is a this happens to make it
work patch, not the correct fix for the problem. We need clean
fixes in QEMU, because if we allow happens to make it work
patches to pile up then the whole system becomes unmaintainable.
Yes, this often means that the amount of work required to
fix a bug is more than a handful of lines. That doesn't mean
that the work is unnecessary.
(For instance, what happens if somebody changes some other
part of QEMU so that it happens that find_image_format() is not
the first thing to call this function?)
We know the correct way to support host cdrom drives, because
we're already doing that on Linux. We should consistently
support host cdrom drives the same way for all hosts.
I have really tried to find out what was wrong. It is a asynchronous,
multi-threaded mess. Trying to follow where QEMU messes up
was hard. The closest I came to was to a function called
bdrv_co_io_em(). It was returning a value of -22.
If some change does happen to make this patch to
not work anymore, I can easily fix it.
Frankly, I don't understand you.
The only thing you have to do is to write:
static int cdrom_is_inserted(BlockDriverState *bs)
{
return raw_getlength(bs) 0;
}
You have introduced yourself the support for raw_getlength() for MacOS X:
commit 728dacbda817b2ca259e9d337fab06bcf14e94a6
Author: Programmingkid programmingk...@gmail.com
Date: Mon Jan 19 17:12:55 2015 -0500
block/raw-posix.c: Fix raw_getlength() on Mac OS X block devices
This patch replaces the dummy code in raw_getlength() for block devices
on OS X, which always returned LLONG_MAX, with a real implementation
that returns the actual block device size.
Then, just #ifdef CONFIG_BSD around the existing bdrv_host_cdrom of
FreeBSD (minus cdrom_eject and cdrom_lock_medium) and bdrv_register().
Laurent
