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

[John Snow <jsnow@redhat.com>]

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

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

'Dirty bitmap directory size (%zu) exceeds the maximum supported size 
(%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.

-- 
—js