| Message ID | 1453272694-17106-11-git-send-email-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <jsnow@redhat.com> > --- > - 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.
On 01/20/2016 06:45 PM, Eric Blake wrote: > 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. >> For sake of review, we'll call these choices (A) and (B). >> 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. >> And these choices will be (1) through (4) as they are annotated in the comment below in this way as well. (1) is identical to (A), and (4) is identical to (B). That leaves (2) and (3) are new behaviors. There is strong rationale for (2). (3) was invented as a last-ditch effort before (4), see below! >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- > >> - 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. > Yes. >> + 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). > Yes. This is the old (A), identical to what is now documented as (1). >> + } 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. > Yes. This is new behavior, documented as (2). >> + } else { >> + /* (3) nsectors match only */ > > nb_sectors? > Yes, I can update the comment to be less fuzzy. >> + 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 The old behavior you are referencing is (A), but that's already taken care of under (1) above because of the (magic || parse->drive == drv->drive) conditional. If magic is set, the types (or physical sizes) do not need to match, just the number of sectors -- and the match is so strong, we exit immediately. > specified 144/288 while we are visiting entries for 120, or they > specified 120 while we are visiting entries for 144/288) [new behavior]. > New indeed. > 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'. > You are right to worry. Rationale below. > 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. :( > It's definitely new behavior. In either case, I think we can agree that QEMU is choosing a garbage format as a last-ditch effort to see if something works. The old behavior of "Choose a wrong geometry of the right type as a last-chance effort" was something I thought that maybe I could "improve" by doing: "Choose a wrong geometry of the right type as a last-chance effort, unless we found something that is the correct number of sectors, in which case maybe use that as a backup strategy because it might work better." It is decidedly "new" behavior, but it is similarly to the old strategy a recourse action when faced with the absence of a more precise match (right size and type (1) or right size and correct physical size (2)) The thought was basically: "I bet the floppy drive code would cope better by setting a geometry that is at the very least the right number of sectors over one that's clearly wrong." >> } >> - if (first_match == -1) { >> - first_match = i; >> + } else if (type_match == -1) { > > Here, we are visiting a row where sectors doesn't match. > Yes, the old "hail mary" behavior present from 2003-2016, what I annotated as type (B) in my reply above. Now documented as behavior (4). >> + 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). > Right, this is the old behavior. "We couldn't find the right geometry, so we're just going to pick one that is of the type you specified and hope you know what you're doing." The nugget of new behavior here is that if the user did NOT specify a type, and we STILL managed to not find a precise matching geometry, we'll choose the first geometry that belongs to the fallback 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... > Should be no way for it not to! >> + } >> + >> + 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. > Should be unnecessary -- (1) should handle nb_sectors match adequately when the user did not specify a drive type. If (3) is undesired, it can be scrapped outright. --js
On 01/21/2016 01:14 PM, John Snow wrote: > > > On 01/20/2016 06:45 PM, Eric Blake wrote: >> 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. >>> > > For sake of review, we'll call these choices (A) and (B). > >>> 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. >>> > > And these choices will be (1) through (4) as they are annotated in the > comment below in this way as well. > > (1) is identical to (A), and (4) is identical to (B). Agreed. > That leaves (2) and (3) are new behaviors. > > There is strong rationale for (2). > (3) was invented as a last-ditch effort before (4), see below! > >> >> Thus, I think you still have a bug. :( >> > > It's definitely new behavior. In either case, I think we can agree that > QEMU is choosing a garbage format as a last-ditch effort to see if > something works. > > The old behavior of "Choose a wrong geometry of the right type as a > last-chance effort" was something I thought that maybe I could "improve" > by doing: > > "Choose a wrong geometry of the right type as a last-chance effort, > unless we found something that is the correct number of sectors, in > which case maybe use that as a backup strategy because it might work > better." I don't think the geometry for a 120 row is going to work right for a guest expecting 144/288 behavior. > > It is decidedly "new" behavior, but it is similarly to the old strategy > a recourse action when faced with the absence of a more precise match > (right size and type (1) or right size and correct physical size (2)) > > The thought was basically: "I bet the floppy drive code would cope > better by setting a geometry that is at the very least the right number > of sectors over one that's clearly wrong." I think the point of this function is: If the user told us a disk type and gave us an image with an exact number of sectors, use the geometry associated with that size. Behavior (2) refines this to further pick sizes that are compatible with the current disk type (with a 288 drive, the exact geometry for 2880 sectors visiting a 144 disk is probably better than the behavior (B) choice for the first 288 disk). If the user did not tell us a disk type, favor a geometry that matches the number of sectors. If the size can occur from more than one disk type, we used to favor the disk type that occurred first in the list (2880 sectors maps to 3.5", not 5.25"; while 720 sectors favors 5.25"); due to behavior (2) the new code may instead favor the disk size that matches the default fallback of "auto" (if auto falls back to 144 or 288, 720 sectors will now select that size rather than 5.25"). If the user told us a disk type but we don't have a sector match, then pick the first geometry associated with that disk type (hopefully the host image is smaller than that choice, and we pad out the host file to pretend that it matches the most common use of that disk type from the guest's view). Finally, the user told us nothing and we have no size heuristic to fall back on, so pick a (hopefully sane) default of the first table entry (old behavior) or the "auto" fallback (new behavior). > >>> } >>> - if (first_match == -1) { >>> - first_match = i; >>> + } else if (type_match == -1) { >> >> Here, we are visiting a row where sectors doesn't match. >> > > Yes, the old "hail mary" behavior present from 2003-2016, what I > annotated as type (B) in my reply above. Now documented as behavior (4). > >>> + 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). >> > > Right, this is the old behavior. "We couldn't find the right geometry, > so we're just going to pick one that is of the type you specified and > hope you know what you're doing." > > The nugget of new behavior here is that if the user did NOT specify a > type, and we STILL managed to not find a precise matching geometry, > we'll choose the first geometry that belongs to the fallback type. Which seems like a good change (if you don't tell us a disk type, then we'll default to giving you a 144/288 drive UNLESS hueristics prove you were probably trying to open a 120 format based on sector size). >> 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 I meant that as 'if (true)', not 'if (condition identical to (1) in the discussion above)'. >> (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. >> > > Should be unnecessary -- (1) should handle nb_sectors match adequately > when the user did not specify a drive type. After (re-)reading the use of 'magic', I concur. > > If (3) is undesired, it can be scrapped outright. I think that's the best approach then - drop condition (3) outright, and only do an early exit (1), set 'match' (2), or set 'type_match' (4). Tricky patch, but I think with that change, I'll be on board for your v5 patch.
diff --git a/hw/block/fdc.c b/hw/block/fdc.c index 8a9747c..f8f7e6d 100644 --- a/hw/block/fdc.c +++ b/hw/block/fdc.c @@ -124,7 +124,6 @@ static const FDFormat fd_formats[] = { { FLOPPY_DRIVE_TYPE_NONE, -1, -1, 0, 0, }, }; -__attribute__((__unused__)) static FDriveSize drive_size(FloppyDriveType drive) { switch (drive) { @@ -283,45 +282,67 @@ 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 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. + */ 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 */ + goto out; + } else if (drive_size(parse->drive) == drive_size(drv->drive)) { + /* (2) physical size match */ + match = (match == -1) ? i : match; + } else { + /* (3) nsectors match only */ + 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)))) { + /* (4) type matches, or type matches the autodetect default if + * we are using the autodetect mechanism. */ + type_match = i; } } } + 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; + } + + 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 {
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 | 63 ++++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 42 insertions(+), 21 deletions(-)