| Message ID | 1358996283-32441-4-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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>
于 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.
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
于 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.
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);
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(-)