[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] qcow2: add overlap check for b
From: |
John Snow |
Subject: |
Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] qcow2: add overlap check for bitmap directory |
Date: |
Thu, 7 Dec 2017 12:03:55 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 |
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 <address@hidden>
>>> ---
>>> 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 <address@hidden>
Carry on...