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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2991 bytes --]

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>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]