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 <js...@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 <ebl...@redhat.com> > Thanks, I'll revise the message and tentatively stick your R-B on it.
