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

[Blue Swirl]

On Fri, Feb 15, 2013 at 9:35 AM, Kevin Wolf <address@hidden> wrote:
> On Thu, Feb 14, 2013 at 09:40:22PM +0000, Blue Swirl wrote:
>> On Thu, Feb 14, 2013 at 9:40 AM, Kevin Wolf <address@hidden> wrote:
>> > 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.
>>
>> Perhaps the problem is that the condition is long, it could be
>> rewritten in this style:
>> bool has_host_offset = *host_offset != 0;
>> bool offset_matches = (cluster_offset & L2E_OFFSET_MASK) != *host_offset;
>> if (has_host_offset && offset_matches) {
>
> I consider the usefulness of this about the same as adding code in order to
> silence gcc warnings. Just that a complaining gcc breaks the build whereas a
> complaining Blue Swirl doesn't. But I'll do it here.

Maybe I complain too much. I think the booleans could actually make
the logic clearer to someone not familiar with the code.

>
> Kevin