[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 03/37] hw/block/fdc: Implement tray status
|
From: |
Max Reitz |
|
Subject: |
Re: [Qemu-devel] [PATCH v2 03/37] hw/block/fdc: Implement tray status |
|
Date: |
Mon, 16 Mar 2015 09:36:51 -0400 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 |
On 2015-03-05 at 05:11, Kevin Wolf wrote:
Am 04.03.2015 um 23:06 hat Max Reitz geschrieben:
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 <address@hidden>
---
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.
This is silly, Max.
Correctness isn't defined by what you _should_ care about, but by what
you actually _need_ to know. This is where you need to start your
considerations, not with what you think you're supposed to be asking.
If the two contradict, then there are two options: Either the assumption
that you shouldn't care is wrong, or your decision that you need the
information is. This conflict needs to be resolved rather than just
working with the answer to a different question.
In this case, I think the problem starts with "empty drive (no BDS
tree)".
I meant "Imagine an empty drive without a BDS tree" or "Imagine a drive
without a BDS tree, thus empty.", so rather "an empty drive, e.g. no BDS
tree" than "an empty drive, i.e. no BDS tree".
This is not correct, an empty drive is defined by
!blk_is_inserted(bs) and not by an empty BDS tree. The passthrough case
(with a hypothetical correct implementation) helps you distinguish the
two. So your assumption that you really need to check whether there is
no BDS tree is wrong here. You need to check whether there is a medium,
and that is blk_is_inserted(). (So yes, I agree with your conclusion,
but not with the way you got there.)
So what I was justifying is that blk_bs() will work, not that
blk_is_inserted() will not work. So why do I think that
blk_is_inserted() is not really what we want here?
Okay, so imagine the case where !!blk_bs() != blk_is_inserted() at
startup. That means, the host drive is empty. Now we insert a medium in
the host drive. Floppy being floppy, qemu won't have any idea that a
medium has been inserted. This means that media_inserted will still be
false and any read to the BB will be stopped in the device model, so
qemu will not have a chance of noticing there is a medium now (it only
has if the host floppy BDS is accessed).
Thus, using blk_is_inserted() without a medium at startup will not allow
the guest to ever read from the disk, unless you manually execute
blockdev-close-tray over QMP (I'm not even sure whether that works, but
I guess it might).
In the USB case, the problem was that a bdrv_*() function is called
(which already violates the rule that this is not for outside the block
layer) and a NULL check is needed there. In that case the assumption
that calling blk_bs() isn't allowed is wrong; it's simply necessary for
calling bdrv_*() functions.
In any case "I want to know A, but I'm not supposed to ask for A, so
I'll just check B" is not the way to go.
Except for when A and B are functionally equivalent.
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).
You called it "horror" above and now you want to rely on it?
I said: It's horror, but it "is actually helpful". And this is what I
meant by it.
The good thing is that I suspect that blk_bs() does _not_ work and
blk_is_inserted() is what you really needed in the first place.
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.
No, no, no, no, no.
Stop talking about implementation details, talk about concepts. The only
valid reason for calling blk_is_inserted() is that you want to know
whether a medium is inserted. If this is what you want to know, call it.
And if it isn't, be sure not to call it.
Maybe. I hate host passthrough, and floppy is the worst incarnation of
it. What does this have to do with anything? See my reasoning above why
blk_is_inserted() is actually sometimes not what we want to use.
Whether it has a NULL check here or a BDS tree there doesn't matter at
all. You can't just call a function because it happens to have the right
side effects if it was never meant for the purpose you're using it.
The problem is that blk_is_inserted() was meant for the purpose I'm
using it for, as you yourself said, but it just breaks with floppy
passthrough because we have to make the guest access the BDS tree even
if there is no medium there.
For our specific case: blk->bs == NULL implies an empty drive, but
that's just an implication, not an equivalence. The other direction
isn't true. That's why it's important to distinguish the cases.
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.
That's the job of the block layer, not of the floppy emulation.
So do you want to fix the floppy passthrough code? I don't even know how
to fix it. I don't even know what's wrong with it, other than that
something's ought to be wrong.
It's actually not much different from CD-ROM passthrough, where you do
have a tray on both the guest and the host. Loading the new medium and
closing the tray will have to be a single action there, because real
CD-ROMs just don't report whether there is a medium in their open tray.
Kevin
So, what I'm taking from this is the following: media_inserted is a kind
of strong attribute. If it is false, no accesses to the BB will be
generated whatsoever, and thus the host drive status will not be
updated. So we need to make sure it's only false if we really don't care
about the BB or the BDS tree; and that's only if there is no BDS tree at
all.
Because I understand your reasoning on "don't use B if A gives you what
you really want, and B just happens to behave the same here", I will be
using blk_bs() here, too, instead of blk_is_inserted(), unless you don't
find my reasoning correct, and although I'm still very much against
using blk_bs() somewhere like here. I will be able to live with it here,
just because it's FDD emulation and adding a drive status to it is kind
of kaput in itself.
Max