Re: [Qemu-devel] [PATCH v2 03/37] hw/block/fdc: Implement tray status

[Max Reitz]

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.

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