From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48207) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6mNm-0008S7-VQ for qemu-devel@nongnu.org; Tue, 15 Nov 2016 17:39:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c6mNl-0002pA-QZ for qemu-devel@nongnu.org; Tue, 15 Nov 2016 17:39:14 -0500 References: <1478715476-132280-1-git-send-email-vsementsov@virtuozzo.com> <1478715476-132280-8-git-send-email-vsementsov@virtuozzo.com> From: Eric Blake Message-ID: <4f079564-831d-3d06-ceea-29f3b17d62cf@redhat.com> Date: Tue, 15 Nov 2016 16:39:06 -0600 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="f4OW9A0obQm8uHd0M0X3LSGQXVauIxAC0" Subject: Re: [Qemu-devel] [PATCH 07/21] qcow2: add dirty bitmaps extension List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , Vladimir Sementsov-Ogievskiy , qemu-block@nongnu.org, qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, armbru@redhat.com, mreitz@redhat.com, stefanha@redhat.com, pbonzini@redhat.com, den@openvz.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --f4OW9A0obQm8uHd0M0X3LSGQXVauIxAC0 From: Eric Blake To: John Snow , Vladimir Sementsov-Ogievskiy , qemu-block@nongnu.org, qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, armbru@redhat.com, mreitz@redhat.com, stefanha@redhat.com, pbonzini@redhat.com, den@openvz.org Message-ID: <4f079564-831d-3d06-ceea-29f3b17d62cf@redhat.com> Subject: Re: [Qemu-devel] [PATCH 07/21] qcow2: add dirty bitmaps extension References: <1478715476-132280-1-git-send-email-vsementsov@virtuozzo.com> <1478715476-132280-8-git-send-email-vsementsov@virtuozzo.com> In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/15/2016 03:42 PM, John Snow wrote: >=20 >=20 > On 11/09/2016 01:17 PM, Vladimir Sementsov-Ogievskiy wrote: >> Add dirty bitmap extension as specified in docs/specs/qcow2.txt. >> For now, just mirror extension header into Qcow2 state and check >> constraints. >> >> For now, disable image resize if it has bitmaps. It will be fixed late= r. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy = >> --- >> + if (!(s->autoclear_features & >> QCOW2_AUTOCLEAR_DIRTY_BITMAPS)) { >> + fprintf(stderr, >> + "WARNING: bitmaps_ext: autoclear flag is not = " >> + "set, all bitmaps will be considered as >> inconsistent"); >> + break; >> + } >> + >=20 > I might drop the "as" and just say "WARNING: bitmaps_ext: [the] > autoclear flag is not set. All bitmaps will be considered inconsistent.= " Even the 'bitmaps_ext:' prefix seems a bit redundant, since the rest of the message talks about bitmaps. >=20 > This may be a good place for Eric to check our English. >=20 Maybe take the message from a different angle: WARNING: all bitmaps are considered inconsistent since the autoclear flag was cleared or WARNING: the autoclear flag was cleared, so all bitmaps are considered inconsistent or even skip the technical details, and report it with a longer message but while still sounding legible: WARNING: a program lacking bitmap support modified this file, so all bitmaps are now considered inconsistent >> + >> + if (bitmaps_ext.nb_bitmaps > QCOW_MAX_DIRTY_BITMAPS) { >> + error_setg(errp, "ERROR: bitmaps_ext: " >> + "too many dirty bitmaps"); >=20 > I might opt for something more like "File %s has %d bitmaps, exceeding > the QEMU supported maximum of %d" to be a little more informative than > "too many." (How many is too many? How many do we have?) >=20 The use of ERROR: in error_setg() seems over the top. John's proposed wording is nice, here. >> + return -EINVAL; >> + } >> + >> + if (bitmaps_ext.nb_bitmaps =3D=3D 0) { >> + error_setg(errp, "ERROR: bitmaps_ext: " >> + "found bitmaps extension with zero >> bitmaps"); So why is it an error to have a bitmaps extension but no bitmaps allocated? Is that too strict? Again, the ERROR: prefix is a bit much in error_setg(). >=20 >> + if (bitmaps_ext.bitmap_directory_size > >> + QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) { >> + error_setg(errp, "ERROR: bitmaps_ext: " >> + "too large dirty bitmap directory");= >> + return -EINVAL; >> + } >> + >=20 > "Too large dirty bitmap" is an awkward phrasing, because it turns the > entire message into a large compound noun. >=20 > I suggest working in a verb into the message, like "is" or "exceeds," > here are some suggestions: >=20 > "[the] dirty bitmap directory size is too large" or "[the] dirty bitmap= > directory size (%zu) exceeds [the] maximum supported size (%zu)" The latter is the most informative. >=20 > I can't decide if it's appropriate to include or exclude the article. Yep, choosing when to use articles is sometimes subjective. the/blank sounds odd - it's the only combo I'd avoid blank/blank seems reasonable, and has the benefit of being short blank/the seems reasonable the/the seems rather formal, but still works >=20 > Luckily nobody else knows how English works either. What, there's rules to follow? :) --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --f4OW9A0obQm8uHd0M0X3LSGQXVauIxAC0 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/ iQEcBAEBCAAGBQJYK46KAAoJEKeha0olJ0Nqo/UIAIAHD1znLHNO+dfNba7mc+AG o/GZvB2QUVberZ52NtPUrLCgKYRtCG/4x2MsGa4ZowOfwMZ+SAak3XCZMZZTHzBm kDx83vGhrgSascOvLXFaY9Rpqkz5e88rJ9Uh0cTANuD9ymJsT5kQuGyMRdhplORX 3BAWX6AuBv4v4RdLEmaJHbJPymLxYw/9WRz9FE2IebzG9vsr7thf2RNuf55+w6Bc HUN4jQMJGoF8P83tqAdqPaIYziSSpZmDyzRKqutnrJpZ0gx951RNSxmaKD//uWl8 XVBAuoRMu4Gg2dKHGUkRDtlYgfZfZUZrdkjaupMK/Iz3SVyTkmJ1tD04qhACWPc= =I0qv -----END PGP SIGNATURE----- --f4OW9A0obQm8uHd0M0X3LSGQXVauIxAC0--