Re: [Qemu-devel] [PATCH v8] spec: add qcow2 bitmaps extension specification

[Max Reitz]

On 27.01.2016 16:52, Vladimir Sementsov-Ogievskiy wrote:
> The new feature for qcow2: storing bitmaps.
> 
> This patch adds new header extension to qcow2 - Bitmaps Extension. It
> provides an ability to store virtual disk related bitmaps in a qcow2
> image. For now there is only one type of such bitmaps: Dirty Tracking
> Bitmap, which just tracks virtual disk changes from some moment.
> 
> Note: Only bitmaps, relative to the virtual disk, stored in qcow2 file,
> should be stored in this qcow2 file. The size of each bitmap
> (considering its granularity) is equal to virtual disk size.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>

The semantics are good, I just have more grammar nitpicks from here on. :-)

[...]

> 
>  docs/specs/qcow2.txt | 225 
> ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 224 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
> index f236d8c..7b0ebef 100644
> --- a/docs/specs/qcow2.txt
> +++ b/docs/specs/qcow2.txt

[...]

> +        12 - 15:    flags
> +                    Bit
> +                      0: in_use
> +                         The bitmap was not saved correctly and may be
> +                         inconsistent.
> +
> +                      1: auto
> +                         The bitmap must reflect all changes of the virtual
> +                         disk by any application that would write to this 
> qcow2
> +                         file (including writes, snapshot switching, etc.). 
> The
> +                         type of this bitmap must be 'dirty tracking bitmap'.
> +
> +                      2: extra_data_compatible
> +                         This flags is meaningful when extra data is unknown 
> for

s/for/to/, and probably also "the extra data".

> +                         the software (currently any extra data is unknown 
> for

s/for/to/

> +                         Qemu).
> +                         If it is set, the bitmap may be used as expected, 
> extra
> +                         data must be left as is.
> +                         If it is not set, the bitmap must not be used, but 
> left
> +                         as is with extra data.

Maybe s/with/along with its/ would sound better; or "but both it and its
extra data be left as is".

> +
> +                    Bits 3 - 31 are reserved and must be 0.

[...]

> +=== Dirty tracking bitmaps ===
> +
> +Bitmaps with 'type' field equal to one are dirty tracking bitmaps.
> +
> +When the virtual disk is in use dirty tracking bitmap may be 'enabled' or
> +'disabled'. While the bitmap is 'enabled', all writes to the virtual disk
> +should be reflected in the bitmap. Set bit in the bitmap means that the

s/Set bit/A set bit/

> +corresponding range of the virtual disk (see above) was written while the

Maybe s/written/written to/, but that's optional ("written" sounds to me
like an allocating write, or as if everything in that range was
overwritten).

> +bitmap was 'enabled'. Unset bit means that this range was not written.

s/Unset bit/An unset bit/, and again maybe s/written/written to/.

> +
> +The software should not sync the bitmap in the image file with its
> +representation in RAM after each write. Flag 'in_use' should be set while the
> +bitmap is not synced.
> +
> +In the image file the 'enabled' state is reflected by 'auto' flag. If this 
> flag

s/'auto' flag/the 'auto' flag/

> +is set, the software must consider the bitmap as 'enabled' and start tracking
> +virtual disk changes to this bitmap from the first write to the virtual 
> disk. If
> +this flag is not set then the bitmap is constant.

s/constant/'disabled'/? It's basically the same, but "disabled" is what
you used above.

> +
> +To maintain the bitmap consistent, the only software is allowed to change the
> +value of 'auto' flag: the software which was created the bitmap.

I'd prefer:

To maintain bitmap consistency, the only software which is allowed to
change the value of the 'auto' flag is the one which has created the bitmap.

>                                                                   The other
> +software must not change this flag, it only tracks changes to this bitmap, if
> +'auto' flag is set and ignores the bitmap otherwise.

I'd drop the second part and shorten it to:

Any other software must not change this flag.

Or just drop it completely. The previous sentence completely suffices to
tell that no other program is allowed to modify it; and I found the
second part ("it only tracks...") confusing because I had to wonder
about what the "it" referred to, and because it's superfluous. It's just
repeating how any program is supposed to handle such a bitmap anyway.

Max

signature.asc
Description: OpenPGP digital signature