Re: [Qemu-block] [Qemu-devel] [PATCH v4 10/12] fdc: rework pick_geometry

[Eric Blake]

On 01/19/2016 11:51 PM, John Snow wrote:
> This one is the crazy one.
> 
> fd_revalidate currently uses pick_geometry to tell if the diskette
> geometry has changed upon an eject/insert event, but it won't allow us
> to insert a 1.44MB diskette into a 2.88MB drive. This is inflexible.
> 
> The new algorithm applies a new heuristic to guessing disk geometries
> that allows us to switch diskette types as long as the physical size
> matches before falling back to the old heuristic.
> 
> The old one is roughly:
>  - If the size (sectors) and type matches, choose it.
>  - Fall back to the first geometry that matched our type.
> 
> The new one is:
>  - If the size (sectors) and type matches, choose it.
>  - If the size (sectors) and physical size match, choose it.
>  - If the size (sectors) matches at all, choose it begrudgingly.

This is the one I worry about; details below.

>  - Fall back to the first geometry that matched our type.
> 
> Signed-off-by: John Snow <address@hidden>
> ---

> -    int i, first_match, match;
> +    int i;
> +    int match, size_match, type_match;
> +    bool magic = drv->drive == FLOPPY_DRIVE_TYPE_AUTO;
>  
>      /* We can only pick a geometry if we have a diskette. */
>      if ((!drv->media_inserted) || (drv->drive == FLOPPY_DRIVE_TYPE_NONE)) {
>          return -1;
>      }
>  
> +    /* We need to determine the likely geometry of the inserted medium.
> +     * In order of preference, we look for:
> +     * (1) The same drive type and number of sectors,
> +     * (2) The same diskette size and number of sectors,
> +     * (3) The same number of sectors,
> +     * (4) The same drive type.
> +     *
> +     * In all cases, matches that occur higher in the drive table will take
> +     * precedence over matches that occur later in the table.

Yay - comments!  Makes it somewhat easier to follow.

> +     */
>      blk_get_geometry(blk, &nb_sectors);
> -    match = -1;
> -    first_match = -1;
> +    match = size_match = type_match = -1;
>      for (i = 0; ; i++) {
>          parse = &fd_formats[i];
>          if (parse->drive == FLOPPY_DRIVE_TYPE_NONE) {
>              break;
>          }
> -        if (drv->drive == parse->drive ||
> -            drv->drive == FLOPPY_DRIVE_TYPE_AUTO) {
> -            size = (parse->max_head + 1) * parse->max_track *
> -                parse->last_sect;
> -            if (nb_sectors == size) {
> -                match = i;
> -                break;
> +        size = (parse->max_head + 1) * parse->max_track * parse->last_sect;
> +        if (nb_sectors == size) {

To make sure I understand this:

The first three conditions are reached only when visiting a row where
nb_sectors matches.

> +            if (magic || parse->drive == drv->drive) {
> +                /* (1) perfect match */
> +                goto out;

The conditional means we get here only if the user specified 'magic'
(aka the new "auto" behavior of picking the first drive that works) or
an actual size (144/288/120).  Since the sectors match, we are set; and
this matches the old behavior (either drive type and sectors match, or
drive type was unspecified and sectors match).

> +            } else if (drive_size(parse->drive) == drive_size(drv->drive)) {
> +                /* (2) physical size match */
> +                match = (match == -1) ? i : match;

Here, drive_size(parse->drive) will never be FDRIVE_SIZE_UNKNOWN, but
drive_size(drv->drive) depends on what the user told us.  If they didn't
tell us what disk size to target, including if they asked for "auto",
this rule will never match.  If they DID tell us a disk size
(144/288/120), then this favors their disk size.  More concretely, if
they asked for 2880 sectors and a 288 disk, the old algorithm would fail
(no 288 entry has 2880 sectors), but now succeeds (the 144 entry with
2880 sectors has the same 3.5" disk size).  This is a good bug fix.

> +            } else {
> +                /* (3) nsectors match only */

nb_sectors?

> +                size_match = (size_match == -1) ? i : size_match;

Here, we don't have a drive match.  Either the user did not specify a
drive type [backwards-compatible behavior - the old loop found the first
entry with a matching size among every row of the table], or they
specified 144/288 while we are visiting entries for 120, or they
specified 120 while we are visiting entries for 144/288) [new behavior].

Should I worry about the new behavior?  If the size is one of the four
ambiguous rows (2880, 3200, 1440, 720), then we are okay - even though
we set 'size_match' on this iteration, another iteration will also set
'match' on the counterpart row, and 'match' takes priority over
'size_match'.

BUT what if the user asked for 3360 sectors and a 120 disk?  The old
code would have set 'first_match' to the 120 15,80,1 row (first row that
matches the drive type, since the search is limited to just 120 rows and
there was no size match); but your code looks for 3360 across ALL rows,
and ends up setting 'size_match' to the 144 21,80,1 row.  And since your
code never sets 'match' (none of the 120 rows match in size), you end up
_changing_ the disk format compared to the old code.

Thus, I think you still have a bug. :(

>              }
> -            if (first_match == -1) {
> -                first_match = i;
> +        } else if (type_match == -1) {

Here, we are visiting a row where sectors doesn't match.

> +            if ((parse->drive == drv->drive) ||
> +                (magic && (parse->drive == get_fallback_drive_type(drv)))) {
> +                /* (4) type matches, or type matches the autodetect default 
> if
> +                 *     we are using the autodetect mechanism. */
> +                type_match = i;

In which case, if the user told us a size, or the default for the "auto"
size matches, we set 'type_match' (in effect, picking the first
144/288/120 row that matches their explicit or default type).

>              }
>          }
>      }
> +
>      if (match == -1) {
> -        if (first_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;
> -        }
> -        parse = &fd_formats[match];
> +        match = (size_match != -1) ? size_match : type_match;

Only reached for (2), (3), and (4).  If (2) fired, 'match' is set and we
take that.  Otherwise if (3) fired, 'size_match' is set and we take that
(but see my claim that (3) can be buggy).  Otherwise, (4) better have
fired, because if not...

> +    }
> +
> +    if (match == -1) {
> +        error_setg(&error_abort, "No candidate geometries present in table "
> +                   " for floppy drive type '%s'",
> +                   FloppyDriveType_lookup[drv->drive]);

...we abort.  The abort looks sane.

>      }
>  
> +    parse = &(fd_formats[match]);
> +
> + out:

And the label is what lets us handle (1) as higher priority than
anything else.

>      if (parse->max_head == 0) {
>          drv->flags &= ~FDISK_DBL_SIDES;
>      } else {
> 

I think you can salvage the patch, though.  The condition for (3) is
currently 'else [if (1)]'.  But what if it is instead 'else if
(drive_size(drv->drive) == FDRIVE_SIZE_UKNOWN)'?  Then we are only
looking for a sector match on rows where we don't know what drive the
user wants, and leaving 'size_match' unchanged on rows that differ in
disk size from an explicit user's request.

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

signature.asc
Description: OpenPGP digital signature