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

[andrzej zaborowski]

On 3 August 2011 15:28, Markus Armbruster <address@hidden> wrote:
> andrzej zaborowski <address@hidden> writes:
>
>> On 3 August 2011 10:12, Markus Armbruster <address@hidden> wrote:
>>> 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".
>>
>> 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.

I didn't say that, I want to check 1. if the underlaying device is
shown as removable (support monitor eject, I guess), 2. if the
underlaying storage can disappear for any other reason if that's
possible to check.

And I said that it looks like the inner implementation of the
_is_removable function is probably correct, but inlining it makes no
sense to me.  And the implementation shouldn't even bother us here.
Reread the part you quoted.

Cheers