All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] qapi: add dirty bitmap status
@ 2015-05-12 19:53 John Snow
  2015-05-12 20:06 ` Eric Blake
  0 siblings, 1 reply; 14+ messages in thread
From: John Snow @ 2015-05-12 19:53 UTC (permalink / raw)
  To: qemu-devel; +Cc: John Snow

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.)

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(-)

diff --git a/block.c b/block.c
index 7904098..a8b9f25 100644
--- a/block.c
+++ b/block.c
@@ -3104,6 +3104,17 @@ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
     return !(bitmap->disabled || bitmap->successor);
 }
 
+DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap)
+{
+    if (bdrv_dirty_bitmap_frozen(bitmap)) {
+        return DIRTY_BITMAP_STATUS_FROZEN;
+    } else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
+        return DIRTY_BITMAP_STATUS_DISABLED;
+    } else {
+        return DIRTY_BITMAP_STATUS_ACTIVE;
+    }
+}
+
 /**
  * Create a successor bitmap destined to replace this bitmap after an operation.
  * Requires that the bitmap is not frozen and has no successor.
@@ -3244,7 +3255,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
         info->granularity = bdrv_dirty_bitmap_granularity(bm);
         info->has_name = !!bm->name;
         info->name = g_strdup(bm->name);
-        info->frozen = bdrv_dirty_bitmap_frozen(bm);
+        info->status = bdrv_dirty_bitmap_status(bm);
         entry->value = info;
         *plist = entry;
         plist = &entry->next;
diff --git a/include/block/block.h b/include/block/block.h
index 7d1a717..4d3ad88 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -474,6 +474,7 @@ uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
 uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
+DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
 int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
 void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
                            int64_t cur_sector, int nr_sectors);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 863ffea..8411d4f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -326,6 +326,25 @@
             'data': 'bool', '*offset': 'int' } }
 
 ##
+# @DirtyBitmapStatus:
+#
+# An enumeration of possible states that a dirty bitmap can report to the user.
+#
+# @frozen: The bitmap is currently in-use by a backup operation or block job,
+#          and is immutable.
+#
+# @disabled: The bitmap is currently in-use by an internal operation and is
+#            read-only. It can still be deleted.
+#
+# @active: The bitmap is actively monitoring for new writes, and can be cleared,
+#          deleted, or used for backup operations.
+#
+# Since: 2.4
+##
+{ 'enum': 'DirtyBitmapStatus',
+  'data': ['active', 'disabled', 'frozen'] }
+
+##
 # @BlockDirtyInfo:
 #
 # Block dirty bitmap information.
@@ -336,13 +355,13 @@
 #
 # @granularity: granularity of the dirty bitmap in bytes (since 1.4)
 #
-# @frozen: whether the dirty bitmap is frozen (Since 2.4)
+# @status: current status of the dirty bitmap (since 2.4)
 #
 # Since: 1.3
 ##
 { 'struct': 'BlockDirtyInfo',
   'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
-           'frozen': 'bool'} }
+           'status': 'DirtyBitmapStatus'} }
 
 ##
 # @BlockInfo:
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH] qapi: add dirty bitmap status
  2015-05-12 19:53 [Qemu-devel] [PATCH] qapi: add dirty bitmap status John Snow
@ 2015-05-12 20:06 ` Eric Blake
  2015-05-12 20:07   ` John Snow
                     ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Eric Blake @ 2015-05-12 20:06 UTC (permalink / raw)
  To: John Snow, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 1109 bytes --]

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>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH] qapi: add dirty bitmap status
  2015-05-12 20:06 ` Eric Blake
@ 2015-05-12 20:07   ` John Snow
  2015-05-19 21:18   ` John Snow
  2015-05-22  8:52   ` Markus Armbruster
  2 siblings, 0 replies; 14+ messages in thread
From: John Snow @ 2015-05-12 20:07 UTC (permalink / raw)
  To: Eric Blake, qemu-devel



On 05/12/2015 04:06 PM, Eric Blake wrote:
> 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).
> 

Good thing you mentioned it!

:)

>> 
>> 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>
> 

Thanks!

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH] qapi: add dirty bitmap status
  2015-05-12 20:06 ` Eric Blake
  2015-05-12 20:07   ` John Snow
@ 2015-05-19 21:18   ` John Snow
  2015-05-20  8:20     ` Markus Armbruster
  2015-05-22  8:52   ` Markus Armbruster
  2 siblings, 1 reply; 14+ messages in thread
