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

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

> On 1 August 2011 13:33, Markus Armbruster <address@hidden> wrote:
>> andrzej zaborowski <address@hidden> writes:
>>> On 20 July 2011 18:24, Markus Armbruster <address@hidden> wrote:
>>>> We try the drive defined with -drive if=ide,index=0 (or equivalent
>>>> sugar).  We use it only if (dinfo && bdrv_is_inserted(dinfo->bdrv) &&
>>>> !bdrv_is_removable(dinfo->bdrv)).  This is a convoluted way to test
>>>> for "drive media can't be removed".
>>>>
>>>> The only way to create such a drive with -drive if=ide is media=cdrom.
>>>> And that sets dinfo->media_cd, so just test that.
>>>
>>> This is a less generic test and more prone to be broken inadvertently,
>>> so it seems like a step back.  What's the argument against the
>>> convoluted and explicit test?
>>
>> My motivation for changing it was to reduce the uses of BlockDriverState
>> member removable prior to nuking it from orbit [PATCH 45/55].
>>
>> I consider my change an improvement, because I find "dinfo->media_cd"
>> clearer than
>> "bdrv_is_inserted(dinfo->bdrv) && !bdrv_is_removable(dinfo->bdrv)".
>
> This seems like an argument for providing a bdrv_supports_eject()
> or whatever we're actually trying to test for here. I kind of felt
> the same way as Andrzej when I saw this patch going past but it
> got lost in my mail folder...

Well, what are you trying to test for here?

Let's start with figuring out what we actually test for right now (may
not be what you *want* to test for, but it's a start).  The test code is
"bdrv_is_inserted(bs) && !bdrv_is_removable(bs)".

bdrv_is_removable() is a confused mess.  It is true when an ide-cd,
scsi-cd or floppy qdev is attached, or when the BlockDriverState was
created with -drive if={floppy,sd} or -drive
if={ide,scsi,xen,none},media=cdrom, except when an ide-hd, scsi-hd,
scsi-generic or virtio-blk qdev is attached.

Since we're about to attach a device here, no other device can be
attached, and the mess simplifies into "when the BlockDriverState was
created with -drive if={floppy,sd} or -drive
if={ide,scsi,xen,none},media=cdrom".

Since we're getting IF_IDE, it further simplifies into "when the
BlockDriverState was created with -drive if=ide,media=cdrom".  Which is
what my patch tests.

The bdrv_is_inserted() part is actually redundant, because it can only
be false if bdrv_is_removable() is true: the only way to create an IDE
drive empty is with media=cdrom (makes bdrv_is_removable() true), and
media is ejectable only when bdrv_is_removable() is true.

Therefore, my patch preserves behavior.


Testing for "block driver supports eject" is something else entirely.
Block drivers "host_cdrom" and "host_floppy" implement method
bdrv_eject()[*].  Is that what you want?

Note that "block driver supports eject" is completely independent of
"monitor command eject works".  You can back a removable device such as
a CD-ROM with a block driver that doesn't support eject, say "file".
Monitor command eject works fine then.  Conversely, you can back a
non-removable device such as a disk with a block driver that does
support eject, such as "host_cdrom".  It may not be the wisest thing to
do, but it works.  Ejecting the physical media while the device model is
using it will result in I/O errors, to nobody's surprise.


Anyway.  bdrv_is_removable() must die.  You're using it.  Please tell me
what exactly you're trying to accomplish, so I can get it done without
bdrv_is_removable().  Best way to tell me is with a patch.


[*] "raw" implements bdrv_eject() as well, but it merely forwards, which
doesn't count.