Patchwork [V7,02/14] block: add bdrv_can_read_snapshot() function

login
register
mail settings
Submitter Wayne Xia
Date Feb. 26, 2013, 10:40 a.m.
Message ID <1361875228-15769-3-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/223193/
State New
Headers show

Comments

Wayne Xia - Feb. 26, 2013, 10:40 a.m.
Compared to bdrv_can_snapshot(), this function return whether
bs* is ready to read snapshot info from instead of write. If yes,
caller can then query snapshot information, but taking snapshot
is not always possible for that *bs may be read only.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c               |   19 +++++++++++++++++++
 include/block/block.h |    1 +
 2 files changed, 20 insertions(+), 0 deletions(-)
Markus Armbruster - Feb. 27, 2013, 1:58 p.m.
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:

>   Compared to bdrv_can_snapshot(), this function return whether
> bs* is ready to read snapshot info from instead of write. If yes,
> caller can then query snapshot information, but taking snapshot
> is not always possible for that *bs may be read only.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block.c               |   19 +++++++++++++++++++
>  include/block/block.h |    1 +
>  2 files changed, 20 insertions(+), 0 deletions(-)
>
> diff --git a/block.c b/block.c
> index 50dab8e..19c2d7b 100644
> --- a/block.c
> +++ b/block.c
> @@ -3058,6 +3058,25 @@ bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag)
>  /**************************************************************/
>  /* handling of snapshots */
>  
> +/* return whether internal snapshot can be read on @bs */
> +bool bdrv_can_read_snapshot(BlockDriverState *bs)
> +{
> +    BlockDriver *drv = bs->drv;
> +    if (!drv || !bdrv_is_inserted(bs)) {
> +        return false;
> +    }
> +
> +    if (!drv->bdrv_snapshot_create) {
> +        if (bs->file != NULL) {
> +            return bdrv_can_read_snapshot(bs->file);
> +        }
> +        return false;
> +    }
> +
> +    return true;
> +}

Looks like

bdrv_can_read_snapshot(bs) && !bdrv_is_read_only(bs) == bdrv_can_snapshot(bs)

Correct?

> +
> +/* return whether internal snapshot can be write on @bs */

"be written".

Or more succinctly:

/* Can @bs take a snapshot?  */

