[Qemu-devel] Re: [PATCH 03/11] raw-posix: Don't "try harder" for BDRV_TYPE_CDROM

[Kevin Wolf]

Am 05.07.2010 18:15, schrieb Markus Armbruster:
> Kevin Wolf <address@hidden> writes:
> 
>> Am 30.06.2010 13:55, schrieb Markus Armbruster:
>>> raw_pread_aligned() retries up to two times if the block device backs
>>> a virtual CD-ROM.  This makes no sense.  Whether retrying reads can
>>> correct read errors may depend on what we're reading, not on how the
>>> result gets used.
>>>
>>> Also clean up gratuitous use of goto.
>>>
>>> This reverts what's left of commit 8c05dbf9.
>>>
>>> Signed-off-by: Markus Armbruster <address@hidden>
>>
>> Are you sure that this won't cause a regression? I mean if there is a
>> patch specifically adding this behaviour, there probably was a problem
>> that made someone touch the code in the first place.
>>
>> Arguably checking for the type hint is nonsense, however I think the
>> case for which this was written is passing through a real CD-ROM to a VM
>> - in which case the condition would be true anyway.
>>
>> So instead of removing the code, the fix to achieve what was probably
>> intended is to check for bs->drv == &bdrv_host_cdrom.
> 
> I can do that.  But does it make sense?  How can retrying failed reads
> help?  Isn't the OS in a much better position to retry?
> 
> Keeping the retry code feels like voodoo-programming to me: I have no
> idea how waving around this dead chicken could help, but we've always
> done it, so keep waving ;)

I would agree that someone tried to be clever without real reason if
this was buried in one of those big Fabrice-style commits. But is was
added as a commit for itself, and I'd be surprised if someone sent a
patch if it didn't change anything for him.

Let's try if I've got a valid email address of Ben for CCing him... Ben,
do you remember this patch and can you help us?

commit 8c05dbf9b68cc8444573116063582e01a0442b0b
Author: ths <address@hidden>
Date:   Thu Sep 13 12:29:23 2007 +0000

    Enhance raw io reliability, by Ben Guthro.

Kevin