diff mbox

[v5,10/12] fdc: rework pick_geometry

Message ID 1453495865-9649-11-git-send-email-jsnow@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

John Snow Jan. 22, 2016, 8:51 p.m. UTC
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.
 - Fall back to the first geometry that matched our type.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/block/fdc.c | 72 ++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 52 insertions(+), 20 deletions(-)

Comments

John Snow Jan. 22, 2016, 8:59 p.m. UTC | #1
On 01/22/2016 03: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.
>  - Fall back to the first geometry that matched our type.
> 

Goofed and didn't update commit. Will change on PULL to omit the third
line if the patch is otherwise OK.

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/block/fdc.c | 72 ++++++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 52 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index e51154b..6e0c5fc 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -125,7 +125,6 @@ static const FDFormat fd_formats[] = {
>      { FLOPPY_DRIVE_TYPE_NONE, -1, -1, 0, 0, },
>  };
>  
> -__attribute__((__unused__))
>  static FDriveSize drive_size(FloppyDriveType drive)
>  {
>      switch (drive) {
> @@ -284,45 +283,78 @@ static int pick_geometry(FDrive *drv)
>      BlockBackend *blk = drv->blk;
>      const FDFormat *parse;
>      uint64_t nb_sectors, size;
> -    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 drive type.
> +     *
> +     * In all cases, matches that occur higher in the drive table will take
> +     * precedence over matches that occur later in the table.
> +     */
>      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) {
> +            if (magic || parse->drive == drv->drive) {
> +                /* (1) perfect match -- nb_sectors and drive type */
> +                goto out;
> +            } else if (drive_size(parse->drive) == drive_size(drv->drive)) {
> +                /* (2) size match -- nb_sectors and physical medium size */
> +                match = (match == -1) ? i : match;
> +            } else {
> +                /* This is suspicious -- Did the user misconfigure? */
> +                size_match = (size_match == -1) ? i : size_match;
>              }
> -            if (first_match == -1) {
> -                first_match = i;
> +        } else if (type_match == -1) {
> +            if ((parse->drive == drv->drive) ||
> +                (magic && (parse->drive == get_fallback_drive_type(drv)))) {
> +                /* (3) type match -- nb_sectors mismatch, but matches the type
> +                 *     specified explicitly by the user, or matches the fallback
> +                 *     default type when using the drive autodetect mechanism */
> +                type_match = i;
>              }
>          }
>      }
> +
> +    /* No exact match found */
>      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;
> +        if (size_match != -1) {
> +            parse = &fd_formats[size_match];
> +            FLOPPY_DPRINTF("User requested floppy drive type '%s', "
> +                           "but inserted medium appears to be a "
> +                           "%d sector '%s' type\n",
> +                           FloppyDriveType_lookup[drv->drive],
> +                           nb_sectors,
> +                           FloppyDriveType_lookup[parse->drive]);
>          }
> -        parse = &fd_formats[match];
> +        match = type_match;
>      }
>  
> +    /* No match of any kind found -- fd_format is misconfigured, abort. */
> +    if (match == -1) {
> +        error_setg(&error_abort, "No candidate geometries present in table "
> +                   " for floppy drive type '%s'",
> +                   FloppyDriveType_lookup[drv->drive]);
> +    }
> +
> +    parse = &(fd_formats[match]);
> +
> + out:
>      if (parse->max_head == 0) {
>          drv->flags &= ~FDISK_DBL_SIDES;
>      } else {
>
Eric Blake Jan. 25, 2016, 5:48 p.m. UTC | #2
On 01/22/2016 01:59 PM, John Snow wrote:
> 
> 
> On 01/22/2016 03: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.
>>  - Fall back to the first geometry that matched our type.
>>
> 
> Goofed and didn't update commit. Will change on PULL to omit the third
> line if the patch is otherwise OK.

Yep, that's the only thing I noticed.

Reviewed-by: Eric Blake <eblake@redhat.com>
diff mbox

Patch

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index e51154b..6e0c5fc 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -125,7 +125,6 @@  static const FDFormat fd_formats[] = {
     { FLOPPY_DRIVE_TYPE_NONE, -1, -1, 0, 0, },
 };
 
-__attribute__((__unused__))
 static FDriveSize drive_size(FloppyDriveType drive)
 {
     switch (drive) {
@@ -284,45 +283,78 @@  static int pick_geometry(FDrive *drv)
     BlockBackend *blk = drv->blk;
     const FDFormat *parse;
     uint64_t nb_sectors, size;
-    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 drive type.
+     *
+     * In all cases, matches that occur higher in the drive table will take
+     * precedence over matches that occur later in the table.
+     */
     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) {
+            if (magic || parse->drive == drv->drive) {
+                /* (1) perfect match -- nb_sectors and drive type */
+                goto out;
+            } else if (drive_size(parse->drive) == drive_size(drv->drive)) {
+                /* (2) size match -- nb_sectors and physical medium size */
+                match = (match == -1) ? i : match;
+            } else {
+                /* This is suspicious -- Did the user misconfigure? */
+                size_match = (size_match == -1) ? i : size_match;
             }
-            if (first_match == -1) {
-                first_match = i;
+        } else if (type_match == -1) {
+            if ((parse->drive == drv->drive) ||
+                (magic && (parse->drive == get_fallback_drive_type(drv)))) {
+                /* (3) type match -- nb_sectors mismatch, but matches the type
+                 *     specified explicitly by the user, or matches the fallback
+                 *     default type when using the drive autodetect mechanism */
+                type_match = i;
             }
         }
     }
+
+    /* No exact match found */
     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;
+        if (size_match != -1) {
+            parse = &fd_formats[size_match];
+            FLOPPY_DPRINTF("User requested floppy drive type '%s', "
+                           "but inserted medium appears to be a "
+                           "%d sector '%s' type\n",
+                           FloppyDriveType_lookup[drv->drive],
+                           nb_sectors,
+                           FloppyDriveType_lookup[parse->drive]);
         }
-        parse = &fd_formats[match];
+        match = type_match;
     }
 
+    /* No match of any kind found -- fd_format is misconfigured, abort. */
+    if (match == -1) {
+        error_setg(&error_abort, "No candidate geometries present in table "
+                   " for floppy drive type '%s'",
+                   FloppyDriveType_lookup[drv->drive]);
+    }
+
+    parse = &(fd_formats[match]);
+
+ out:
     if (parse->max_head == 0) {
         drv->flags &= ~FDISK_DBL_SIDES;
     } else {