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
