On 2018-02-01 11:40, Alberto Garcia wrote: > On Wed 31 Jan 2018 09:11:48 PM CET, Max Reitz wrote: >> On 2018-01-26 15:59, Alberto Garcia wrote: >>> After the previous patch we're now always using l2_load() in >>> get_cluster_table() regardless of whether a new L2 table has to be >>> allocated or not. >>> >>> This patch refactors that part of the code to use one single l2_load() >>> call. >>> >>> Signed-off-by: Alberto Garcia <be...@igalia.com> >>> --- >>> block/qcow2-cluster.c | 21 +++++++-------------- >>> 1 file changed, 7 insertions(+), 14 deletions(-) >>> >>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c >>> index 2a53c1dc5f..0c0cab85e8 100644 >>> --- a/block/qcow2-cluster.c >>> +++ b/block/qcow2-cluster.c >>> @@ -695,15 +695,7 @@ static int get_cluster_table(BlockDriverState *bs, >>> uint64_t offset, >>> return -EIO; >>> } >>> >>> - /* seek the l2 table of the given l2 offset */ >>> - >>> - if (s->l1_table[l1_index] & QCOW_OFLAG_COPIED) { >>> - /* load the l2 table in memory */ >>> - ret = l2_load(bs, offset, l2_offset, &l2_table); >>> - if (ret < 0) { >>> - return ret; >>> - } >>> - } else { >>> + if (!(s->l1_table[l1_index] & QCOW_OFLAG_COPIED)) { >>> /* First allocate a new L2 table (and do COW if needed) */ >>> ret = l2_allocate(bs, l1_index); >>> if (ret < 0) { >>> @@ -719,11 +711,12 @@ static int get_cluster_table(BlockDriverState *bs, >>> uint64_t offset, >>> /* Get the offset of the newly-allocated l2 table */ >>> l2_offset = s->l1_table[l1_index] & L1E_OFFSET_MASK; >>> assert(offset_into_cluster(s, l2_offset) == 0); >>> - /* Load the l2 table in memory */ >>> - ret = l2_load(bs, offset, l2_offset, &l2_table); >>> - if (ret < 0) { >>> - return ret; >>> - } >>> + } >>> + >>> + /* load the l2 table in memory */ >>> + ret = l2_load(bs, offset, l2_offset, &l2_table); >>> + if (ret < 0) { >>> + return ret; >>> } >> >> I'd pull the l2_offset assignment (and alignment check) down below the >> QCOW_OFLAG_COPIED check, saving us another two lines (!!1!) which we >> could then spend on an >> >> assert(s->l1_table[l1_index] & QCOW_OFLAG_COPIED); > > I'm not sure if I'm following you. We need to set l2_offset before and > after allocating a new L2 table. > > Before, because we need the offset of the old table (if there was one) > in order to decrease its refcount. > > And after, because we need the offset of the new table in order to load > it.
Ah, right, yeah... Well, too bad. Then I probably can't ask for the assert() that badly. Reviewed-by: Max Reitz <mre...@redhat.com>
signature.asc
Description: OpenPGP digital signature