Re: [Qemu-devel] [PATCH] qcow2 resize with snapshot

[zhangzhiming]

hi, thanks for the review and the format of code changed.
have a good day!

zhangzhiming
address@hidden

Signed-off-by: zhangzhiming <address@hidden>
---
 block.c                |    4 ++--
 block/qcow2-cluster.c  |    4 ++--
 block/qcow2-snapshot.c |    5 ++---
 block/qcow2.c          |    4 ++--
 4 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index 729f820..b6f2004 100644
--- a/block.c
+++ b/block.c
@@ -2643,9 +2643,9 @@ int bdrv_apply_snapshot(BlockDriverState *bs, const char 
*snapshot_id,
     if (ret < 0) {
         return ret;
     }
-    bdrv_dirty_bitmap_truncate(bs); /* void return */
+    bdrv_dirty_bitmap_truncate(bs);
     if (bs->blk) {
-        blk_dev_resize_cb(bs->blk); /* void return too */
+        blk_dev_resize_cb(bs->blk);
     }
     return ret;
 }
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index e4c5c05..f921fd8 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -37,14 +37,14 @@ int shrink_l1_table(BlockDriverState *bs, int64_t 
new_l1_size)
     int64_t old_l1_size = s->l1_size;
     s->l1_size = new_l1_size;
     int ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
-                                                     s->l1_size, 1);
+                                             s->l1_size, 1);
     if (ret < 0) {
         return ret;
     }
 
     s->l1_size = old_l1_size;
     ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
-                                                     s->l1_size, -1);
+                                         s->l1_size, -1);
     if (ret < 0) {
         return ret;
     }
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 9c77096..1ed0e18 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -562,12 +562,11 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char 
*snapshot_id)
      * Now update the in-memory L1 table to be in sync with the on-disk one. We
      * need to do this even if updating refcounts failed.
      */
-    memset(s->l1_table, 0, s->l1_size*sizeof(uint64_t));
-    for(i = 0;i < sn->l1_size; i++) {
+    memset(s->l1_table, 0, s->l1_size * sizeof(uint64_t));
+    for (i = 0; i < sn->l1_size; i++) {
         s->l1_table[i] = be64_to_cpu(sn_l1_table[i]);
     }
 
-
     if (ret < 0) {
         goto fail;
     }
diff --git a/block/qcow2.c b/block/qcow2.c
index 70ec890..58b53e1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2503,14 +2503,14 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t 
offset)
 
     bool v3_truncate = (s->qcow_version == 3);
 
-    /* cannot proceed if image has snapshots and qcow_version is not 3*/
+    /* cannot proceed if image has snapshots and qcow_version is not 3 */
     if (!v3_truncate && s->nb_snapshots) {
         error_report("Can't resize an image which has snapshots and "
                      "qcow_version is not 3");
         return -ENOTSUP;
     }
 
