On 12/07/2017 04:43 AM, Vladimir Sementsov-Ogievskiy wrote: > 07.12.2017 01:56, John Snow wrote: >> >> On 11/30/2017 11:47 AM, Vladimir Sementsov-Ogievskiy wrote: >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> >>> --- >>> block/qcow2.h | 7 +++++-- >>> block/qcow2-refcount.c | 12 ++++++++++++ >>> block/qcow2.c | 6 ++++++ >>> 3 files changed, 23 insertions(+), 2 deletions(-) >>> >>> diff --git a/block/qcow2.h b/block/qcow2.h >>> index 6f0ff15dd0..8f226a3609 100644 >>> --- a/block/qcow2.h >>> +++ b/block/qcow2.h >>> @@ -98,6 +98,7 @@ >>> #define QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE >>> "overlap-check.snapshot-table" >>> #define QCOW2_OPT_OVERLAP_INACTIVE_L1 "overlap-check.inactive-l1" >>> #define QCOW2_OPT_OVERLAP_INACTIVE_L2 "overlap-check.inactive-l2" >>> +#define QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY >>> "overlap-check.bitmap-directory" >>> #define QCOW2_OPT_CACHE_SIZE "cache-size" >>> #define QCOW2_OPT_L2_CACHE_SIZE "l2-cache-size" >>> #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size" >>> @@ -406,8 +407,9 @@ typedef enum QCow2MetadataOverlap { >>> QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5, >>> QCOW2_OL_INACTIVE_L1_BITNR = 6, >>> QCOW2_OL_INACTIVE_L2_BITNR = 7, >>> + QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8, >>> - QCOW2_OL_MAX_BITNR = 8, >>> + QCOW2_OL_MAX_BITNR = 9, >>> QCOW2_OL_NONE = 0, >>> QCOW2_OL_MAIN_HEADER = (1 << QCOW2_OL_MAIN_HEADER_BITNR), >>> @@ -420,12 +422,13 @@ typedef enum QCow2MetadataOverlap { >>> /* NOTE: Checking overlaps with inactive L2 tables will result >>> in bdrv >>> * reads. */ >>> QCOW2_OL_INACTIVE_L2 = (1 << QCOW2_OL_INACTIVE_L2_BITNR), >>> + QCOW2_OL_BITMAP_DIRECTORY = (1 << QCOW2_OL_BITMAP_DIRECTORY_BITNR), >>> } QCow2MetadataOverlap; >>> /* Perform all overlap checks which can be done in constant time */ >>> #define QCOW2_OL_CONSTANT \ >>> (QCOW2_OL_MAIN_HEADER | QCOW2_OL_ACTIVE_L1 | >>> QCOW2_OL_REFCOUNT_TABLE | \ >>> - QCOW2_OL_SNAPSHOT_TABLE) >>> + QCOW2_OL_SNAPSHOT_TABLE | QCOW2_OL_BITMAP_DIRECTORY) >>> /* Perform all overlap checks which don't require disk access */ >>> #define QCOW2_OL_CACHED \ >>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c >>> index 3de1ab51ba..a7a2703f26 100644 >>> --- a/block/qcow2-refcount.c >>> +++ b/block/qcow2-refcount.c >>> @@ -2585,6 +2585,18 @@ int >>> qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t >>> offset, >>> } >>> } >>> + if ((chk & QCOW2_OL_BITMAP_DIRECTORY) && >>> + (s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS)) >>> + { >>> + /* update_ext_header_and_dir_in_place firstly drop autoclear >>> flag, >>> + * so it will not fail */ >>> + if (overlaps_with(s->bitmap_directory_offset, >>> + s->bitmap_directory_size)) >>> + { >>> + return QCOW2_OL_BITMAP_DIRECTORY; >>> + } >>> + } >>> + >> Isn't the purpose of this function to test if a given offset conflicts >> with known regions of the file? I don't see you actually utilize the >> 'offset' parameter here, but maybe I don't understand what you're trying >> to accomplish. > > #define overlaps_with(ofs, sz) \ > ranges_overlap(offset, size, ofs, sz) > > I've just copied one of similar blocks in qcow2_check_metadata_overlap() >
Sigh, I didn't realize that was a macro. I don't really like lowercase macros, but you didn't add it. Reviewed-by: John Snow <js...@redhat.com> Carry on...