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

[Peter Maydell]

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.

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

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.

thanks
-- PMM