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

[Max Reitz]

On 07.01.2016 22:42, Peter Maydell wrote:
> On 7 January 2016 at 20:14, Max Reitz <address@hidden> wrote:
>> On 07.01.2016 20:56, Peter Maydell wrote:
>>> It looks like sd.c is the only one which implements a change_media_cb
>>> but no is_tray_open, but it would be nice if we could implement this
>>> in the default blk_dev_is_tray_open() method rather than in the
>>> sd and floppy models (ie if I don't implement tray-open then the
>>> tray is closed if there's a medium attached, and the block backend
>>> ought to know if there's a medium attached itself already).
>>
>> That would be nice, but there's a difference between "there's a medium
>> attached" (tray can be both open and closed) and "the medium is
>> accessible by the guest" (tray must be closed). The BlockBackend does
>> not know this difference, only the guest devices does.
>>
>> It gets told of when to open/close the tray by invocation of the
>> change_media_cb() (the @load parameter set to false or true,
>> respectively), and we could track this state in the BlockBackend instead
>> of in the SDState. But that looks like the wrong place to me.
> 
> 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.

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

Max

signature.asc
Description: OpenPGP digital signature