Re: [Qemu-devel] [PATCH v4 05/12] fdc: Throw an assertion on misconfigured fd_formats table

[John Snow <jsnow@redhat.com>]

On 01/20/2016 04:23 PM, Eric Blake wrote:
> On 01/19/2016 11:51 PM, John Snow wrote:
>> pick_geometry is a convoluted function that makes it difficult to tell
>> at a glance what QEMU's current behavior for choosing a floppy drive
>> type is when it can't quite identify the diskette.
>>
>> If drive type is NONE, it considers all drive types in the candidate
>> geometry table to be a match, and saves the first such one as
>> "first_match." If drive_type is not NONE, first_match is set to the first
>> candidate geometry in the table that matched our specific drive type.
> 
> That seems subtly different than how I read the code (it is possible to
> exit the for loop with match == 0 but first_match == -1, if nb_sectors
> is right for the very first entry; but your statement implies that
> "first_match" will always be non-negative after the loop). Maybe a
> better wording would be:
> 

Yeah, I was oversimplifying in retrospect. In any case where we bother
to read first_match, it must always be set. We don't bother when we get
a real, exact match.

> The code starts iterating over all entries in the table, and if our
> specific drive type matches a row in the table, then either "match" is
> set to that entry (an exact match) and the loop exits, or "first_match"
> will be non-negative (the first such entry that shares the same drive
> type), and the loop continues.  If our specific drive type is NONE, then
> all drive types in the candidate geometry table are considered.  After
> iteration, if "match" was not set, we fall back to "first_match".
> 

This is literally the worst function in QEMU. It is so wrong, describing
why it is wrong is itself difficult.

>>
>> This means:
> 
> This means that either "match" was set, or we exited the loop without an
> exact match, in which case:
> 
>>
>> - If drive type is NONE, the default is truly fd_formats[0], a 1.44MB
>>   type, because first_match will always get set to the first item.
> 
> - If drive type is NONE, the default is truly fd_formats[0], a 1.44MB
> type, because "first_match" will always get set to the first item.
> 

Just adding quotes, OK.

>>
>> - If drive type is not NONE, pick_geometry gets fussier and attempts to
>>   only pick a geometry if it matches our drive type. In this case,
>>   first_match must always be set because all known drive types have
>>   candidate geometries listed in the table.
> 
> - If drive type is not NONE, pick_geometry() was fussier and only looked
> at rows that match our drive type.  However, since all possible drive
> types are represented in the table, we still know that "first_match" was
> set.
> 

gets, was, is. I can use your wording, anyway.

>>
>> - If drive type is not NONE and the fd_formats table lists no options for
>>   our drive type, we choose fd_formats[1], an incomprehensibly bizarre
>>   choice that can never happen anyway.
>>
>>
>> Correct this: If first_match is -1, it can ONLY mean we didn't edit our
>> fd_formats table correctly. Throw an assertion instead.
> 
> But this part is right.
> 
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  hw/block/fdc.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> The code change itself is correct, so with an improved commit message,
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Thanks, I'll revise the message and tentatively stick your R-B on it.