qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: andrzej zaborowski
Subject: Re: [Qemu-devel] [PATCH 44/55] spitz tosa: Simplify "drive is suitable for microdrive" test
Date: Wed, 3 Aug 2011 14:05:25 +0200

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.

> 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.

>
> 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?

No, from your description of ejectable it looks like we want to check
if the device supports the monitor command "eject", although
additionally testing for "block driver supports eject" may make sense.

Cheers



reply via email to

[Prev in Thread] Current Thread [Next in Thread]