On Thu, Jan 23, 2014 at 06:13:48PM +0100, Benoît Canet wrote:
> Le Thursday 23 Jan 2014 à 11:04:06 (+0800), Hu Tao a écrit :
> > When cluster size is big enough it can lead offset overflow
>
> Maybe "it can lead to an offset overflow in"
> > in qcow2_alloc_clusters_at(). This patch fixes it.
Sure.
> >
> > The allocation each time is stopped at L2 table boundary
> "The allocation is stopped each time at"
Sure.
>
> > (see handle_alloc()), so the possible maximum bytes could be
> >
> > 2^(cluster_bits - 3 + cluster_bits)
>
> So if understand cluster_bits - 3 is used to compute the number of entry by L2
> and the additional cluster_bits is to take into account each clusters
> referenced
> by the L2 entries.
Exactly. This is clearer than just one calculation. Do you mind I
put the sentence in commit message?
> It makes sense.
>
> >
> > so int is safe for cluster_bits<=17, unsafe otherwise.
> >
> > Reviewed-by: Max Reitz <mre...@redhat.com>
> > Signed-off-by: Hu Tao <hu...@cn.fujitsu.com>
> > ---
> > block/qcow2-refcount.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> > index c974abe..8712d8b 100644
> > --- a/block/qcow2-refcount.c
> > +++ b/block/qcow2-refcount.c
> > @@ -676,7 +676,13 @@ int qcow2_alloc_clusters_at(BlockDriverState *bs,
> > uint64_t offset,
> > BDRVQcowState *s = bs->opaque;
> > uint64_t cluster_index;
> > uint64_t old_free_cluster_index;
> > - int i, refcount, ret;
> > + uint64_t i;
> > + int refcount, ret;
> > +
>
>
> > + assert(nb_clusters >= 0);
> > + if (nb_clusters == 0) {
> > + return 0;
> > + }
> ^
> Adding a a line on the commit message about this assertion and chunk of code
> would be helpful.
How about
Add an assert to guard the comparation between signed and unsigned
?
>
> Best regards
>
> Benoît
>
> >
> > /* Check how many clusters there are free */
> > cluster_index = offset >> s->cluster_bits;
> > --
> > 1.8.5.2.229.g4448466
> >
> >
