On Fri, Feb 15, 2013 at 9:35 AM, Kevin Wolf <kw...@redhat.com> 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 <kw...@redhat.com> wrote: >> > Am 13.02.2013 22:17, schrieb Blue Swirl: >> >> On Wed, Feb 13, 2013 at 1:22 PM, Kevin Wolf <kw...@redhat.com> wrote: >> >>> Look only for clusters that start at a given physical offset. >> >>> >> >>> Signed-off-by: Kevin Wolf <kw...@redhat.com> >> >>> --- >> >>> 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