[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH 07/21] qcow2: add dirty bitmaps ext
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH 07/21] qcow2: add dirty bitmaps extension |
Date: |
Tue, 15 Nov 2016 16:39:06 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 |
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 <address@hidden>
>> ---
>> + 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
- Re: [Qemu-block] [Qemu-devel] [PATCH 04/21] tests: add hbitmap iter test, (continued)
- [Qemu-block] [PATCH 10/21] block/dirty-bitmap: add autoload field to BdrvDirtyBitmap, Vladimir Sementsov-Ogievskiy, 2016/11/09
- [Qemu-block] [PATCH 05/21] block: fix bdrv_dirty_bitmap_granularity signature, Vladimir Sementsov-Ogievskiy, 2016/11/09
- [Qemu-block] [PATCH 16/21] qmp: add persistent flag to block-dirty-bitmap-add, Vladimir Sementsov-Ogievskiy, 2016/11/09
- [Qemu-block] [PATCH 11/21] block: introduce persistent dirty bitmaps, Vladimir Sementsov-Ogievskiy, 2016/11/09
- [Qemu-block] [PATCH 18/21] qmp: add x-debug-block-dirty-bitmap-sha256, Vladimir Sementsov-Ogievskiy, 2016/11/09
- [Qemu-block] [PATCH 08/21] block: introduce auto-loading bitmaps, Vladimir Sementsov-Ogievskiy, 2016/11/09
- [Qemu-block] [PATCH 07/21] qcow2: add dirty bitmaps extension, Vladimir Sementsov-Ogievskiy, 2016/11/09
- [Qemu-block] [PATCH 19/21] iotests: test qcow2 persistent dirty bitmap, Vladimir Sementsov-Ogievskiy, 2016/11/09
- [Qemu-block] [PATCH 03/21] hbitmap: improve dirty iter, Vladimir Sementsov-Ogievskiy, 2016/11/09
- [Qemu-block] [PATCH 06/21] block/dirty-bitmap: add deserialize_ones func, Vladimir Sementsov-Ogievskiy, 2016/11/09
- [Qemu-block] [PATCH 13/21] qcow2: add .bdrv_store_persistent_dirty_bitmaps(), Vladimir Sementsov-Ogievskiy, 2016/11/09
- [Qemu-block] [PATCH 09/21] qcow2: add .bdrv_load_autoloading_dirty_bitmaps, Vladimir Sementsov-Ogievskiy, 2016/11/09