Re: [Qemu-devel] [PATCH v4 09/10] block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw

[Vladimir Sementsov-Ogievskiy]

27.09.2019 2:21, John Snow wrote:
> 
> 
> On 8/7/19 10:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>> - Correct check for write access to file child, and in correct place
>>    (only if we want to write).
>> - Support reopen rw -> rw (which will be used in following commit),
>>    for example, !bdrv_dirty_bitmap_readonly() is not a corruption if
>>    bitmap is marked IN_USE in the image.
>> - Consider unexpected bitmap as a corruption and check other
>>    combinations of in-image and in-RAM bitmaps.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>>   block/qcow2-bitmap.c | 56 +++++++++++++++++++++++++++++---------------
>>   1 file changed, 37 insertions(+), 19 deletions(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index a636dc50ca..e276a95154 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -1108,18 +1108,14 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, 
>> Error **errp)
>>       Qcow2BitmapList *bm_list;
>>       Qcow2Bitmap *bm;
>>       GSList *ro_dirty_bitmaps = NULL;
>> -    int ret = 0;
>> +    int ret = -EINVAL;
>> +    bool need_header_update = false;
>>   
>>       if (s->nb_bitmaps == 0) {
>>           /* No bitmaps - nothing to do */
>>           return 0;
>>       }
>>   
>> -    if (!can_write(bs)) {
>> -        error_setg(errp, "Can't write to the image on reopening bitmaps 
>> rw");
>> -        return -EINVAL;
>> -    }
>> -
>>       bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>>                                  s->bitmap_directory_size, errp);
>>       if (bm_list == NULL) {
>> @@ -1128,32 +1124,54 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, 
>> Error **errp)
>>   
>>       QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>>           BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
>> -        if (bitmap == NULL) {
>> -            continue;
>> -        }
>>   
>> -        if (!bdrv_dirty_bitmap_readonly(bitmap)) {
>> -            error_setg(errp, "Bitmap %s was loaded prior to rw-reopen, but 
>> was "
>> -                       "not marked as readonly. This is a bug, something 
>> went "
>> -                       "wrong. All of the bitmaps may be corrupted", 
>> bm->name);
>> -            ret = -EINVAL;
>> +        if (!bitmap) {
>> +            error_setg(errp, "Unexpected bitmap '%s' in the image '%s'",
>> +                       bm->name, bs->filename);
>>               goto out;
>>           }
>>   
> 
> I think you can actually drop the definite article, because the image
> name is the specifier.
> 
> "Unexpected bitmap '%s' in image '%s'" is sufficient.
> 
>> -        bm->flags |= BME_FLAG_IN_USE;
>> -        ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
>> +        if (!(bm->flags & BME_FLAG_IN_USE)) {
>> +            if (!bdrv_dirty_bitmap_readonly(bitmap)) {
>> +                error_setg(errp, "Corruption: bitmap '%s' is not marked 
>> IN_USE "
>> +                           "in the image '%s' and not marked readonly in 
>> RAM",
>> +                           bm->name, bs->filename);
>> +                goto out;
>> +            }
>> +            if (bdrv_dirty_bitmap_inconsistent(bitmap)) {
>> +                error_setg(errp, "Corruption: bitmap '%s' is inconsistent 
>> but "
>> +                           "is not marked IN_USE in the image '%s'", 
>> bm->name,
>> +                           bs->filename);
>> +                goto out;
>> +            }
> 
> We support RW --> RW now, but what happens if something is marked IN_USE
> on RO --> RW? It's not obvious from this function alone why that's safe
> to ignore.

If bitmap is IN_USE in the image, it's always safe to ignore it, as it's marked 
as
invalid anyway.

Consider RO->RW with IN_USE.

if corresponding BdrvDirtyBitmap is inconsistent, it's OK.

If not, how can that be? I can't imagine. But we have correct bitmap in RAM, 
should
we mark it inconsistent, because of bitmap in image? I don't know.


> 
>> +
>> +            bm->flags |= BME_FLAG_IN_USE;
>> +            need_header_update = true;
>> +        }
>> +
>> +        if (bdrv_dirty_bitmap_readonly(bitmap)) {
>> +            ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
>> +        }
>>       }
>>   
>> -    if (ro_dirty_bitmaps != NULL) {
>> +    if (need_header_update) {
>> +        if (!can_write(bs->file->bs) || !(bs->file->perm & BLK_PERM_WRITE)) 
>> {
>> +            error_setg(errp, "Failed to reopen bitmaps rw: no write access "
>> +                       "the protocol file");
>> +            goto out;
>> +        }
>> +
>>           /* in_use flags must be updated */
>>           ret = update_ext_header_and_dir_in_place(bs, bm_list);
>>           if (ret < 0) {
>> -            error_setg_errno(errp, -ret, "Can't update bitmap directory");
>> +            error_setg_errno(errp, -ret, "Cannot update bitmap directory");
>>               goto out;
>>           }
>> -        g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false);
>>       }
>>   
>> +    g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false);
>> +    ret = 0;
>> +
>>   out:
>>       g_slist_free(ro_dirty_bitmaps);
>>       bitmap_list_free(bm_list);
>>
> 
> Seems OK otherwise, but I just have that one doubt.
> 


-- 
Best regards,
Vladimir