On 12/5/19 3:09 PM, Eric Blake wrote:
> On 12/5/19 1:30 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Here is double bug:
>>
>> First, return error but not set errp. This may lead to:
>> qmp block-dirty-bitmap-remove may report success when actually failed
>>
>> block-dirty-bitmap-remove used in a transaction will crash, as
>> qmp_transaction will think that it returned success and will cal
>
> call
>
>> block_dirty_bitmap_remove_commit which will crash, as state->bitmap is
>> NULL
>>
>> Second (like in anecdote), this case is not an error at all. As it is
>> documented in the comment above bdrv_co_remove_persistent_dirty_bitmap
>> definition, absence of bitmap is not an error, and similar case handled
>> at start of qcow2_co_remove_persistent_dirty_bitmap, it returns 0 when
>> there is no bitmaps at all..
>
> double .
>
>>
>> But when there are some bitmaps, but not the requested one, it return
>> error with errp unset.
>>
>> Fix that.
>>
>> Fixes: b56a1e31759b750
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@xxxxxxxxxxxxx>
>> ---
>>
>> Hi all!
>>
>> Ohm, suddenly we faced this bug. It's a regression in 4.2. I'm very
>> sorry for introducing it, and it sad that it's found so late..
>>
>> Personally, I think that this one worth rc5, as it makes new bitmap
>> interfaces unusable. But the decision is yours.
>>
>> Last minute edit: hmm, actually, transaction action introduced in
>> 4.2, so crash is not a regression, only broken block-dirty-bitmap-remove
>> command is a regression... Maybe it's OK for stable.
>
> Libvirt REALLY wants to use transaction bitmap management (and require
> qemu 4.2) for its incremental backup patches that Peter is almost ready
> to merge in. I'm trying to ascertain:
>
> When exactly can this bug hit? Can you give a sequence of QMP commands
> that will trigger it for easy reproduction? Are there workarounds (such
> as checking that a bitmap exists prior to attempting to remove it)? If
> it does NOT get fixed in rc5, how will libvirt be able to probe whether
> the fix is in place?
>
It looks like:
- You need to have at least one bitmap
- You need to use transactional remove
- you need to misspell the bitmap name
- The remove will fail (return -EINVAL) but doesn't set errp
- The caller chokes on incomplete information, state->bitmap is NULL
>>
>> block/qcow2-bitmap.c | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 8abaf632fc..c6c8ebbe89 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -1469,8 +1469,10 @@ int coroutine_fn
>> qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> Qcow2BitmapList *bm_list;
>> if (s->nb_bitmaps == 0) {
>> - /* Absence of the bitmap is not an error: see explanation above
>> - * bdrv_remove_persistent_dirty_bitmap() definition. */
>> + /*
>> + * Absence of the bitmap is not an error: see explanation above
>> + * bdrv_co_remove_persistent_dirty_bitmap() definition.
>> + */
>> return 0;
>> }
>> @@ -1485,7 +1487,8 @@ int coroutine_fn
>> qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> bm = find_bitmap_by_name(bm_list, name);
>> if (bm == NULL) {
>> - ret = -EINVAL;
>> + /* Absence of the bitmap is not an error, see above. */
>> + ret = 0;
>> goto out;
>> }
>>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
