[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: John Snow
Subject: Re: [Qemu-devel] [PATCH 07/21] qcow2: add dirty bitmaps extension
Date: Tue, 15 Nov 2016 17:45:08 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0

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

For now, disable image resize if it has bitmaps. It will be fixed later.

Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>

+            if (!(s->autoclear_features &
+                fprintf(stderr,
+                        "WARNING: bitmaps_ext: autoclear flag is not "
+                        "set, all bitmaps will be considered as
+                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


WARNING: the autoclear flag was cleared, so all bitmaps are considered

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

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


reply via email to

[Prev in Thread] Current Thread [Next in Thread]