| Message ID | 1453272694-17106-6-git-send-email-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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: 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 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. > > - 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. > > - 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>
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.
diff --git a/hw/block/fdc.c b/hw/block/fdc.c index e72a906..370ece1 100644 --- a/hw/block/fdc.c +++ b/hw/block/fdc.c @@ -273,7 +273,9 @@ static void pick_geometry(FDrive *drv) } if (match == -1) { if (first_match == -1) { - match = 1; + error_setg(&error_abort, "No candidate geometries present in table " + " for floppy drive type '%s'", + FloppyDriveType_lookup[drv->drive]); } else { match = first_match; }
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. This means: - 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 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 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. Signed-off-by: John Snow <jsnow@redhat.com> --- hw/block/fdc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)