From: John Snow @ 2015-05-19 21:18 UTC (permalink / raw)
  To: Eric Blake, qemu-devel; +Cc: Markus Armbruster



On 05/12/2015 04:06 PM, Eric Blake wrote:
> 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>
> 

I'm not actually sure whose tree this should go in. Markus's, perhaps?

("ping")

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH] qapi: add dirty bitmap status
  2015-05-19 21:18   ` John Snow
@ 2015-05-20  8:20     ` Markus Armbruster
  2015-05-21 21:48       ` John Snow
  0 siblings, 1 reply; 14+ messages in thread
From: Markus Armbruster @ 2015-05-20  8:20 UTC (permalink / raw)
  To: John Snow; +Cc: Kevin Wolf, qemu-devel

John Snow <jsnow@redhat.com> writes:

> On 05/12/2015 04:06 PM, Eric Blake wrote:
>> 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>
>> 
>
> I'm not actually sure whose tree this should go in. Markus's, perhaps?
>
> ("ping")

I guess the case for "Block layer core" (Kevin) is at least as strong as
the case for "QAPI" (me).  Kevin, what do you think?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH] qapi: add dirty bitmap status
  2015-05-20  8:20     ` Markus Armbruster
@ 2015-05-21 21:48       ` John Snow
  2015-05-22  8:22         ` Kevin Wolf
  0 siblings, 1 reply; 14+ messages in thread
From: John Snow @ 2015-05-21 21:48 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Kevin Wolf, qemu-devel



On 05/20/2015 04:20 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 05/12/2015 04:06 PM, Eric Blake wrote:
>>> 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>
>>>
>>
>> I'm not actually sure whose tree this should go in. Markus's, perhaps?
>>
>> ("ping")
> 
> I guess the case for "Block layer core" (Kevin) is at least as strong as
> the case for "QAPI" (me).  Kevin, what do you think?
> 

His silence says "Markus, can you please do it? I discovered today that
I don't care about this patch."

--js

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH] qapi: add dirty bitmap status
  2015-05-21 21:48       ` John Snow
@ 2015-05-22  8:22         ` Kevin Wolf
  2015-05-22  8:31           ` Markus Armbruster
  2015-05-22 15:36           ` John Snow
  0 siblings, 2 replies; 14+ messages in thread
From: Kevin Wolf @ 2015-05-22  8:22 UTC (permalink / raw)
  To: John Snow; +Cc: Markus Armbruster, qemu-block, qemu-devel

Am 21.05.2015 um 23:48 hat John Snow geschrieben:
> 
> 
> On 05/20/2015 04:20 AM, Markus Armbruster wrote:
> > John Snow <jsnow@redhat.com> writes:
> > 
> >> On 05/12/2015 04:06 PM, Eric Blake wrote:
> >>> 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>
> >>>
> >>
> >> I'm not actually sure whose tree this should go in. Markus's, perhaps?
> >>
> >> ("ping")
> > 
> > I guess the case for "Block layer core" (Kevin) is at least as strong as
> > the case for "QAPI" (me).  Kevin, what do you think?

I think bdrv_query_dirty_bitmaps() really belongs into block/qapi.c,
which is yours anyway. So it's either you as the QAPI maintainer or you
as the block submaintainer.

But if you think otherwise, I can consider it.

> His silence says "Markus, can you please do it? I discovered today that
> I don't care about this patch."

I'm sorry, John, but you didn't CC me, you didn't CC qemu-block, you
didn't CC anyone. I only had a chance to know about it since Wednesday
when Markus forwarded it, and I'm not sitting there waiting for new
patch emails because I'm bored. Rest assured, I have enough of them.

