Patchwork [1/6] snapshot: export function in block.c

login
register
mail settings
Submitter Wayne Xia
Date Dec. 17, 2012, 6:25 a.m.
Message ID <1355725509-5429-2-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/206782/
State New
Headers show

Comments

Wayne Xia - Dec. 17, 2012, 6:25 a.m.
This patch moves bdrv_snapshotfind from savevm.c to block.c and export
it, also added bdrv_deappend in block.c.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block.c  |   30 ++++++++++++++++++++++++++++++
 block.h  |    3 +++
 savevm.c |   22 ----------------------
 3 files changed, 33 insertions(+), 22 deletions(-)
Juan Quintela - Dec. 21, 2012, 6:13 p.m.
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>   This patch moves bdrv_snapshotfind from savevm.c to block.c and export
> it, also added bdrv_deappend in block.c.

I think this patch can be splitted in two:
- new bdv_deappend
- move bdrv_snapshot_find
Wayne Xia - Dec. 25, 2012, 4:31 a.m.
于 2012-12-22 2:13, Juan Quintela 写道:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>    This patch moves bdrv_snapshotfind from savevm.c to block.c and export
>> it, also added bdrv_deappend in block.c.
> 
> I think this patch can be splitted in two:
> - new bdv_deappend
> - move bdrv_snapshot_find
> 
  OK.
Stefan Hajnoczi - Jan. 4, 2013, 2:49 p.m.
On Mon, Dec 17, 2012 at 02:25:04PM +0800, Wenchao Xia wrote:
>   This patch moves bdrv_snapshotfind from savevm.c to block.c and export
> it, also added bdrv_deappend in block.c.

I suggest naming the patch "block: export bdrv_snapshot_find()" because
it is more specific than "snapshot: export function in block.c".

> +/* revert the action */
> +void bdrv_deappend(BlockDriverState *bs_new, BlockDriverState *bs_top)
> +{
> +    bdrv_swap(bs_new, bs_top);
> +    /* this is enough? */
> +}

Please document this function in the same level of detail as
bdrv_append().  These functions are tricky and you must explain the
semantics so anyone else reading the code can safely use or modify this
function.

We also need to resolve the "this is enough?" comment.  I'll think it
through when I see how bdrv_deappend() is used later.

Stefan
Wayne Xia - Jan. 5, 2013, 8:26 a.m.
Thanks for reviewing carefully, this patch would be split to two
patches.

> On Mon, Dec 17, 2012 at 02:25:04PM +0800, Wenchao Xia wrote:
>>    This patch moves bdrv_snapshotfind from savevm.c to block.c and export
>> it, also added bdrv_deappend in block.c.
>
> I suggest naming the patch "block: export bdrv_snapshot_find()" because
> it is more specific than "snapshot: export function in block.c".
>
>> +/* revert the action */
>> +void bdrv_deappend(BlockDriverState *bs_new, BlockDriverState *bs_top)
>> +{
>> +    bdrv_swap(bs_new, bs_top);
>> +    /* this is enough? */
>> +}
>
> Please document this function in the same level of detail as
> bdrv_append().  These functions are tricky and you must explain the
> semantics so anyone else reading the code can safely use or modify this
> function.
>
> We also need to resolve the "this is enough?" comment.  I'll think it
> through when I see how bdrv_deappend() is used later.
>
> Stefan
>
Kevin Wolf - Jan. 7, 2013, 4:43 p.m.
Am 17.12.2012 07:25, schrieb Wenchao Xia:
>   This patch moves bdrv_snapshotfind from savevm.c to block.c and export
> it, also added bdrv_deappend in block.c.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

Should be two separate patches.

> ---
>  block.c  |   30 ++++++++++++++++++++++++++++++
>  block.h  |    3 +++
>  savevm.c |   22 ----------------------
>  3 files changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 0668c4b..61c7c6a 100644
> --- a/block.c
> +++ b/block.c
> @@ -1376,6 +1376,13 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
>              bs_new->drv ? bs_new->drv->format_name : "");
>  }
>  
> +/* revert the action */
> +void bdrv_deappend(BlockDriverState *bs_new, BlockDriverState *bs_top)
> +{
> +    bdrv_swap(bs_new, bs_top);
> +    /* this is enough? */
> +}

