Re: [Qemu-devel] [PATCH v2 0/4] blockdev: Add blockdev-change-medium with read-only option

[Max Reitz <mreitz@redhat.com>]

On 2014-12-05 at 14:14, Eric Blake wrote:
> On 12/05/2014 03:08 AM, Max Reitz wrote:
>> The 'change' QMP and HMP command allows replacing the medium in drives
>> which support this, e.g. floppy disk drives. For some drives, the medium
>> carries information about whether it can be written to or not (again,
>> floppy drives). Therefore, it should be possible to change the read-only
>> state of block devices when changing the loaded medium.
>>
>> Following a suggestion from Eric, this series first introduces a
>> 'blockdev-change-medium' QMP command which is intended to replace the
>> 'change' command for block devices. Then, an optional additional
>> 'read-only' parameter is added which allows chaning the read-only state
> s/chaning/changing/ (I first read it as chaining) - of course, typos in
> cover letters don't really matter in the long run :)
>
>> in three ways:
>>
>> - 'retain': Just keep the status as it was before; this is the current
>>    behavior and thus this will be the default.
>> - 'ro': Force read-only access
>> - 'rw': Force writable access
>>
>> Finally, that 'read-only' parameter is added to the HMP 'change'
>> command. This series does not add a 'blockdev-change-medium' QMP command
> I assume you meant HMP in this line.

Yes, right.

>> because 'change' being overloaded for VNC and block devices is not too
>> bad for HMP (while it is for QMP).
> I agree with that approach.
>
>>
>> v2:
>> - basically completely rewritten
>> - Dropped 'auto' [Kevin and Markus]
>> - Introduced blockdev-change-medium [Eric]
>>
>> - Patch 1 introduces the new QMP command 'blockdev-change-medium'; there
>>    are (at least) two questionable design choices which I want to explain
>>    here:
>>    - The name is rather long; furthermore, the name 'change-blockdev' was
>>      already suggested by the existing code. I used such a long name
>>      because (1) there are no *-blockdev commands, but there are
>>      blockdev-* commands, so "blockdev" should be the prefix, not the
>>      suffix, and (2) "blockdev-change" could mean anything, so I wanted
>>      to be as clear as possible.
> That's actually a good explanation; I'm fine with the name you ended up
> with, even if it feels long.
>
>>    - The 'format' argument is optional; this is because by making it
>>      mandatory, it would have been difficult for the 'change' QMP and HMP
>>      commands to retain their 'format' argument optional as well (which
>>      we have to do thanks to compatibility)
> Yep, I can see that.  On the other hand, the other questionable feature
> of 'change' was that it required a filename, but then allowed "" as the
> filename that meant no new medium.

Hm, really?

HMP:

(qemu) change fdd ""
Could not open image: No such file or directory

QMP:

{'execute':'change','arguments':{'device':'fdd','target':''}}
{"timestamp": {"seconds": 1417785755, "microseconds": 519130}, "event": 
"DEVICE_TRAY_MOVED", "data": {"device": "fdd", "tray-open": true}}
{"error": {"class": "GenericError", "desc": "Could not open image: No 
such file or directory"}}

Technically, the medium is now empty, but that wasn't a successful 
execution.

> In patch 1/4, I wonder if we should
> make the new command a bit stricter, by having 'filename' be optional,
> and by forbidding "" as a filename (it's still a 1:1 mapping to the old
> semantics, and while it would require more HMP glue, it would feel a bit
> cleaner from the interface side).

If it was indeed possible somehow, I'd vote for disallowing empty file 
names for blockdev-change-medium and instead invoke 'eject' from the 
'change' commands (and point to 'eject' in the 'change' documentation).

Max