Re: [Qemu-devel] [RFC PATCH v2 13/23] qcow2: handle_copied(): Implement non-zero host_offset

[Kevin Wolf]

Am 13.02.2013 22:17, schrieb Blue Swirl:
> On Wed, Feb 13, 2013 at 1:22 PM, Kevin Wolf <address@hidden> wrote:
>> Look only for clusters that start at a given physical offset.
>>
>> Signed-off-by: Kevin Wolf <address@hidden>
>> ---
>>  block/qcow2-cluster.c |   26 ++++++++++++++++++--------
>>  1 files changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>> index 5ce2c88..90fe36c 100644
>> --- a/block/qcow2-cluster.c
>> +++ b/block/qcow2-cluster.c
>> @@ -827,8 +827,6 @@ static int handle_dependencies(BlockDriverState *bs, 
>> uint64_t guest_offset,
>>   *          the length of the area that can be written to.
>>   *
>>   *  -errno: in error cases
>> - *
>> - * TODO Make non-zero host_offset behave like describe above
>>   */
>>  static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
>>      uint64_t *host_offset, uint64_t *bytes, QCowL2Meta **m)
>> @@ -843,7 +841,6 @@ static int handle_copied(BlockDriverState *bs, uint64_t 
>> guest_offset,
>>
>>      trace_qcow2_handle_copied(qemu_coroutine_self(), guest_offset, 
>> *host_offset,
>>                                *bytes);
>> -    assert(*host_offset == 0);
>>
>>      /*
>>       * Calculate the number of clusters to look for. We stop at L2 table
>> @@ -867,6 +864,15 @@ static int handle_copied(BlockDriverState *bs, uint64_t 
>> guest_offset,
>>      if (qcow2_get_cluster_type(cluster_offset) == QCOW2_CLUSTER_NORMAL
>>          && (cluster_offset & QCOW_OFLAG_COPIED))
>>      {
>> +        /* If a specific host_offset is required, check it */
>> +        if (*host_offset != 0
>> +            && (cluster_offset & L2E_OFFSET_MASK) != *host_offset)
>> +        {
> 
> Braces should cuddle with the previous line.

Can we get rid of this rule for multiline ifs? Having the
second/third/... line of the condition and the first line of the then
branch with no clear separation severely hurts readability in my opinion.

> 
>> +            *bytes = 0;
>> +            ret = 0;
>> +            goto out;
>> +        }
>> +
>>          /* We keep all QCOW_OFLAG_COPIED clusters */
>>          keep_clusters =
>>              count_contiguous_clusters(nb_clusters, s->cluster_size,

Kevin