From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47102) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aM0EQ-0003SD-Ue for qemu-devel@nongnu.org; Wed, 20 Jan 2016 16:24:00 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aM0EP-0004j1-Kj for qemu-devel@nongnu.org; Wed, 20 Jan 2016 16:23:58 -0500 References: <1453272694-17106-1-git-send-email-jsnow@redhat.com> <1453272694-17106-6-git-send-email-jsnow@redhat.com> From: Eric Blake Message-ID: <569FFAE7.7020900@redhat.com> Date: Wed, 20 Jan 2016 14:23:51 -0700 MIME-Version: 1.0 In-Reply-To: <1453272694-17106-6-git-send-email-jsnow@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="A26MOk3js1NcVc76dsSPlqrr4kuKKbaPW" Subject: Re: [Qemu-devel] [PATCH v4 05/12] fdc: Throw an assertion on misconfigured fd_formats table 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) --A26MOk3js1NcVc76dsSPlqrr4kuKKbaPW Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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. >=20 > 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 fir= st > 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 =3D=3D 0 but first_match =3D=3D -1, if nb_se= ctors 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". >=20 > This means: This means that either "match" was set, or we exited the loop without an exact match, in which case: >=20 > - 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. >=20 > - 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. >=20 > - If drive type is not NONE and the fd_formats table lists no options f= or > our drive type, we choose fd_formats[1], an incomprehensibly bizarre > choice that can never happen anyway. >=20 >=20 > 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. >=20 > Signed-off-by: John Snow > --- > 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 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --A26MOk3js1NcVc76dsSPlqrr4kuKKbaPW 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/ iQEcBAEBCAAGBQJWn/rnAAoJEKeha0olJ0NqIu4H/09Go9ReLiY7xoUIbSWLumo7 hl0pdmXUwFhcbQsj98pHEeeimXaKdB2d3nqW6UciQ6OpNbIvBTIWWa785itljBV1 l1PZwiRQbsEp8v5W1ExEjpKku4bVYMIBRfUjl2zQiNUJ8qhti9EIIolZ037BOcrI UWD1Bxay8a2nAF8m7iyJ6XpsnQQB6jlmVo6m8eBkgnDgWQ9TAXheJ3f7QWt/Cgb5 lMGeCobc3XzreSQwZpsm7/9SzgkaOhCZYgnYPVoTG9xNncMSDzOgOlafd2JM9G6O IB5rDkFr3+V0s63GNNSEyEEHAKXWNXLUlil1l+nZgE4h5zjGKhrlhAsZKqlFFYw= =j++i -----END PGP SIGNATURE----- --A26MOk3js1NcVc76dsSPlqrr4kuKKbaPW--