Re: [Qemu-block] [Qemu-devel] [PULL v2 10/40] blockdev: Implement change with basic operations

[Max Reitz]

On 07.01.2016 23:19, Peter Maydell wrote:
> On 7 January 2016 at 21:57, Max Reitz <address@hidden> wrote:
>> On 07.01.2016 22:42, Peter Maydell wrote:
>>> Well, previously sd.c didn't need to have any state for this
>>> to all work right (or indeed care about implementing a fake
>>> tray status for a device that doesn't have a tray), so it seems
>>> odd that we need to invent some extra state for it to work now.
>>
>> Before, it took the state from the block layer by calling
>> blk_is_inserted(). Works fine as long we only have the high-level
>> operations change and eject. Stops to work once we introduce lower-level
>> operations (open-tray, remove-medium, insert-medium, close-tray).
>>
>> Why do we need the low-level operations? Mainly because they integrate
>> much better into the model of a more freely configurable block layer
>> (there aren't many options one can give to the 'change' operation; now
>> you can use blockdev-add and the lower-level operations afterwards).
>>
>> Why did I reimplement 'change' and 'eject' using these new operations?
>> Because otherwise we'd have those operations doing one kind of thing,
>> and the lower-level ones doing something completely different. I'd find
>> that bad.
> 
> But these lower level operations now let you put the system
> conceptually into states that it really can't be in in hardware.
> That seems wrong to me and we should fail those commands where
> it makes sense (eg trying to "open a tray" when there is no tray),
> rather than forcing the sd.c layer code to cope with the user
> putting the system into impossible conditions.

Hm, OK. So I assume you are proposing just not doing any *-tray
operations on devices without a tray? I personally still consider
"pushing the eject button" and "actually taking the medium out of the
slot" to distinct operations, but I think I could live with that.

Technically, the block layer should handle that just fine.

>>>> Right now, sd.c completely ignores the @load parameter of
>>>> change_media_cb(), which seems wrong; this means that "opening the tray"
>>>> keeps the medium accessible for the guest. In the past that was fine,
>>>> because eject closed the associated BDS (the medium) right afterwards,
>>>> so it actually wasn't accessible any more. The new
>>>> blockdev-open-tray no longer does that, so now we need to respect the
>>>> value of @load and no longer rely on blk_is_inserted() alone any more.
>>>
>>> I think blockdev-open-tray should probably not work for SD cards
>>> (or for floppies?), because saying "open the tray" on a device
>>> without a tray is meaningless.
>>
>> Depends on what you think it is. For me, blockdev-open-tray generally
>> means "pushing the eject button".
>>
>> For actual tray devices, this means that the tray may open (or the OS
>> may be asked to do so). For floppy disk drives, this means the floppy
>> disk will be ejected. For SD slots (where there often is no dedicated
>> button, but one can push the SD card further in for it to get released),
>> this means the SD card will be ejected.
>>
>> Note that this is different from the 'eject' command which models a
>> drive where the user pushes the eject button and suddenly the medium
>> bursts into flames/is dropped on the floor/just disappears. Therefore, I
>> find the 'eject' command quite misnamed, but having named the new
>> command 'blockdev-eject-medium' wouldn't have made things any better.
> 
> I don't understand what the difference is between your
> "floppy disk will be ejected" and "SD card will be ejected"
> cases, and "medium is dropped on the floor" case. Those seem
> like exactly the same thing to me, so we should just use "eject"
> and not have an extra command.

From the guest's perspective, you are right, there is no difference. I
do see a difference from the user's perspective, but I think you are
right in that that difference may not actually matter to anyone.

The difference is the following (let's use a floppy disk drive, because
there are all kinds of SD slots in common use): On a physical device,
you push the eject button and the disk comes out. You can then just push
the disk back in, or you can remove it, and maybe replace it.

Using 'eject', you cannot push the disk back in. It's gone. You'll have
to find a new one to insert. Yes, this is a use case that rarely anybody
ever needs, but it still is a difference from what physical hardware does.

Using 'blockdev-open-tray' means pushing the eject button. You can use
'blockdev-close-tray' to push the medium back in. Or you invoke
'(x-)blockdev-remove-medium' to remove it and
'(x-)blockdev-insert-medium' to insert a new one (which you'd then push
in using 'blockdev-close-tray').

Technically, what happens is this: The *-tray commands are for the
device model. blockdev-close-tray invokes the change_media_cb with
load=false; blockdev-open-tray invokes it with load=true. The general
block layer doesn't do anything here.

The *-medium commands are for the block layer. blockdev-remove-medium
removes BlockDriverState from the BlockBackend attached to the device,
and blockdev-insert-medium attaches a BDS to the BB. The device model
does not get notified of this, because it should not care at this point
(the tray, be it actually existing or just some boolean in its state, is
open, so the virtual device cannot see the medium anyway).

> I can see why we want to model tray state for devices with trays,
> but I don't see why this is bleeding into devices without trays.

I hope that the above explanation helped you understand why it bled into
tray-less devices, from a technical perspective.

Anyway, from a usability standpoint, you are right in that probably
nobody ever needs the *-tray commands for tray-less devices.

I don't think however that having these commands will hurt anyone, so I
think (please correct me if I'm wrong (not just here, of course :-))
that this is mainly a technical discussion of how ugly having these
commands for tray-less devices is, and of how difficult it is to
implement it reasonably.

Without migration, I would have said that I do not find a simple boolean
in the device state struct to be too bad. However, you are correct in
that this field needs to be migrated, so now it becomes a different story.

The alternative, which I'll be looking into next week and which I guess
is what you'd find more appealing, is to drop the *-tray commands for
tray-less devices by design. The problem here is that then the
blockdev-*-medium devices actually need to notify tray-less devices
(invoke their change_medium_cb), which they do not for devices that do
have a tray. While that's probably not difficult to implement, it is a
bit ugly. I'll look into it.

Thank you for this discussion!

Max

signature.asc
Description: OpenPGP digital signature