What will this be used for? Maybe it's better to introduce a function
simple as this only when it's actually used.

Kevin
Wayne Xia - Jan. 8, 2013, 2:25 a.m.
于 2013-1-8 0:43, Kevin Wolf 写道:
> Am 17.12.2012 07:25, schrieb Wenchao Xia:
>>    This patch moves bdrv_snapshotfind from savevm.c to block.c and export
>> it, also added bdrv_deappend in block.c.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>
> Should be two separate patches.
>
   OK, it have been split into two patches in V2.

>> ---
>>   block.c  |   30 ++++++++++++++++++++++++++++++
>>   block.h  |    3 +++
>>   savevm.c |   22 ----------------------
>>   3 files changed, 33 insertions(+), 22 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 0668c4b..61c7c6a 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1376,6 +1376,13 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
>>               bs_new->drv ? bs_new->drv->format_name : "");
>>   }
>>
>> +/* revert the action */
>> +void bdrv_deappend(BlockDriverState *bs_new, BlockDriverState *bs_top)
>> +{
>> +    bdrv_swap(bs_new, bs_top);
>> +    /* this is enough? */
>> +}
>
> What will this be used for? Maybe it's better to introduce a function
> simple as this only when it's actually used.
>
   If bdrv_append is called before, this function should revert it, so
new file created at the top of the backing file chain is dropped.

> Kevin
>
Kevin Wolf - Jan. 8, 2013, 10:37 a.m.
Am 08.01.2013 03:25, schrieb Wenchao Xia:
> 于 2013-1-8 0:43, Kevin Wolf 写道:
>> Am 17.12.2012 07:25, schrieb Wenchao Xia:
>>>    This patch moves bdrv_snapshotfind from savevm.c to block.c and export
>>> it, also added bdrv_deappend in block.c.
>>>
>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>
>> Should be two separate patches.
>>
>    OK, it have been split into two patches in V2.
> 
>>> ---
>>>   block.c  |   30 ++++++++++++++++++++++++++++++
>>>   block.h  |    3 +++
>>>   savevm.c |   22 ----------------------
>>>   3 files changed, 33 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 0668c4b..61c7c6a 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -1376,6 +1376,13 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
>>>               bs_new->drv ? bs_new->drv->format_name : "");
>>>   }
>>>
>>> +/* revert the action */
>>> +void bdrv_deappend(BlockDriverState *bs_new, BlockDriverState *bs_top)
>>> +{
>>> +    bdrv_swap(bs_new, bs_top);
>>> +    /* this is enough? */
>>> +}
>>
>> What will this be used for? Maybe it's better to introduce a function
>> simple as this only when it's actually used.
>>
>    If bdrv_append is called before, this function should revert it, so
> new file created at the top of the backing file chain is dropped.

Yes, I understand that. I meant which functionality/feature will make
use of the function?

Kevin
Wayne Xia - Jan. 9, 2013, 4:32 a.m.
于 2013-1-8 18:37, Kevin Wolf 写道:
> Am 08.01.2013 03:25, schrieb Wenchao Xia:
>> 于 2013-1-8 0:43, Kevin Wolf 写道:
>>> Am 17.12.2012 07:25, schrieb Wenchao Xia:
>>>>     This patch moves bdrv_snapshotfind from savevm.c to block.c and export
>>>> it, also added bdrv_deappend in block.c.
>>>>
>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>
>>> Should be two separate patches.
>>>
>>     OK, it have been split into two patches in V2.
>>
>>>> ---
>>>>    block.c  |   30 ++++++++++++++++++++++++++++++
>>>>    block.h  |    3 +++
>>>>    savevm.c |   22 ----------------------
>>>>    3 files changed, 33 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/block.c b/block.c
>>>> index 0668c4b..61c7c6a 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -1376,6 +1376,13 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
>>>>                bs_new->drv ? bs_new->drv->format_name : "");
>>>>    }
>>>>
>>>> +/* revert the action */
>>>> +void bdrv_deappend(BlockDriverState *bs_new, BlockDriverState *bs_top)
>>>> +{
>>>> +    bdrv_swap(bs_new, bs_top);
>>>> +    /* this is enough? */
>>>> +}
>>>
>>> What will this be used for? Maybe it's better to introduce a function
>>> simple as this only when it's actually used.
>>>
>>     If bdrv_append is called before, this function should revert it, so
>> new file created at the top of the backing file chain is dropped.
> 
> Yes, I understand that. I meant which functionality/feature will make
> use of the function?
> 
> Kevin
> 
  I have used it in group snapshot transaction canceling callback, but
