On 26.10.19 23:25, Alberto Garcia wrote: > The L2 bitmap needs to be updated after each write to indicate what > new subclusters are now allocated. > > This needs to happen even if the cluster was already allocated and the > L2 entry was otherwise valid. > > Signed-off-by: Alberto Garcia <be...@igalia.com> > --- > block/qcow2-cluster.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index fb6cf8df17..acb7226e03 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -980,6 +980,22 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, > QCowL2Meta *m) > > set_l2_entry(s, l2_slice, l2_index + i, QCOW_OFLAG_COPIED | > (cluster_offset + (i << s->cluster_bits))); > + > + /* Update bitmap with the subclusters that were just written */ > + if (has_subclusters(s)) { > + uint64_t written_from = m->cow_start.offset; > + uint64_t written_to = m->cow_end.offset + m->cow_end.nb_bytes;
It’s a bit strange to make these uint64_t when all the fields queried are of type unsigned, but more on that below. > + uint64_t l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index + i); > + int sc; > + for (sc = 0; sc < s->subclusters_per_cluster; sc++) { > + uint64_t sc_off = i * s->cluster_size + sc * > s->subcluster_size; It’s weird to give this a uint64_t type when all the variables in the term are of type int. I’m not sure whether it can overflow. handle_alloc() limits everything to INT_MAX, but I’m not sure about handle_copied(). Speaking of handle_copied(); both elements of Qcow2COWRegion are of type unsigned. handle_copied() doesn’t look like it takes any precautions to limit the range to even UINT_MAX (and it should probably limit it to INT_MAX). Max > + if (sc_off >= written_from && sc_off < written_to) { > + l2_bitmap |= QCOW_OFLAG_SUB_ALLOC(sc); > + l2_bitmap &= ~QCOW_OFLAG_SUB_ZERO(sc); > + } > + } > + set_l2_bitmap(s, l2_slice, l2_index + i, l2_bitmap); > + } > } > > >
signature.asc
Description: OpenPGP digital signature