And then the forwarded email didn't even quote the patch any more, so I
couldn't just give a quick reply, but had to find the full email thread
in a different folder.

If you want to have patches applied quickly, make it easy for the
maintainers. You did the exact opposite, so you have no reason to
complain.

Kevin

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH] qapi: add dirty bitmap status
  2015-05-22  8:22         ` Kevin Wolf
@ 2015-05-22  8:31           ` Markus Armbruster
  2015-05-22 11:49             ` Kevin Wolf
  2015-05-22 15:36           ` John Snow
  1 sibling, 1 reply; 14+ messages in thread
From: Markus Armbruster @ 2015-05-22  8:31 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: John Snow, qemu-devel, qemu-block

Kevin Wolf <kwolf@redhat.com> writes:

> Am 21.05.2015 um 23:48 hat John Snow geschrieben:
>> 
>> 
>> On 05/20/2015 04:20 AM, Markus Armbruster wrote:
>> > John Snow <jsnow@redhat.com> writes:
>> > 
>> >> On 05/12/2015 04:06 PM, Eric Blake wrote:
>> >>> 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>
>> >>>
>> >>
>> >> I'm not actually sure whose tree this should go in. Markus's, perhaps?
>> >>
>> >> ("ping")
>> > 
>> > I guess the case for "Block layer core" (Kevin) is at least as strong as
>> > the case for "QAPI" (me).  Kevin, what do you think?
>
> I think bdrv_query_dirty_bitmaps() really belongs into block/qapi.c,
> which is yours anyway. So it's either you as the QAPI maintainer or you
> as the block submaintainer.

s/the block submaintainer/the newly minted block submaintainer/

> But if you think otherwise, I can consider it.
>
>> His silence says "Markus, can you please do it? I discovered today that
>> I don't care about this patch."
>
> I'm sorry, John, but you didn't CC me, you didn't CC qemu-block, you
> didn't CC anyone. I only had a chance to know about it since Wednesday
> when Markus forwarded it, and I'm not sitting there waiting for new
> patch emails because I'm bored. Rest assured, I have enough of them.
>
> And then the forwarded email didn't even quote the patch any more, so I
> couldn't just give a quick reply, but had to find the full email thread
> in a different folder.
>
> If you want to have patches applied quickly, make it easy for the
> maintainers. You did the exact opposite, so you have no reason to
> complain.

On the other hand, his "complaining" made me smile, which I appreciate :)

Don't worry, John, I'll take it through my tree.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH] qapi: add dirty bitmap status
  2015-05-12 20:06 ` Eric Blake
  2015-05-12 20:07   ` John Snow
  2015-05-19 21:18   ` John Snow
@ 2015-05-22  8:52   ` Markus Armbruster
  2015-05-22 17:27     ` John Snow
  2 siblings, 1 reply; 14+ messages in thread
From: Markus Armbruster @ 2015-05-22  8:52 UTC (permalink / raw)
  To: Eric Blake; +Cc: John Snow, qemu-devel

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>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH] qapi: add dirty bitmap status
  2015-05-22  8:31           ` Markus Armbruster
@ 2015-05-22 11:49             ` Kevin Wolf
  2015-05-22 17:26               ` John Snow
  0 siblings, 1 reply; 14+ messages in thread
From: Kevin Wolf @ 2015-05-22 11:49 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: John Snow, qemu-devel, qemu-block

