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

[Max Reitz]

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