qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Max Reitz <mreitz@redhat.com>
To: Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>,
	qemu-devel@nongnu.org, Stefan Hajnoczi <stefanha@redhat.com>,
	Luiz Capitulino <lcapitulino@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v2 0/4] blockdev: Add blockdev-change-medium with read-only option
Date: Fri, 05 Dec 2014 15:21:50 +0100	[thread overview]
Message-ID: <5481BF7E.50501@redhat.com> (raw)
In-Reply-To: <5481BC2C.2050808@redhat.com>

On 2014-12-05 at 15:07, Eric Blake wrote:
> On 12/05/2014 06:47 AM, Max Reitz wrote:
>>> I'd like to make sure that new commands to control removable media get
>>> us closer to a sane set of such commands.  Let's consider states and
>>> transitions.
>>>
>>> If we ignore the tray lock for a moment, we have:
>>>
>>>                                       load
>>>                       tray open  --------------->  tray closed
>>>                         empty    <---------------    empty
>>>                         ^   |        eject
>>>                         |   |
>>>           remove medium |   | insert medium
>>>                         |   |
>>>                         |   v        load
>>>                       tray open  --------------->  tray closed
>>>                          full    <---------------    full
>>>                                       eject
> By name, this command feels like it is just the remove/insert medium
> step, and unrelated to tray open/close steps.
>
>>> Both the operator and the guest OS can load / eject.
>>>
>>> Only the operator can remove / insert medium.
> Where change is a special case of remove and insert done together.
>
>>> A tray lock complicates things a bit.  Each state above splits into a
>>> locked and unlocked state, with the obvious lock / unlock state
>>> transitions.  Only the guest OS can lock / unlock.
>>>
>>> When the tray is locked and closed, operator eject merely notifies the
>>> guest OS (blk_dev_eject_request(blk, false)).
>>>
>>> In states tray closed / locked, there's an additional operation "eject
>>> forcefully".  It notifies the guest OS (blk_dev_eject_request(blk,
>>> true)), and opens the tray.  Whether unlocks it depends on the device.
>>>
>>> Like change, blockdev-change-medium conflates several basic operations.
>>> Is that what we want, or should we create something that lets us do
>>> basic operations?
> change lacks a force parameter, which means that it feels like something
> that can only be attempted when the tray is open.  The fact that we
> (can) make it auto-open an (unlocked) tray under the hood may be okay
> for HMP, but should not be part of the QMP command.
>
>> Good question. I don't think it will be bad in practice, though. If you
>> split up blockdev-change-medium or really only change the medium without
>> loading it, then the only advantage that you get is that you can
>> exchange media without loading them; I can't really think of a use case
>> for that, so in reality you'll always have blockdev-change-medium
>> followed immediately by blockdev-load-medium or blockdev-close-tray or
>> whatever.
>>
>> You could split it up even more of course, then you'd have the following
>> order for loading a medium:
>> (1) 'blockdev-open-tray', if not yet open
>> (2) 'blockdev-remove-medium', if not yet empty
>> (3) 'blockdev-insert-medium'
> I'm not sure if 2 and 3 need to be separate, or if they can be a single
> change-medium command.

Rather not. I want to be able to remove a medium without having to 
insert a new one (you can make the new medium optional for 
change-medium, but I think it's better to just separate the commands).

>> (4) 'blockdev-close-tray
> But yes, I think we need separate QMP commands for tray movement as
> compared to medium changes, and that medium change should fail if the
> tray is closed.  Higher level software can string together multiple QMP
> commands to give the user a single 'change' experience, as desired,

We'll have to keep 'change' anyway, and I wouldn't want to remove it for 
HMP.

> but
> I don't think QMP should provide such a command,

Well, it will, because we will have to keep 'change'.

> as it is harder to
> determine what state things are left in if it fails halfway through.
>
>> I can't think of any time when you'd want to call insert-medium without
>> close-tray or without having called remove-medium before (well, if it's
>> empty, you don't have to, but well...).
>>
>> And 'eject' does blockdev-open-tray plus blockdev-remove-medium, so...
>>
>> Or better, let's collect use cases:
> Use cases are the HMP (or libvirt) side of things.  If the use case can
> be accomplished by stringing together multiple well-defined QMP
> operations, instead of having an overloaded QMP command that does
> multiple steps, that's still okay for the user's point of view .

Right. The question for me was whether there's a practical benefit. If 
you can split an operation into four fundamental steps but nobody would 
ever execute anything different than those four steps in the exactly 
same order, there is no real reason to split.

(Disclaimer: That was just an example which doesn't have to apply here ;-))

>> (1) Insert medium into empty drive and load it: Works with
>> 'blockdev-change-medium'
> Umm, I'm actually agreeing with Markus that we probably want to separate
> changing the medium from closing the tray; while for floppies, there is
> no tray, so changing a medium is all the more you need to do.

For me, floppy trays work like this:
- tray closed, no medium: Doesn't exist. qemu might have that state 
internally, but to the guest it should always look like the tray is open.
- tray open, no medium: Drive is empty.
- tray closed, with medium: Medium is inserted.
- tray open, with medium: Medium was just pushed out or is ready to be 
pushed in (so there is a medium in the slot, but it isn't pushed in). To 
the guest, there is no difference to the "tray open, no medium" state.

So we can indeed just go with the standard four states, only that the 
guest sees only two of them (but that's the problem of qemu's floppy 
emulation).

For the record, I'm looking forward when we're trying to explain floppy 
handling to developers even younger than me in a couple of years. :-)

>> (2) Open drive, remove medium: Works with 'eject'
> What about 'open drive, but leave medium in place for next time drive is
> closed'.  Does 'eject' do that (possibly via the addition of an optional
> boolean that says whether to leave the medium intact)?

Urgh, I'd avoid that. If that's the alternative you're threatening me 
with, I will split up 'eject', fine. :-)

> Also, floppy
> drives can be locked, but have no tray; do we have the right semantics
> for requesting an eject but waiting for the guest to comply by unlocking
> the drive?

I don't know and I'm not sure whether I want to take a look into the 
floppy emulation code. *g*

>> (3) Open drive, remove medium, insert medium, close drive: Works with
>> 'blockdev-change-medium'
> Or rather, works with the high-level HMP 'change', and may be
> implemented via multiple QMP commands under the hood.
>
>> (4) Open drive, remove medium, close drive: Does not work with only
>> 'eject' and 'blockdev-change-medium', but I can't see a difference
>> between an open drive and a closed empty drive
> For floppies, there isn't a difference (no tray, so no medium is all the
> more distinction you get).  But for cdroms, the guest knows if the tray
> is still open or closed

