Patchwork [V5,03/13] block: add bdrv_can_read_snapshot() function

login
register
mail settings
Submitter Wayne Xia
Date Jan. 24, 2013, 2:57 a.m.
Message ID <1358996283-32441-4-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/215100/
State New
Headers show

Comments

Wayne Xia - Jan. 24, 2013, 2:57 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>
---
 block.c               |   19 +++++++++++++++++++
 include/block/block.h |    1 +
 2 files changed, 20 insertions(+), 0 deletions(-)
Eric Blake - Jan. 25, 2013, 6:11 p.m.
On 01/23/2013 07:57 PM, 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>
> ---
>  block.c               |   19 +++++++++++++++++++
>  include/block/block.h |    1 +
>  2 files changed, 20 insertions(+), 0 deletions(-)
> 

>  
> +/* return whether internal snapshot can be read on @bs */
> +int bdrv_can_read_snapshot(BlockDriverState *bs)
> +{

> +/* return whether internal snapshot can be write on @bs */
>  int bdrv_can_snapshot(BlockDriverState *bs)

I see you just copied existing code; but any reason why these functions
return int instead of bool?  Would that be worth a separate cleanup patch?

Reviewed-by: Eric Blake <eblake@redhat.com>
Wayne Xia - Jan. 28, 2013, 1:22 a.m.
于 2013-1-26 2:11, Eric Blake 写道:
> On 01/23/2013 07:57 PM, 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>
>> ---
>>   block.c               |   19 +++++++++++++++++++
>>   include/block/block.h |    1 +
>>   2 files changed, 20 insertions(+), 0 deletions(-)
>>
> 
>>   
>> +/* return whether internal snapshot can be read on @bs */
>> +int bdrv_can_read_snapshot(BlockDriverState *bs)
>> +{
> 
>> +/* return whether internal snapshot can be write on @bs */
>>   int bdrv_can_snapshot(BlockDriverState *bs)
> 
> I see you just copied existing code; but any reason why these functions
> return int instead of bool?  Would that be worth a separate cleanup patch?
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
  Just follow bdrv_can_snapshot() style, no special reason.:)
I think it is OK for a clean up patch following.
Kevin Wolf - Jan. 29, 2013, 12:37 p.m.
Am 25.01.2013 19:11, schrieb Eric Blake:
> On 01/23/2013 07:57 PM, 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>
>> ---
>>  block.c               |   19 +++++++++++++++++++
>>  include/block/block.h |    1 +
>>  2 files changed, 20 insertions(+), 0 deletions(-)
>>
> 
>>  
>> +/* return whether internal snapshot can be read on @bs */
>> +int bdrv_can_read_snapshot(BlockDriverState *bs)
>> +{
> 
>> +/* return whether internal snapshot can be write on @bs */
>>  int bdrv_can_snapshot(BlockDriverState *bs)
> 
> I see you just copied existing code; but any reason why these functions
> return int instead of bool?  Would that be worth a separate cleanup patch?

More importantly, you shouldn't copy code. Make both of them small
wrappers around a static helper functions that contains the existing code.

Kevin
Wayne Xia - Jan. 31, 2013, 9 a.m.
于 2013-1-29 20:37, Kevin Wolf 写道:
> Am 25.01.2013 19:11, schrieb Eric Blake:
>> On 01/23/2013 07:57 PM, 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>
>>> ---
>>>   block.c               |   19 +++++++++++++++++++
>>>   include/block/block.h |    1 +
>>>   2 files changed, 20 insertions(+), 0 deletions(-)
>>>
>>
>>>
>>> +/* return whether internal snapshot can be read on @bs */
>>> +int bdrv_can_read_snapshot(BlockDriverState *bs)
>>> +{
>>
>>> +/* return whether internal snapshot can be write on @bs */
>>>   int bdrv_can_snapshot(BlockDriverState *bs)
>>
>> I see you just copied existing code; but any reason why these functions
>> return int instead of bool?  Would that be worth a separate cleanup patch?
>
> More importantly, you shouldn't copy code. Make both of them small
> wrappers around a static helper functions that contains the existing code.
>
> Kevin
>
   I tried that before, but found that hard to wrapper around a common
function, because both need to recusively check some condition.

Patch

diff --git a/block.c b/block.c
index a86257d..934bb3f 100644
--- a/block.c
+++ b/block.c
@@ -3091,6 +3091,25 @@  bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag)
 /**************************************************************/
 /* handling of snapshots */
 
+/* return whether internal snapshot can be read on @bs */
+int bdrv_can_read_snapshot(BlockDriverState *bs)
+{
+    BlockDriver *drv = bs->drv;
+    if (!drv || !bdrv_is_inserted(bs)) {
+        return 0;
+    }
+
+    if (!drv->bdrv_snapshot_create) {
+        if (bs->file != NULL) {
+            return bdrv_can_read_snapshot(bs->file);
+        }
+        return 0;
+    }
+
+    return 1;
+}
+
+/* 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 0b84e9b..b4c1612 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -318,6 +318,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);
+int bdrv_can_read_snapshot(BlockDriverState *bs);
 int bdrv_can_snapshot(BlockDriverState *bs);
 int bdrv_is_snapshot(BlockDriverState *bs);
 BlockDriverState *bdrv_snapshots(void);