| Message ID | 1355725509-5429-2-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
于 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.
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
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 >
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
于 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 >
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
于 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....
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. */
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(-)