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

[Max Reitz <mreitz@redhat.com>]

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 <mreitz@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.
> 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