currently it will never be executed because updating step never
fail, so it can be removed but leaves a comments that need add in
if an updating function which may fail....

Patch

diff --git a/block.c b/block.c
index 0668c4b..61c7c6a 100644
--- a/block.c
+++ b/block.c
@@ -1376,6 +1376,13 @@  void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
             bs_new->drv ? bs_new->drv->format_name : "");
 }
 
+/* revert the action */
+void bdrv_deappend(BlockDriverState *bs_new, BlockDriverState *bs_top)
+{
+    bdrv_swap(bs_new, bs_top);
+    /* this is enough? */
+}
+
 void bdrv_delete(BlockDriverState *bs)
 {
     assert(!bs->dev);
@@ -3210,6 +3217,29 @@  int bdrv_snapshot_load_tmp(BlockDriverState *bs,
     return -ENOTSUP;
 }
 
+int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
+                       const char *name)
+{
+    QEMUSnapshotInfo *sn_tab, *sn;
+    int nb_sns, i, ret;
+
+    ret = -ENOENT;
+    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+    if (nb_sns < 0) {
+        return ret;
+    }
+    for (i = 0; i < nb_sns; i++) {
+        sn = &sn_tab[i];
+        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
+            *sn_info = *sn;
+            ret = 0;
+            break;
+        }
+    }
+    g_free(sn_tab);
+    return ret;
+}
+
 /* backing_file can either be relative, or absolute, or a protocol.  If it is
  * relative, it must be relative to the chain.  So, passing in bs->filename
  * from a BDS as backing_file should not be done, as that may be relative to
diff --git a/block.h b/block.h
index 893448a..2322b13 100644
--- a/block.h
+++ b/block.h
@@ -130,6 +130,7 @@  BlockDriverState *bdrv_new(const char *device_name);
 void bdrv_make_anon(BlockDriverState *bs);
 void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
 void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
+void bdrv_deappend(BlockDriverState *bs_new, BlockDriverState *bs_top);
 void bdrv_delete(BlockDriverState *bs);
 int bdrv_parse_cache_flags(const char *mode, int *flags);
 int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags);
@@ -330,6 +331,8 @@  int bdrv_snapshot_list(BlockDriverState *bs,
 int bdrv_snapshot_load_tmp(BlockDriverState *bs,
                            const char *snapshot_name);
 char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn);
+int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
+                       const char *name);
 
 char *get_human_readable_size(char *buf, int buf_size, int64_t size);
 int path_is_absolute(const char *path);
diff --git a/savevm.c b/savevm.c
index 5d04d59..c027529 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2061,28 +2061,6 @@  out:
     return ret;
 }
 
-static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
-                              const char *name)
-{
-    QEMUSnapshotInfo *sn_tab, *sn;
-    int nb_sns, i, ret;
-
-    ret = -ENOENT;
-    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
-    if (nb_sns < 0)
-        return ret;
-    for(i = 0; i < nb_sns; i++) {
-        sn = &sn_tab[i];
-        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
-            *sn_info = *sn;
-            ret = 0;
-            break;
-        }
-    }
-    g_free(sn_tab);
-    return ret;
-}
-
 /*
  * Deletes snapshots of a given name in all opened images.
  */