qemu-devel
[Top][All Lists]
Advanced

[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: Kevin Wolf
Subject: Re: [Qemu-devel] [PATCH v2 03/37] hw/block/fdc: Implement tray status
Date: Thu, 5 Mar 2015 11:11:45 +0100
User-agent: Mutt/1.5.21 (2010-09-15)

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)". 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.)

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.

> 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?

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.

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.

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.

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]