Am 22.05.2015 um 10:31 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 21.05.2015 um 23:48 hat John Snow geschrieben:
> >> 
> >> 
> >> On 05/20/2015 04:20 AM, Markus Armbruster wrote:
> >> > John Snow <jsnow@redhat.com> writes:
> >> > 
> >> >> On 05/12/2015 04:06 PM, Eric Blake wrote:
> >> >>> 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>
> >> >>>
> >> >>
> >> >> I'm not actually sure whose tree this should go in. Markus's, perhaps?
> >> >>
> >> >> ("ping")
> >> > 
> >> > I guess the case for "Block layer core" (Kevin) is at least as strong as
> >> > the case for "QAPI" (me).  Kevin, what do you think?
> >
> > I think bdrv_query_dirty_bitmaps() really belongs into block/qapi.c,
> > which is yours anyway. So it's either you as the QAPI maintainer or you
> > as the block submaintainer.
> 
> s/the block submaintainer/the newly minted block submaintainer/
> 
> > But if you think otherwise, I can consider it.
> >
> >> His silence says "Markus, can you please do it? I discovered today that
> >> I don't care about this patch."
> >
> > I'm sorry, John, but you didn't CC me, you didn't CC qemu-block, you
> > didn't CC anyone. I only had a chance to know about it since Wednesday
> > when Markus forwarded it, and I'm not sitting there waiting for new
> > patch emails because I'm bored. Rest assured, I have enough of them.
> >
> > And then the forwarded email didn't even quote the patch any more, so I
> > couldn't just give a quick reply, but had to find the full email thread
> > in a different folder.
> >
> > If you want to have patches applied quickly, make it easy for the
> > maintainers. You did the exact opposite, so you have no reason to
> > complain.
> 
> On the other hand, his "complaining" made me smile, which I appreciate :)

Drom secht mr's jô em Guada. ;-)

I'm sorry if my reply reads a bit too harsh, it's not meant like that.
In fact, the way John phrased it made me smile, too - but that doesn't
change that it is a reproach for me, and looking at the timestamp I
didn't feel that it was entirely fair.

Kevin

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH] qapi: add dirty bitmap status
  2015-05-22  8:22         ` Kevin Wolf
  2015-05-22  8:31           ` Markus Armbruster
@ 2015-05-22 15:36           ` John Snow
  1 sibling, 0 replies; 14+ messages in thread
From: John Snow @ 2015-05-22 15:36 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: Markus Armbruster, qemu-block, qemu-devel



On 05/22/2015 04:22 AM, Kevin Wolf wrote:
> Am 21.05.2015 um 23:48 hat John Snow geschrieben:
>>
>>
>> On 05/20/2015 04:20 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> On 05/12/2015 04:06 PM, Eric Blake wrote:
>>>>> 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>
>>>>>
>>>>
>>>> I'm not actually sure whose tree this should go in. Markus's, perhaps?
>>>>
>>>> ("ping")
>>>
>>> I guess the case for "Block layer core" (Kevin) is at least as strong as
>>> the case for "QAPI" (me).  Kevin, what do you think?
> 
> I think bdrv_query_dirty_bitmaps() really belongs into block/qapi.c,
> which is yours anyway. So it's either you as the QAPI maintainer or you
> as the block submaintainer.
> 
> But if you think otherwise, I can consider it.
> 
>> His silence says "Markus, can you please do it? I discovered today that
>> I don't care about this patch."
> 
> I'm sorry, John, but you didn't CC me, you didn't CC qemu-block, you
> didn't CC anyone. I only had a chance to know about it since Wednesday
> when Markus forwarded it, and I'm not sitting there waiting for new
> patch emails because I'm bored. Rest assured, I have enough of them.
> 
> And then the forwarded email didn't even quote the patch any more, so I
> couldn't just give a quick reply, but had to find the full email thread
> in a different folder.
> 
> If you want to have patches applied quickly, make it easy for the
> maintainers. You did the exact opposite, so you have no reason to
> complain.
> 
> Kevin
> 

Sorry, I didn't mean it to come across that way. I wasn't complaining,
I just figured that it wasn't on your radar and decided to ping Markus
again.

My apologies for making it seem like I was being critical of your
response times, that wasn't my intent. I figured it got lost in the
shuffle and just wanted to prod Markus to take it into his QAPI tree.

This patch isn't /that/ important, so I promise I wasn't being
impatient, just a miss on being funny.

--js

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH] qapi: add dirty bitmap status
  2015-05-22 11:49             ` Kevin Wolf
