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

[Markus Armbruster <armbru@redhat.com>]

andrzej zaborowski <balrogg@gmail.com> writes:

> On 3 August 2011 10:12, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On 1 August 2011 13:33, Markus Armbruster <armbru@redhat.com> wrote:
>>>> andrzej zaborowski <balrogg@gmail.com> writes:
>>>>> On 20 July 2011 18:24, Markus Armbruster <armbru@redhat.com> 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".
>
> What's wrong with that again?  All sounds sensible to me.

I'm not claiming otherwise, just double-checking this is what you want.

>> Which is
>> what my patch tests.
>
> It's like if you changed the named constants/#defines in a project for
> their preprocessed values... Behaviour is preserved but it makes worse
> code, and its easier to break too, when someone updates the value of
> some constant.

Well, you just said you want to check whether the drive was created as
CD-ROM.  The most direct way to check precisely that is drive->media_cd,
and I can't see how that could break.

If you can see how, please tell me.

If you want to check for something else, please explain what you want.

"Keep it the way it was" isn't an explanation.  And keeping
bdrv_is_removable() mess alive just for this one use would be silly.

[...]