From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35316) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aM2RI-0002HK-47 for qemu-devel@nongnu.org; Wed, 20 Jan 2016 18:45:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aM2RG-0005iF-Cu for qemu-devel@nongnu.org; Wed, 20 Jan 2016 18:45:24 -0500 References: <1453272694-17106-1-git-send-email-jsnow@redhat.com> <1453272694-17106-11-git-send-email-jsnow@redhat.com> From: Eric Blake Message-ID: <56A01C06.4010804@redhat.com> Date: Wed, 20 Jan 2016 16:45:10 -0700 MIME-Version: 1.0 In-Reply-To: <1453272694-17106-11-git-send-email-jsnow@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="cMWVFrSGx9UIodKh8ikXXd1BvLA4ht5gL" Subject: Re: [Qemu-devel] [PATCH v4 10/12] fdc: rework pick_geometry List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , qemu-block@nongnu.org Cc: kwolf@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --cMWVFrSGx9UIodKh8ikXXd1BvLA4ht5gL Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 01/19/2016 11:51 PM, John Snow wrote: > This one is the crazy one. >=20 > 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. >=20 > 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. >=20 > The old one is roughly: > - If the size (sectors) and type matches, choose it. > - Fall back to the first geometry that matched our type. >=20 > 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. >=20 > Signed-off-by: John Snow > --- > - int i, first_match, match; > + int i; > + int match, size_match, type_match; > + bool magic =3D drv->drive =3D=3D FLOPPY_DRIVE_TYPE_AUTO; > =20 > /* We can only pick a geometry if we have a diskette. */ > if ((!drv->media_inserted) || (drv->drive =3D=3D FLOPPY_DRIVE_TYPE= _NONE)) { > return -1; > } > =20 > + /* We need to determine the likely geometry of the inserted medium= =2E > + * 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 =3D -1; > - first_match =3D -1; > + match =3D size_match =3D type_match =3D -1; > for (i =3D 0; ; i++) { > parse =3D &fd_formats[i]; > if (parse->drive =3D=3D FLOPPY_DRIVE_TYPE_NONE) { > break; > } > - if (drv->drive =3D=3D parse->drive || > - drv->drive =3D=3D FLOPPY_DRIVE_TYPE_AUTO) { > - size =3D (parse->max_head + 1) * parse->max_track * > - parse->last_sect; > - if (nb_sectors =3D=3D size) { > - match =3D i; > - break; > + size =3D (parse->max_head + 1) * parse->max_track * parse->las= t_sect; > + if (nb_sectors =3D=3D 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 =3D=3D 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) =3D=3D drive_size(drv-= >drive)) { > + /* (2) physical size match */ > + match =3D (match =3D=3D -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 =3D (size_match =3D=3D -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 =3D=3D -1) { > - first_match =3D i; > + } else if (type_match =3D=3D -1) { Here, we are visiting a row where sectors doesn't match. > + if ((parse->drive =3D=3D drv->drive) || > + (magic && (parse->drive =3D=3D get_fallback_drive_type= (drv)))) { > + /* (4) type matches, or type matches the autodetect de= fault if > + * we are using the autodetect mechanism. */ > + type_match =3D 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 =3D=3D -1) { > - if (first_match =3D=3D -1) { > - error_setg(&error_abort, "No candidate geometries present = in table " > - " for floppy drive type '%s'", > - FloppyDriveType_lookup[drv->drive]); > - } else { > - match =3D first_match; > - } > - parse =3D &fd_formats[match]; > + match =3D (size_match !=3D -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 =3D=3D -1) { > + error_setg(&error_abort, "No candidate geometries present in t= able " > + " for floppy drive type '%s'", > + FloppyDriveType_lookup[drv->drive]); =2E..we abort. The abort looks sane. > } > =20 > + parse =3D &(fd_formats[match]); > + > + out: And the label is what lets us handle (1) as higher priority than anything else. > if (parse->max_head =3D=3D 0) { > drv->flags &=3D ~FDISK_DBL_SIDES; > } else { >=20 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) =3D=3D 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. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --cMWVFrSGx9UIodKh8ikXXd1BvLA4ht5gL Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWoBwGAAoJEKeha0olJ0Nq73QH/3HWkNm/boWrWbywNiZF+VWe 6l/IyvKWGRIppjTbvfnKYwawl4rCdDuqYTEhziRzQE5M3G5MzmHBwiTk0TdSa63e 8S/SE4x4mecWhUDxZS4MPKk8maT+JEAH+WMgsQYSPiKgDnOstQY2eAFHB5RSOC0O eIx7kdbPy4gP2xu2/zNYVlE3nhQC+cg5Lx3UsUEiK7MEzryUXorcQeTY5pz8uH+2 5gYhyGc9nrtFBEXAWtLlR7MbK7lvfqceXRugNAJ534Qz+tYz1OJY99ny3c6Iqrcf ygmnt0AOrAwAgmIICktJnDs4HVcv0+5hZkXx+S3GmzDpGr+bWagN1IHtwZX/kFk= =T/2P -----END PGP SIGNATURE----- --cMWVFrSGx9UIodKh8ikXXd1BvLA4ht5gL--