From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49676) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6mTd-0003k8-Mf for qemu-devel@nongnu.org; Tue, 15 Nov 2016 17:45:18 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c6mTc-0004w8-JG for qemu-devel@nongnu.org; Tue, 15 Nov 2016 17:45:17 -0500 References: <1478715476-132280-1-git-send-email-vsementsov@virtuozzo.com> <1478715476-132280-8-git-send-email-vsementsov@virtuozzo.com> <4f079564-831d-3d06-ceea-29f3b17d62cf@redhat.com> From: John Snow Message-ID: <889e1dbf-3b67-9730-f00b-d0ea87a15045@redhat.com> Date: Tue, 15 Nov 2016 17:45:08 -0500 MIME-Version: 1.0 In-Reply-To: <4f079564-831d-3d06-ceea-29f3b17d62cf@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable 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: Eric Blake , 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 On 11/15/2016 05:39 PM, Eric Blake wrote: > On 11/15/2016 03:42 PM, John Snow wrote: >> >> >> 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 lat= er. >>> >>> 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; >>> + } >>> + >> >> 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. > >> >> This may be a good place for Eric to check our English. >> > > 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"); >> >> 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?) >> > > 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(). > We wrote it that way in the spec: ```` The fields of the bitmaps extension are: Byte 0 - 3: nb_bitmaps The number of bitmaps contained in the image. Must be greater than or equal to 1. ```` So if we have no bitmaps, we must have no header. > >> >>> + 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; >>> + } >>> + >> >> "Too large dirty bitmap" is an awkward phrasing, because it turns the >> entire message into a large compound noun. >> >> I suggest working in a verb into the message, like "is" or "exceeds," >> here are some suggestions: >> >> "[the] dirty bitmap directory size is too large" or "[the] dirty bitma= p >> directory size (%zu) exceeds [the] maximum supported size (%zu)" > > The latter is the most informative. > >> >> 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 > 'Dirty bitmap directory size (%zu) exceeds the maximum supported size=20 (%zu)' is probably my favorite upon review. >> >> Luckily nobody else knows how English works either. > > What, there's rules to follow? :) > Not that I am aware of. Thanks for the proofreading. --=20 =E2=80=94js