-    /* shrinking is supported from version 3*/
+    /* shrinking is supported from version 3 */
     if (!v3_truncate && offset < bs->total_sectors * 512) {
         error_report("qcow2 doesn't support shrinking images yet while"
                      " qcow_version is not 3");
-- 
1.7.1


> On Jun 1, 2016, at 12:50 AM, Eric Blake <address@hidden> wrote:
> 
> On 05/27/2016 02:14 AM, zhangzhiming wrote:
>> Hi, i modified my code for qcow2 resize, and delete some code related to 
>> qemu monitor.
>> 
>> and thanks for the review.
>> 
>> zhangzhiming
>> address@hidden <mailto:address@hidden>
> 
> Still missing a Signed-off-by designation, so it can't be applied as-is.
> 
>> 
>> ---
>> block.c                  |   19 +++++++++++++++++++
>> block/qcow2-cluster.c    |   29 +++++++++++++++++++++++++++++
>> block/qcow2-snapshot.c   |   34 ++++++++++++++++++++++++----------
>> block/qcow2.c            |   29 ++++++++++++++++++++---------
>> block/qcow2.h            |    1 +
>> include/block/snapshot.h |    1 +
>> 6 files changed, 94 insertions(+), 19 deletions(-)
>> 
>> diff --git a/block.c b/block.c
>> index 18a497f..729f820 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2631,6 +2631,25 @@ int bdrv_truncate(BlockDriverState *bs, int64_t 
>> offset)
>>     return ret;
>> }
>> 
>> +int bdrv_apply_snapshot(BlockDriverState *bs, const char *snapshot_id,
>> +                        uint64_t snapshot_size)
>> +{
>> +    int ret = bdrv_snapshot_goto(bs, snapshot_id);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +
>> +    ret = refresh_total_sectors(bs, snapshot_size >> BDRV_SECTOR_BITS);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +    bdrv_dirty_bitmap_truncate(bs); /* void return */
>> +    if (bs->blk) {
>> +        blk_dev_resize_cb(bs->blk); /* void return too */
> 
> The comments don't add anything here.
> 
>> +    }
>> +    return ret;
>> +}
>> +
>> /**
>>  * Length of a allocated file in bytes. Sparse files are counted by actual
>>  * allocated space. Return < 0 if error or unknown.
>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>> index 31ecc10..e4c5c05 100644
>> --- a/block/qcow2-cluster.c
>> +++ b/block/qcow2-cluster.c
>> @@ -31,6 +31,35 @@
>> #include "block/qcow2.h"
>> #include "trace.h"
>> 
>> +int shrink_l1_table(BlockDriverState *bs, int64_t new_l1_size)
>> +{
>> +    BDRVQcow2State *s = bs->opaque;
>> +    int64_t old_l1_size = s->l1_size;
>> +    s->l1_size = new_l1_size;
>> +    int ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
>> +                                                     s->l1_size, 1);
> 
> Indentation is off.
> 
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +
>> +    s->l1_size = old_l1_size;
>> +    ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
>> +                                                     s->l1_size, -1);
> 
> and again.
> 
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +    s->l1_size = new_l1_size;
>> +
> 
>> +++ b/block/qcow2-snapshot.c
> 
>> @@ -552,10 +562,12 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const 
>> char *snapshot_id)
>>      * Now update the in-memory L1 table to be in sync with the on-disk one. 
>> We
>>      * need to do this even if updating refcounts failed.
>>      */
>> -    for(i = 0;i < s->l1_size; i++) {
>> +    memset(s->l1_table, 0, s->l1_size*sizeof(uint64_t));
>> +    for(i = 0;i < sn->l1_size; i++) {
> 
> As long as you are touching this, use the preferred spacing:
> 
> for (i = 0; i < sn->l1_size; i++) {
> 
>>         s->l1_table[i] = be64_to_cpu(sn_l1_table[i]);
>>     }
>> 
>> +
>>     if (ret < 0) {
> 
> Why the added blank line?
> 
> 
>> +++ b/block/qcow2.c
>> @@ -2501,22 +2501,33 @@ static int qcow2_truncate(BlockDriverState *bs, 
>> int64_t offset)
>>         return -EINVAL;
>>     }
>> 
>> -    /* cannot proceed if image has snapshots */
>> -    if (s->nb_snapshots) {
>> -        error_report("Can't resize an image which has snapshots");
>> +    bool v3_truncate = (s->qcow_version == 3);
>> +
>> +    /* cannot proceed if image has snapshots and qcow_version is not 3*/
> 
> Space before */
> 
>> +    if (!v3_truncate && s->nb_snapshots) {
>> +        error_report("Can't resize an image which has snapshots and "
>> +                     "qcow_version is not 3");
>>         return -ENOTSUP;
>>     }
>> 
>> -    /* shrinking is currently not supported */
>> -    if (offset < bs->total_sectors * 512) {
>> -        error_report("qcow2 doesn't support shrinking images yet");
>> +    /* shrinking is supported from version 3*/
> 
> and again
> 
>> +    if (!v3_truncate && offset < bs->total_sectors * 512) {
>> +        error_report("qcow2 doesn't support shrinking images yet while"
>> +                     " qcow_version is not 3");
>>         return -ENOTSUP;
>>     }
> 
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>