On 3 August 2011 15:28, Markus Armbruster <arm...@redhat.com> wrote: > andrzej zaborowski <balr...@gmail.com> writes: > >> On 3 August 2011 10:12, Markus Armbruster <arm...@redhat.com> wrote: >>> Peter Maydell <peter.mayd...@linaro.org> writes: >>> >>>> On 1 August 2011 13:33, Markus Armbruster <arm...@redhat.com> wrote: >>>>> andrzej zaborowski <balr...@gmail.com> writes: >>>>>> On 20 July 2011 18:24, Markus Armbruster <arm...@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.
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