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

2013-02-16 Thread Blue Swirl 
On Fri, Feb 15, 2013 at 9:35 AM, Kevin Wolf kw...@redhat.com wrote:
 On Thu, Feb 14, 2013 at 09:40:22PM +, 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

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

2013-02-15 Thread Kevin Wolf 
On Thu, Feb 14, 2013 at 09:40:22PM +, 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.

Kevin

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

2013-02-14 Thread Kevin Wolf 
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.

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

Kevin

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

2013-02-14 Thread Blue Swirl 
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) {



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

 Kevin

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

2013-02-13 Thread Kevin Wolf 
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)
+{
+*bytes = 0;
+ret = 0;
+goto out;
+}
+
 /* We keep all QCOW_OFLAG_COPIED clusters */
 keep_clusters =
 count_contiguous_clusters(nb_clusters, s-cluster_size,
@@ -880,19 +886,22 @@ static int handle_copied(BlockDriverState *bs, uint64_t 
guest_offset,
 
 ret = 1;
 } else {
-cluster_offset = 0;
 ret = 0;
 }
 
-cluster_offset = L2E_OFFSET_MASK;
-*host_offset = cluster_offset;
-
 /* Cleanup */
+out:
 pret = qcow2_cache_put(bs, s-l2_table_cache, (void**) l2_table);
 if (pret  0) {
 return pret;
 }
 
+/* Only return a host offset if we actually made progress. Otherwise we
+ * would make requirements for handle_alloc() that it can't fulfill */
+if (ret) {
+*host_offset = cluster_offset  L2E_OFFSET_MASK;
+}
+
 return ret;
 }
 
@@ -1162,7 +1171,6 @@ again:
 
 /*
  * 2. Count contiguous COPIED clusters.
- *TODO: Consider cluster_offset if set in step 1c.
  */
 ret = handle_copied(bs, offset, cluster_offset, cur_bytes, m);
 if (ret  0) {
@@ -1175,6 +1183,8 @@ again:
 if (!*host_offset) {
 *host_offset = cluster_offset;
 }
+} else if (cur_bytes == 0) {
+goto done;
 } else {
 keep_clusters = 0;
 }
-- 
1.7.6.5

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

2013-02-13 Thread 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.

 +*bytes = 0;
 +ret = 0;
 +goto out;
 +}
 +
  /* We keep all QCOW_OFLAG_COPIED clusters */
  keep_clusters =
  count_contiguous_clusters(nb_clusters, s-cluster_size,
 @@ -880,19 +886,22 @@ static int handle_copied(BlockDriverState *bs, uint64_t 
 guest_offset,

  ret = 1;
  } else {
 -cluster_offset = 0;
  ret = 0;
  }

 -cluster_offset = L2E_OFFSET_MASK;
 -*host_offset = cluster_offset;
 -
  /* Cleanup */
 +out:
  pret = qcow2_cache_put(bs, s-l2_table_cache, (void**) l2_table);
  if (pret  0) {
  return pret;
  }

 +/* Only return a host offset if we actually made progress. Otherwise we
 + * would make requirements for handle_alloc() that it can't fulfill */
 +if (ret) {
 +*host_offset = cluster_offset  L2E_OFFSET_MASK;
 +}
 +
  return ret;
  }

 @@ -1162,7 +1171,6 @@ again:

  /*
   * 2. Count contiguous COPIED clusters.
 - *TODO: Consider cluster_offset if set in step 1c.
   */
  ret = handle_copied(bs, offset, cluster_offset, cur_bytes, m);
  if (ret  0) {
 @@ -1175,6 +1183,8 @@ again:
  if (!*host_offset) {
  *host_offset = cluster_offset;
  }
 +} else if (cur_bytes == 0) {
 +goto done;
  } else {
  keep_clusters = 0;
  }
 --
 1.7.6.5