@ 2015-05-22 17:26               ` John Snow
  0 siblings, 0 replies; 14+ messages in thread
From: John Snow @ 2015-05-22 17:26 UTC (permalink / raw)
  To: Kevin Wolf, Markus Armbruster; +Cc: qemu-devel, qemu-block



On 05/22/2015 07:49 AM, Kevin Wolf wrote:
> Am 22.05.2015 um 10:31 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>>> Am 21.05.2015 um 23:48 hat John Snow geschrieben:
>>>>
>>>>
>>>> On 05/20/2015 04:20 AM, Markus Armbruster wrote:
>>>>> John Snow <jsnow@redhat.com> writes:
>>>>>
>>>>>> On 05/12/2015 04:06 PM, Eric Blake wrote:
>>>>>>> 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>
>>>>>>>
>>>>>>
>>>>>> I'm not actually sure whose tree this should go in. Markus's, perhaps?
>>>>>>
>>>>>> ("ping")
>>>>>
>>>>> I guess the case for "Block layer core" (Kevin) is at least as strong as
>>>>> the case for "QAPI" (me).  Kevin, what do you think?
>>>
>>> I think bdrv_query_dirty_bitmaps() really belongs into block/qapi.c,
>>> which is yours anyway. So it's either you as the QAPI maintainer or you
>>> as the block submaintainer.
>>
>> s/the block submaintainer/the newly minted block submaintainer/
>>
>>> But if you think otherwise, I can consider it.
>>>
>>>> His silence says "Markus, can you please do it? I discovered today that
>>>> I don't care about this patch."
>>>
>>> I'm sorry, John, but you didn't CC me, you didn't CC qemu-block, you
>>> didn't CC anyone. I only had a chance to know about it since Wednesday
>>> when Markus forwarded it, and I'm not sitting there waiting for new
>>> patch emails because I'm bored. Rest assured, I have enough of them.
>>>
>>> And then the forwarded email didn't even quote the patch any more, so I
>>> couldn't just give a quick reply, but had to find the full email thread
>>> in a different folder.
>>>
>>> If you want to have patches applied quickly, make it easy for the
>>> maintainers. You did the exact opposite, so you have no reason to
>>> complain.
>>
>> On the other hand, his "complaining" made me smile, which I appreciate :)
> 
> Drom secht mr's jô em Guada. ;-)
> 
> I'm sorry if my reply reads a bit too harsh, it's not meant like that.
> In fact, the way John phrased it made me smile, too - but that doesn't
> change that it is a reproach for me, and looking at the timestamp I
> didn't feel that it was entirely fair.
> 
> Kevin
> 

Yes, sorry again. I will try to choose my jokes a little more carefully
in the future. I want to make people laugh, but not at the expense of
anyone's integrity.

--John Snow

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH] qapi: add dirty bitmap status
  2015-05-22  8:52   ` Markus Armbruster
@ 2015-05-22 17:27     ` John Snow
  2015-05-26  8:13       ` Markus Armbruster
  0 siblings, 1 reply; 14+ messages in thread
From: John Snow @ 2015-05-22 17:27 UTC (permalink / raw)
  To: Markus Armbruster, Eric Blake; +Cc: qemu-devel



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.

Thank you,
--John

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH] qapi: add dirty bitmap status
  2015-05-22 17:27     ` John Snow
@ 2015-05-26  8:13       ` Markus Armbruster
  0 siblings, 0 replies; 14+ messages in thread
From: Markus Armbruster @ 2015-05-26  8:13 UTC (permalink / raw)
  To: John Snow; +Cc: qemu-devel

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!

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2015-05-26  8:13 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-12 19:53 [Qemu-devel] [PATCH] qapi: add dirty bitmap status John Snow
2015-05-12 20:06 ` Eric Blake
2015-05-12 20:07   ` John Snow
2015-05-19 21:18   ` John Snow
2015-05-20  8:20     ` Markus Armbruster
2015-05-21 21:48       ` John Snow
2015-05-22  8:22         ` Kevin Wolf
2015-05-22  8:31           ` Markus Armbruster
2015-05-22 11:49             ` Kevin Wolf
2015-05-22 17:26               ` John Snow
2015-05-22 15:36           ` John Snow
2015-05-22  8:52   ` Markus Armbruster
2015-05-22 17:27     ` John Snow
2015-05-26  8:13       ` Markus Armbruster

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.