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