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