Re: [Qemu-devel] [PATCH] qapi: add dirty bitmap status

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> On 05/22/2015 04:52 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> On 05/12/2015 01:53 PM, John Snow wrote:
>>>> Bitmaps can be in a handful of different states with potentially
>>>> more to come as we tool around with migration and persistence patches.
>>>>
>>>> Instead of having a bunch of boolean fields, it was suggested that we
>>>> just have an enum status field that will help expose the reason to
>>>> management APIs why certain bitmaps may be unavailable for various
>>>> commands
>>>>
>>>> (e.g. busy in another operation, busy being migrated, etc.)
>>>
>>> Might be worth mentioning that this is an API change, but safe because
>>> the old API is unreleased (and therefore, this patch MUST go in the 2.4
>>> time frame, if at all).
>>>
>>>>
>>>> Suggested-by: Eric Blake <eblake@redhat.com>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>  block.c               | 13 ++++++++++++-
>>>>  include/block/block.h |  1 +
>>>>  qapi/block-core.json  | 23 +++++++++++++++++++++--
>>>>  3 files changed, 34 insertions(+), 3 deletions(-)
>>>>
>>>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> 
>> Patch does two things:
>> 
>> 1. Convert status from bool frozen to enum.
>> 2. Add new status 'disabled'.
>> 
>> I would've done this separately, but it's no big deal.  But I think we
>> should spell it out in the commit message.
>> 
>> What about:
>> 
>>     qapi: add dirty bitmap status
>> 
>>     Bitmaps can be in a handful of different states with potentially
>>     more to come as we tool around with migration and persistence patches.
>> 
>>     Management applications may need to know why certain bitmaps are
>>     unavailable for various commands, e.g. busy in another operation,
>>     busy being migrated, etc.
>> 
>>     Right now, all we offer is BlockDirtyInfo's boolean member 'frozen'.
>>     Instead of adding more booleans, replace it by an enumeration member
>>     'status' with values 'active' and 'frozen'.  Then add new value
>>     'disabled'.
>> 
>>     Incompatible change.  Fine because the changed part hasn't been
>>     released so far.
>> 
>>     Suggested-by: Eric Blake <eblake@redhat.com>
>>     Signed-off-by: John Snow <jsnow@redhat.com>
>>     Reviewed-by: Eric Blake <eblake@redhat.com>
>>     [Commit message tweaked]
>>     Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> 
>
> This is OK by me, Markus. I'll assume that you are OK with or have
> already made these changes locally, so I won't resend.

Applied to my block-next branch, thanks!