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 <vsement...@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
signature.asc
Description: OpenPGP digital signature
