Il 12/06/2012 16:21, Kevin Wolf ha scritto:
>>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>>> index 9aee9fc..763b724 100644
>>> --- a/block/qcow2-cluster.c
>>> +++ b/block/qcow2-cluster.c
>>> @@ -640,11 +640,10 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs,
>>> QCowL2Meta *m)
>>> }
>>>
>>> if (m->nb_available & (s->cluster_sectors - 1)) {
>>> - uint64_t end = m->nb_available & ~(uint64_t)(s->cluster_sectors -
>>> 1);
>>> cow = true;
>>> qemu_co_mutex_unlock(&s->lock);
>>> - ret = copy_sectors(bs, start_sect + end, cluster_offset + (end <<
>>> 9),
>>> - m->nb_available - end, s->cluster_sectors);
>>> + ret = copy_sectors(bs, start_sect, cluster_offset,
>>> + m->nb_available, s->cluster_sectors);
>>
>> Do you need to add end to s->cluster_sectors too, so that "start_sect +
>> n_end" and "n_end - n_start" remain the same?
>
> You mean because n_end is now relative to start_sect instead of
> start_sect + end, right?
Yes. Or more simply, because I was expecting no other uses of
start_sect, cluster_offset and n_start after reading your commit message. :)
> I thought about it and I find this code is a bit confusing, but I think
> you're right that I need to replace n_end as well because it would be
> wrong for an allocating request than spans multiple clusters. I think
> this one should be right, would you agree?
>
> ret = copy_sectors(bs, start_sect, cluster_offset,
> m->nb_available, align_offset(m->nb_available, s->cluster_sectors));
The obvious expression would be
s->cluster_sectors
+ (m->nb_available & ~(uint64_t)(s->cluster_sectors - 1))
which is a bit different from align_offset. If m->nb_available is
already aligned, it returns the *next* aligned value rather than
m->nb_available itself.
So the equivalent expression using align_offset would be this one:
align_offset(m->nb_available+1, s->cluster_sectors)
Paolo
