Re: [Qemu-devel] [PATCH v6 08/10] qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}

[Paolo Bonzini]

On 24/11/2014 10:46, Max Reitz wrote:
> On 2014-11-24 at 10:41, Paolo Bonzini wrote:
>>
>> On 24/11/2014 09:35, Max Reitz wrote:
>>>>> But what if the dirty bitmap was enabled before so that this enabling
>>>>> transaction was supposed to be a no-op?
>>>> Maybe it's not a problem: The only case in which the enable/disable
>>>> operation will fail is if it cannot find the device or bitmap -- in
>>>> which case, the abort operation isn't going to be able to affect
>>>> anything either.
>>> Well, it's part of a transaction. As far as I understand it, one groups
>>> several operations in one transaction and if any fails, all are aborted.
>>> Therefore, the "enable the dirty bitmap" operation can trivially succeed
>>> (because it was enabled before), but some different operation may fail,
>>> which then results in invocation of this abort function.
>> Would it work to do the actual enabling/disabling in the commit
>> function, since it cannot fail?
> 
> Too easy. :-)
> 
> This patch could then no longer reuse qmp_block_dirty_bitmap_enable(),
> but reimplementing it here should be fine (it's short enough).

Ah, right:

prepare:

+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    bitmap = bdrv_find_dirty_bitmap(bs, name);
+    if (!bitmap) {
+        error_setg(errp, "Dirty bitmap not found: %s", name);
+        return;
+    }

commit:

+    bdrv_enable_dirty_bitmap(bs, bitmap);

But qmp_block_dirty_bitmap_enable can reuse qmp_transaction instead! :)

Paolo