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: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH 44/55] spitz tosa: Simplify "drive is suitable for microdrive" test
Date: Wed, 03 Aug 2011 18:38:47 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux)

andrzej zaborowski <address@hidden> writes:

> 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),

After PATCH 45 all of them do, without exception.

"eject" works unless a device model is attached that doesn't want its
medium removed.

Before PATCH 45, "eject" sometimes refuses to work when no device model
is attached (arbitrary, but harmless), and sometimes worked even when
the attached device model doesn't want its medium removed (bug).  See
the patch description for details.

>                                                      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.  It happily returns false for
-device if=ide,file=/dev/sr0.  tosa and spitz will happily use it, as
long as there's a CD in the drive during startup.

The block layer currently doesn't provide such a check.  For what it's
worth, IDE disks, SCSI disks and all the other non-removable decive
models other than microdrives happily work without it.

If you want such a check, what about a patch?

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



reply via email to

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