Re: [Qemu-block] [PATCH v2] block/dirty-bitmap: Documentation and Comment fixups

[John Snow]

On 2/4/19 10:00 AM, Eric Blake wrote:
> On 2/1/19 7:10 PM, John Snow wrote:
>> The meaning of the states has changed subtly over time,
>> this should bring the understanding more in-line with the
>> current, actual usages.
>>
>> Reported-by: Eric Blake <address@hidden>
>> Signed-off-by: John Snow <address@hidden>
>>
>> ---
>> V2: Amended some wordings, though this is a little chatty now.
>>     Hopefully it's clearer, though.
>> ---
> 
>>  /**
>> - * A BdrvDirtyBitmap can be in three possible states:
>> - * (1) successor is NULL and disabled is false: full r/w mode
>> - * (2) successor is NULL and disabled is true: read only mode ("disabled")
>> - * (3) successor is set: frozen mode.
>> - *     A frozen bitmap cannot be renamed, deleted, anonymized, cleared, set,
>> - *     or enabled. A frozen bitmap can only abdicate() or reclaim().
>> + * A BdrvDirtyBitmap can be in four possible user-visible states:
>> + * (1) Active:   successor is NULL, and disabled is false: full r/w mode
>> + * (2) Disabled: successor is NULL, and disabled is true: qualified r/w 
>> mode,
>> + *               guest writes are dropped, but monitor writes are possible,
>> + *               through commands like merge and clear.
>> + * (3) Frozen:   successor is not NULL.
>> + *               A frozen bitmap cannot be renamed, deleted, cleared, set,
>> + *               enabled, merged to, etc. A frozen bitmap can only 
>> abdicate()
>> + *               or reclaim().
>> + *               In this state, the anonymous successor bitmap may be either
>> + *               Active and recording writes from the guest (e.g. backup 
>> jobs),
>> + *               but it can be Disabled and not recording writes.
> 
> s/but/or/
> 

I'll touch that up.

>> + * (4) Locked:   Whether Active or Disabled, the user cannot modify this 
>> bitmap
>> + *               in any way from the monitor.
> 
> Worth a parenthetical comment mentioning NBD export as a possible reason
> for Locked to be visible?
> 

Since this is effectively developer documentation, yes. How about:

"Whether Active or Disabled, the user cannot modify this bitmap in any
way from the monitor. This mode is used to achieve a successor-less
locking of a bitmap for operations like point-in-time NBD export or for
incoming migration data."

> With the one word fix,
> Reviewed-by: Eric Blake <address@hidden>
> 

If Vladimir agrees with these phrasings I'll stage it.

--js