>  int bdrv_can_snapshot(BlockDriverState *bs)
>  {
>      BlockDriver *drv = bs->drv;
> diff --git a/include/block/block.h b/include/block/block.h
> index 5c3b911..4c48052 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -321,6 +321,7 @@ void bdrv_get_full_backing_filename(BlockDriverState *bs,
>                                      char *dest, size_t sz);
>  BlockInfo *bdrv_query_info(BlockDriverState *s);
>  BlockStats *bdrv_query_stats(const BlockDriverState *bs);
> +bool bdrv_can_read_snapshot(BlockDriverState *bs);
>  int bdrv_can_snapshot(BlockDriverState *bs);
>  int bdrv_is_snapshot(BlockDriverState *bs);
>  BlockDriverState *bdrv_snapshots(void);
Wayne Xia - Feb. 28, 2013, 1:39 a.m.
于 2013-2-27 21:58, Markus Armbruster 写道:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> 
>>    Compared to bdrv_can_snapshot(), this function return whether
>> bs* is ready to read snapshot info from instead of write. If yes,
>> caller can then query snapshot information, but taking snapshot
>> is not always possible for that *bs may be read only.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   block.c               |   19 +++++++++++++++++++
>>   include/block/block.h |    1 +
>>   2 files changed, 20 insertions(+), 0 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 50dab8e..19c2d7b 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3058,6 +3058,25 @@ bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag)
>>   /**************************************************************/
>>   /* handling of snapshots */
>>   
>> +/* return whether internal snapshot can be read on @bs */
>> +bool bdrv_can_read_snapshot(BlockDriverState *bs)
>> +{
>> +    BlockDriver *drv = bs->drv;
>> +    if (!drv || !bdrv_is_inserted(bs)) {
>> +        return false;
>> +    }
>> +
>> +    if (!drv->bdrv_snapshot_create) {
>> +        if (bs->file != NULL) {
>> +            return bdrv_can_read_snapshot(bs->file);
>> +        }
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
> 
> Looks like
> 
> bdrv_can_read_snapshot(bs) && !bdrv_is_read_only(bs) == bdrv_can_snapshot(bs)
> 
> Correct?
> 
  not equal, it is tricky for that recusively check is done inside
bdrv_can_snapshot(bs), that is why I did not use a common internal
static function.

>> +
>> +/* return whether internal snapshot can be write on @bs */
> 
> "be written".
> 
> Or more succinctly:
> 
> /* Can @bs take a snapshot?  */
> 
  OK, this seems better.

>>   int bdrv_can_snapshot(BlockDriverState *bs)
>>   {
>>       BlockDriver *drv = bs->drv;
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 5c3b911..4c48052 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -321,6 +321,7 @@ void bdrv_get_full_backing_filename(BlockDriverState *bs,
>>                                       char *dest, size_t sz);
>>   BlockInfo *bdrv_query_info(BlockDriverState *s);
>>   BlockStats *bdrv_query_stats(const BlockDriverState *bs);
>> +bool bdrv_can_read_snapshot(BlockDriverState *bs);
>>   int bdrv_can_snapshot(BlockDriverState *bs);
>>   int bdrv_is_snapshot(BlockDriverState *bs);
>>   BlockDriverState *bdrv_snapshots(void);
>
Stefan Hajnoczi - Feb. 28, 2013, 3:20 p.m.
On Tue, Feb 26, 2013 at 06:40:16PM +0800, Wenchao Xia wrote:
>   Compared to bdrv_can_snapshot(), this function return whether
> bs* is ready to read snapshot info from instead of write. If yes,
> caller can then query snapshot information, but taking snapshot
> is not always possible for that *bs may be read only.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block.c               |   19 +++++++++++++++++++
>  include/block/block.h |    1 +
>  2 files changed, 20 insertions(+), 0 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 50dab8e..19c2d7b 100644
> --- a/block.c
> +++ b/block.c
> @@ -3058,6 +3058,25 @@ bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag)
>  /**************************************************************/
>  /* handling of snapshots */
>  
> +/* return whether internal snapshot can be read on @bs */
> +bool bdrv_can_read_snapshot(BlockDriverState *bs)

Which operations are included in "read a snapshot"?

Candidate list:
 * bdrv_snapshot_load_tmp()
 * bdrv_snapshot_list()
 * bdrv_snapshot_dump()
 * bdrv_load_vmstate()
 * others?

Please document the meaning of this function so that others can
use/modify it correctly in the future.

Stefan
Wayne Xia - March 1, 2013, 1:39 a.m.
于 2013-2-28 23:20, Stefan Hajnoczi 写道:
> On Tue, Feb 26, 2013 at 06:40:16PM +0800, Wenchao Xia wrote:
>>    Compared to bdrv_can_snapshot(), this function return whether
>> bs* is ready to read snapshot info from instead of write. If yes,
>> caller can then query snapshot information, but taking snapshot
>> is not always possible for that *bs may be read only.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   block.c               |   19 +++++++++++++++++++
>>   include/block/block.h |    1 +
>>   2 files changed, 20 insertions(+), 0 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 50dab8e..19c2d7b 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3058,6 +3058,25 @@ bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag)
>>   /**************************************************************/
>>   /* handling of snapshots */
>>
>> +/* return whether internal snapshot can be read on @bs */
>> +bool bdrv_can_read_snapshot(BlockDriverState *bs)
>
> Which operations are included in "read a snapshot"?
>
> Candidate list:
>   * bdrv_snapshot_load_tmp()
>   * bdrv_snapshot_list()
>   * bdrv_snapshot_dump()
>   * bdrv_load_vmstate()
>   * others?
>
> Please document the meaning of this function so that others can
> use/modify it correctly in the future.
>
> Stefan
>

OK.

Patch

diff --git a/block.c b/block.c
index 50dab8e..19c2d7b 100644
--- a/block.c
+++ b/block.c
@@ -3058,6 +3058,25 @@  bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag)
 /**************************************************************/
 /* handling of snapshots */
 
+/* return whether internal snapshot can be read on @bs */
+bool bdrv_can_read_snapshot(BlockDriverState *bs)
+{
+    BlockDriver *drv = bs->drv;
+    if (!drv || !bdrv_is_inserted(bs)) {
+        return false;
+    }
+
+    if (!drv->bdrv_snapshot_create) {
+        if (bs->file != NULL) {
+            return bdrv_can_read_snapshot(bs->file);
+        }
+        return false;
+    }
+
+    return true;
+}
+
+/* return whether internal snapshot can be write on @bs */
 int bdrv_can_snapshot(BlockDriverState *bs)
 {
     BlockDriver *drv = bs->drv;
diff --git a/include/block/block.h b/include/block/block.h
index 5c3b911..4c48052 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -321,6 +321,7 @@  void bdrv_get_full_backing_filename(BlockDriverState *bs,
                                     char *dest, size_t sz);
 BlockInfo *bdrv_query_info(BlockDriverState *s);
 BlockStats *bdrv_query_stats(const BlockDriverState *bs);
+bool bdrv_can_read_snapshot(BlockDriverState *bs);
 int bdrv_can_snapshot(BlockDriverState *bs);
 int bdrv_is_snapshot(BlockDriverState *bs);
 BlockDriverState *bdrv_snapshots(void);