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 <vsement...@virtuozzo.com>
>> ---
>> 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
