Re: [Qemu-devel] [PATCH 07/21] qcow2: add dirty bitmaps extension

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3796 bytes --]

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 later.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---

>> +            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 == 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().


> 
>> +            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 bitmap
> 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

> 
> Luckily nobody else knows how English works either.

What, there's rules to follow?  :)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]