Re: [Qemu-devel] [PATCH V5 03/13] block: add bdrv_can_read_snapshot() function
于 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
---
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.
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [PATCH V5 03/13] block: add bdrv_can_read_snapshot() function
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
>> ---
>> 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
Re: [Qemu-devel] [PATCH V5 03/13] block: add bdrv_can_read_snapshot() function
于 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
>> ---
>> 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
>
Just follow bdrv_can_snapshot() style, no special reason.:)
I think it is OK for a clean up patch following.
--
Best Regards
Wenchao Xia
Re: [Qemu-devel] [PATCH V5 03/13] block: add bdrv_can_read_snapshot() function
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
> ---
> 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
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH V5 03/13] block: add bdrv_can_read_snapshot() function
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
---
block.c | 19 +++
include/block/block.h |1 +
2 files changed, 20 insertions(+), 0 deletions(-)
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);
--
1.7.1
