Re: [Qemu-devel] [PATCH 44/55] spitz tosa: Simplify "drive is suitable for microdrive" test

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 09.08.2011 06:32, schrieb andrzej zaborowski:
>> On 4 August 2011 10:02, Kevin Wolf <kwolf@redhat.com> wrote:
>>> Am 03.08.2011 22:20, schrieb andrzej zaborowski:
>>>> On 3 August 2011 20:24, Markus Armbruster <armbru@redhat.com> wrote:
>>>>> andrzej zaborowski <balrogg@gmail.com> writes:
>>>>>> On 3 August 2011 18:38, Markus Armbruster <armbru@redhat.com> wrote:
>>>>>>> andrzej zaborowski <balrogg@gmail.com> writes:
>>>>>>>>                                                      2. if the
>>>>>>>> underlaying storage can disappear for any other reason if that's
>>>>>>>> possible to check.
>>>>>>>
>>>>>>> bdrv_is_removable() *isn't* such a check.
>>>>>>
>>>>>> Obviously I wasn't claiming it is, just that it might be useful, but
>>>>>> not necessrily possible.  After all pretty much any storage can be
>>>>>> "ejected" with enough force, depending on how far you want to go.
>>>>>>
>>>>>>>>> What's wrong with that again?  All sounds sensible to me.
>>>>>>>>
>>>>>>>> I'm not claiming otherwise, just double-checking this is what you want.
>>>>>>
>>>>>> So first you said you had a problem with _is_removable, and then you
>>>>>> said nothing was wrong with the implementation you outlined, plase
>>>>>> make up your mind.
>>>>>
>>>>> I don't appreciate you quoting me out of context like that.
>>>>
>>>> I got quite annoyed when you started putting words in my mouth by
>>>> saying I said anything about CD-ROM.. the code in spitz/tosa is not
>>>> concerned with CD-ROMs even if downstream it boils down to that, it is
>>>> concerned with whether the device is removable or not, and that's what
>>>> the check does.  It doesn't help readability or anything by inlining
>>>> that check.  If you're trying to check for A then don't spell it out
>>>> as B, be explicit.  It's not a big deal but I just don't see the
>>>> point, sorry.
>>>>
>>>>>
>>>>> The sentence you quoted was in the middle of my attempt to get you to
>>>>> explain what you're trying to accomplish there.
>>>>
>>>> I already said about 3 times what it's trying to acomplish.  You also
>>>> have used the word "removable" so I'm sure you know what it means and
>>>> don't need further explanation.  But let's define it this way: if a
>>>> GUI is going to display an "eject" button next to a drive in the qemu
>>>> device tree, that's a removable device.  CD-ROM is an example of that.
>>>>  An IDE HDD is an example of something that's not going to have that
>>>> button (I assume).
>>>
>>> But this is a property of the device, not of the backend. This means
>>> that it belongs in the device emulation and not in block.c.
>> 
>> By device do you mean hw/ide/microdrive.c?
>
> I mean just anything in hw/. I'm not familiar with how these devices
> work internally, so if hw/ide/microdrive.c is the right place, fine.

We have way too many things we call "device", or "driver"...  Let me try
to dispell the confusion.

Machines spitz, borzoi, terrier (hw/spitz.c) have a spitz microdrive
device.  Machine tosa (hw/tosa.c) has a tosa microdrive device.

Like all block devices, they consist of a frontend and a backend.

Our block device backend abstraction is BlockDriverState.  Internally, a
block device backend is the block layer sitting on top of an optional
BlockDriver.  No block driver is interpreted as no medium.

Monitor command "eject" drops the block driver.

Monitor command "change" first drops the block driver, then attaches a
new one.

A BlockDriver may support removable media.  Host device block drivers
do, the others don't.  Doesn't affect monitor commands eject and change
at all, because these *drop* the block driver.

A block device frontend is attached to a backend on initialization.  In
other words, a backend may exist alone, but a frontend is always
attached to a backend.

A block device frontend may support removable media.  If it does,
monitor commands eject and change work while the frontend is attached.
They always work while no frontend is attached.

Note: we got *two* different "removables" here, frontend and backend.
It is possible to back a removable frontend, say ide-cd, with a
non-removable backend, say file.  The opposite is also possible (whether
it's wise is a different question).

The frontend code is in hw/ide/microdrive.c.  It does *not* support
removable media.  Monitor commands eject and change refuse to touch this
device.

The property "monitor command eject is applicable to the device" is a
property of the frontend, because the frontend and only the frontend
decide whether eject is okay.

The property 'GUI shall display an "eject" button next to a drive in the
qemu device tree' is just the same.

Kevin wrote "this is a property of the device, not of the backend.  This
means that it belongs in the device emulation and not in block.c."
Kevin is correct.  The code is also correct: microdrive.c defines a
non-removable device (by not implementing callback change_media_cb()).
Therefore, monitor commands eject and change will not touch microdrives.

The checks in spitz_microdrive_attach() and tosa_microdrive_attach() do
*not* contribute to that.  All they accomplish is silently refrain from
creating the device if the user created the first IDE drive
(if=ide,index=0) with media=cd.  No more, no less.

I have no opinion on whether that's useful.

If you want to keep it, take my PATCH 44/55 as is.

If you want to create the drive regardless of media=cd, tell me, and
I'll do exactly that in my respin.

>> I'm not saying it belongs
>> in block.c, but logically it belongs in the same place as
>> bdrv_is_inserted, bdrv_is_locked, bdrv_eject etc. no?  So it is a
>> property of whatever "media" is property of.
>
> Depends. As long as you're talking about purely virtual device state, I
> would say yes, they belong there, too. However, we have things like
> CD-ROM passthrough where bdrv_is_inserted etc. are supposed to return
> the status of the physical host device. These are host state and need to
> be in the block layer.

bdrv_is_inserted() is about the backend's media.  At least it is after
my PATCH 21/55 cleans up the mess I made in commit 4be9762a.

bdrv_eject() is a function frontends supporting removable media call to
notify backends of medium eject/load.  If the backend happens to use a
host device block driver, it'll eject/load the physical medium.

Likewise, bdrv_set_locked() is a function frontends supporting removable
media call to notify backends of medium lock/unlock.  If the backend
happens to use a host device block driver, it'll lock/unlock the
physical medium.

bdrv_is_locked() is about the frontend's media lock.  To make this more
obvious, my PATCH 29/55 replaces it by bdrv_dev_is_medium_locked().  It
does *not* query the backend's lock (which may not even exist!) set by
bdrv_set_locked().