On 2015-03-04 at 09:00, Kevin Wolf wrote:
Am 09.02.2015 um 18:11 hat Max Reitz geschrieben:
The tray of an FDD is open iff there is no medium inserted (there are
only two states for an FDD: "medium inserted" or "no medium inserted").
This results in the tray being reported as open if qemu has been started
with the default floppy drive, which breaks some tests. Fix them.
Signed-off-by: Max Reitz <mre...@redhat.com>
---
hw/block/fdc.c | 20 +++++++++++++---
tests/fdc-test.c | 4 +---
tests/qemu-iotests/067.out | 60 +++++++---------------------------------------
tests/qemu-iotests/071.out | 2 --
tests/qemu-iotests/081.out | 1 -
tests/qemu-iotests/087.out | 6 -----
6 files changed, 26 insertions(+), 67 deletions(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 2bf87c9..0c5a6b4 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -192,6 +192,8 @@ typedef struct FDrive {
uint8_t ro; /* Is read-only */
uint8_t media_changed; /* Is media changed */
uint8_t media_rate; /* Data rate of medium */
+
+ bool media_inserted; /* Is there a medium in the tray */
} FDrive;
static void fd_init(FDrive *drv)
@@ -261,7 +263,7 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t
track, uint8_t sect,
#endif
drv->head = head;
if (drv->track != track) {
- if (drv->blk != NULL && blk_is_inserted(drv->blk)) {
+ if (drv->media_inserted) {
I suspect that with the removal of blk_is_inserted() in several places,
floppy passthrough (host_floppy block driver) is now even more broken
than before, potentially not noticing removal of a medium any more.
While checking this, I noticed that since commit 21fcf360,
bdrv_media_changed() is completely unused. Media change has therefore
probably been broken since at least May 2012.
Considering this, it might actually be reasonable enough to remove the
block driver. It's definitely better than having it there, but not
working any better than host_device.
Of course, alternatively you would also be welcome to fix the device
model and reintroduce bdrv_media_changed() and blk_is_inserted() calls
where necessary.
Status:
I thought about why I need this tray status at all. The result is: We
need it because otherwise blockdev-close-tray doesn't do anything on
floppy disk drives (you'd insert a medium with blockdev-insert-medium
and the tray would immediately be reported as being closed, without any
TRAY_MOVED event). So we do need it.
I tested floppy passthrough in a VM and it's horror, as can be expected.
For instance, one cannot start qemu with floppy passthrough if the host
drive is empty (which is actually helpful).
So what I'll be doing is set media_inserted to load && drv->blk in the
media change CB and set it to true in the floppy drive initialization
code if there is a BB and blk_is_inserted() is true. The tray will be
considered closed if media_inserted && blk_is_inserted().
So why do I check blk_is_inserted() in the initialization code? If we
just set media_inserted to true, that would be wrong. Imagine an empty
drive (no BDS tree), media_inserted will be set to true, then you
inserted a BDS tree (blockdev-insert-medium) and the tray will be
considered closed without you having executed blockdev-close-tray
(media_inserted is true, and now blk_is_inserted() is true as well).
So I have to check whether there's a BDS tree in the initialization
code, if there isn't, media_inserted must be false. Why am I not using
blk_bs() for that? I tried to explain for the USB patch: blk_bs()
returns a BDS, and that's something for the block layer, nobody else
should care about that.
How could blk_is_inserted() be wrong here, if blk_bs() is the thing that
will work? Correct, if blk_bs() && !bdrv_is_inserted(blk_bs()). However,
this will never be the case, because as said above, qemu actually cannot
start with a host floppy drive without a medium, so
!!blk_is_inserted(blk) == !!blk_bs(blk).
I see the question popping up "Why don't you just add a bool has_bs(BB)
{ return blk_bs(BB); } and then not add that test to blk_is_inserted()"?
I've asked that myself. Answer: Again, anything outside of the block
layer should not care about things like BDS trees. But moreover,
bdrv_is_inserted() does not only check whether all the host devices
represented by BDSs are inserted, but also whether BDS.drv != NULL,
which until this series was the sign for an empty drive. Therefore,
checking blk_is_inserted() is the logical conclusion of
bdrv_is_inserted() (bdrv_is_inserted() tests whether BDS.drv != NULL,
blk_is_inserted() tests whether BLK.bs != NULL).
But medium management is now done on the BB level, so a separate
function for checking whether there's a BDS tree (because that should be
equivalent to "whether there's a medium") seems to have its merits.
However, I don't think so. blk_is_inserted() is exactly that function:
It's true if there is a BDS tree and, if there is host passthrough,
whether all the media are inserted, which is correct.
In theory, guest models should not have to distinguish whether a BB does
not have a medium because there is no BDS tree or because we're using
passthrough and that BDS does not have a medium. So why does floppy have
to distinguish? Because it does not have a real tray, but that's the
model we have to be working with. Inserting a medium into a
passed-through host drive must result in the tray being considered
closed immediately; inserting a medium into a guest device through
blockdev-insert-medium must not.
So our bane is that we need tray status for floppy disks but they don't
have a tray, so inserting a medium in a host drive does something
different than doing the same in a purely virtual drive. Dropping
host_floppy probably solves that problem, but I'm too cautious for that.
Max
