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

[Vladimir Sementsov-Ogievskiy]

04.02.2019 22:26, John Snow wrote:
> 
> 
> 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.
> 

Ooops, sorry for the long delay.

Honestly, I still don't like it very much, as is worded in manner that bitmap
may be in two states (Active and Locked), and it is not very good for something
called "state".. But anyway it is understandable and a lot better than what it 
was, so:

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>


-- 
Best regards,
Vladimir