[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v4 05/12] fdc: Throw an assertion o
Re: [Qemu-block] [Qemu-devel] [PATCH v4 05/12] fdc: Throw an assertion on misconfigured fd_formats table
Wed, 20 Jan 2016 16:33:04 -0500
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0
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, 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, 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
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, 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 <address@hidden>
>> 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 <address@hidden>
Thanks, I'll revise the message and tentatively stick your R-B on it.
- [Qemu-block] [PATCH v4 00/12] fdc: fix 2.88mb floppy diskette support, John Snow, 2016/01/20
- [Qemu-block] [PATCH v4 01/12] fdc: move pick_geometry, John Snow, 2016/01/20
- [Qemu-block] [PATCH v4 02/12] fdc: reduce number of pick_geometry arguments, John Snow, 2016/01/20
- [Qemu-block] [PATCH v4 05/12] fdc: Throw an assertion on misconfigured fd_formats table, John Snow, 2016/01/20
- [Qemu-block] [PATCH v4 03/12] fdc: add drive type qapi enum, John Snow, 2016/01/20
- [Qemu-block] [PATCH v4 04/12] fdc: add disk field, John Snow, 2016/01/20
- [Qemu-block] [PATCH v4 07/12] fdc: Add fallback option, John Snow, 2016/01/20
- [Qemu-block] [PATCH v4 09/12] fdc: add physical disk sizes, John Snow, 2016/01/20