I know, but does the guest care? Or, to be more exact, should it care so 
much that the user actually wants to be able to enforce and empty closed 
drive?

> (it doesn't know if the medium is present when
> the tray is open; the fact that qemu still tracks medium for an open
> tray is for convenience in closing the tray and still having the medium
> ready to use if it wasn't changed while the tray was open).

I wouldn't just call it convenience. qemu is a system emulator, so it 
should emulate a system both to the guest and the user. The user may 
expect to be able to have a medium in an open drive, so that's what qemu 
should support, too.

>> (5) Open drive, repeatedly change medium, close drive: Does not work
>> with 'blockdev-change-medium' because the guest will see all the media
>> you cycled through; but I don't consider this an important use case
> May not be important, and may not be something we expose through HMP,
> but I agree with Markus that it would be nice to let the QMP side allow
> this.
>
>> So, of course it may be nice in principle to have broken it down to the
>> fundamental operations, but I don't see the practical implication.
>>
>> Hm, well, there is one. I remember someone complaining that 'eject'
>> sometimes removes the medium and sometimes doesn't. It did remove the
>> medium when qemu could immediately eject it; but it didn't if the drive
>> was locked, the guest was notified and then the guest opened the tray.
>> So that is a practical implication, because after calling
>> blockdev-open-tray, you'd be sure that the medium is still inserted.
>>
>> I personally don't have a strong opinion. Introducing more commands
>> would be work, but I guess I would have time for that now.
> Requiring multiple QMP commands to do a high-level HMP operation may
> feel like overkill, but in the long run the finer granularity will be
> worth it.  I think that limiting 'blockdev-change-medium' to work only
> on unlocked floppy drives and only on open-tray cdrom drives, plus the
> additional glue to open/close the cdrom tray as necessary, will be worth
> the effort.

At the least it'll be a good example to other QMP commands. *g*

Okay, will rewrite again. v3 might therefore take a bit.

Max

      reply	other threads:[~2014-12-05 14:22 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-12-05 10:08 [Qemu-devel] [PATCH v2 0/4] blockdev: Add blockdev-change-medium with read-only option Max Reitz
2014-12-05 10:08 ` [Qemu-devel] [PATCH v2 1/4] qmp: Introduce blockdev-change-medium Max Reitz
2014-12-05 13:10   ` Eric Blake
2014-12-05 13:18     ` Max Reitz
2014-12-05 13:25       ` Eric Blake
2014-12-05 10:08 ` [Qemu-devel] [PATCH v2 2/4] hmp: Use blockdev-change-medium for change command Max Reitz
2014-12-05 13:24   ` Eric Blake
2014-12-05 13:27     ` Max Reitz
2014-12-05 13:45       ` Eric Blake
2014-12-05 10:08 ` [Qemu-devel] [PATCH v2 3/4] blockdev: Add read-only option to blockdev-change-medium Max Reitz
2014-12-05 13:28   ` Eric Blake
2014-12-05 10:08 ` [Qemu-devel] [PATCH v2 4/4] hmp: Add read-only option to change command Max Reitz
2014-12-05 13:49   ` Eric Blake
2014-12-05 13:14 ` [Qemu-devel] [PATCH v2 0/4] blockdev: Add blockdev-change-medium with read-only option Eric Blake
2014-12-05 13:25   ` Max Reitz
2014-12-05 13:31 ` Markus Armbruster
2014-12-05 13:47   ` Max Reitz
2014-12-05 14:07     ` Eric Blake
2014-12-05 14:21       ` Max Reitz [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5481BF7E.50501@redhat.com \
    --to=mreitz@redhat.com \
    --cc=armbru@redhat.com \
    --cc=eblake@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=lcapitulino@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=stefanha@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).