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

[andrzej zaborowski]

On 3 August 2011 20:24, Markus Armbruster <address@hidden> wrote:
> andrzej zaborowski <address@hidden> writes:
>> On 3 August 2011 18:38, Markus Armbruster <address@hidden> wrote:
>>> andrzej zaborowski <address@hidden> writes:
>>>>                                                      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.
>>
>> Obviously I wasn't claiming it is, just that it might be useful, but
>> not necessrily possible.  After all pretty much any storage can be
>> "ejected" with enough force, depending on how far you want to go.
>>
>>>>> What's wrong with that again?  All sounds sensible to me.
>>>>
>>>> I'm not claiming otherwise, just double-checking this is what you want.
>>
>> So first you said you had a problem with _is_removable, and then you
>> said nothing was wrong with the implementation you outlined, plase
>> make up your mind.
>
> I don't appreciate you quoting me out of context like that.

I got quite annoyed when you started putting words in my mouth by
saying I said anything about CD-ROM.. the code in spitz/tosa is not
concerned with CD-ROMs even if downstream it boils down to that, it is
concerned with whether the device is removable or not, and that's what
the check does.  It doesn't help readability or anything by inlining
that check.  If you're trying to check for A then don't spell it out
as B, be explicit.  It's not a big deal but I just don't see the
point, sorry.

>
> The sentence you quoted was in the middle of my attempt to get you to
> explain what you're trying to accomplish there.

I already said about 3 times what it's trying to acomplish.  You also
have used the word "removable" so I'm sure you know what it means and
don't need further explanation.  But let's define it this way: if a
GUI is going to display an "eject" button next to a drive in the qemu
device tree, that's a removable device.  CD-ROM is an example of that.
 An IDE HDD is an example of something that's not going to have that
button (I assume).

> I started with
> analyzing what your code does.  You asked "what's wrong with that", and
> I replied that I'm not passing judgement on whether your code is right
> or wrong, I just want to know what you're trying to do.
>
> I'm getting tired of our misunderstandings, but let me try one more
> time, and tiresomely verbose:

Yes, same here.

>
> 1. bdrv_is_removable() is ill-defined junk, and needs to go away.

Then replace it with something else, I don't know.  Your original
argument was it was too convoluted.

>
>   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 ("created removable"),
>   except when an ide-hd, scsi-hd, scsi-generic or virtio-blk qdev is
>   attached.
>
>   That's not semantics, that's an embarrassment.

Certainly if it can be made simpler then by all means let's make it
simpler, but world is complicated like that.

>
> 2. I don't have an opinion on what should be done in
>   tosa_microdrive_attach() and spitz_microdrive_attach().  Heck, I
>   wouldn't touch it with a 9ft pole if I could help it.  But I have to,
>   because it uses bdrv_is_removable().
>
> 3. My first try was to preserve the current behavior.  You don't like
>   the result.  Fine.
>
> 4. For my second try, I tried to understand what you're trying to
>   accomplish there.  Turns out you want to check whether "the
>   underlaying storage can disappear for any other reason if that's
>   possible to check."  Thanks for explaining that.

No, I shouldn't have mentioned that, it's a lateral "would be nice to
have" issue and as I said it's likely not worth it.

>
> 5. Sorry, that's not what the code there does.
>
> 6. The block layer does not offer an API to check that.  You're quite
>   welcome to create it.

See above.

>
> 7. For what it's worth, IDE disks, SCSI disks and all the other
>   non-removable decive models other than microdrives happily work
>   without such a check.

Yes, removing the check would probably make more sense than you
current approach, but then I don't see a particular need for it
either.  (You can remove other checks like writing beyond the end of
device and stuff will keep working.)

And you have used the word "non-removable" so you must know what it
means, why then are you asking about the intention of that if ()?

Cheers