| Message ID | 1358996283-32441-10-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Am 24.01.2013 03:57, schrieb Wenchao Xia: > This patch move it from savevm.c to block.c and export it. To make > it clear about id and name in searching, the API was changed a bit > to distinguish them. Caller can choose to search by id or name now. > > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> Please keep code motion and semantic changes separate. One change per patch. > --- > block.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++ > include/block/block.h | 2 + > savevm.c | 32 ++++------------------------- > 3 files changed, 58 insertions(+), 27 deletions(-) > > diff --git a/block.c b/block.c > index 02c74dc..6631d9a 100644 > --- a/block.c > +++ b/block.c > @@ -3395,6 +3395,57 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs, > return -ENOTSUP; > } > > +/* > + * Try find an internal snapshot with @id or @name, @id have higher priority > + * in searching. > + * @bs block device to search on, must not be NULL. > + * @sn_info snapshot information to be filled in, must not be NULL. > + * @id snapshot id to search with, can be NULL. > + * @name snapshot name to search with, can be NULL. > + * returns 0 and @sn_info is filled with related information if found, > + * otherwise it returns negative value, -ENOENT. > + */ > +int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, > + const char *id, 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; Why don't you return the real error? > + } > + > + /* search by id */ > + if (id) { > + for (i = 0; i < nb_sns; i++) { > + sn = &sn_tab[i]; > + if (!strcmp(sn->id_str, id)) { > + *sn_info = *sn; > + ret = 0; > + goto out; > + } > + } > + } > + > + /* search by name */ > + if (name) { > + for (i = 0; i < nb_sns; i++) { > + sn = &sn_tab[i]; > + if (!strcmp(sn->name, name)) { > + *sn_info = *sn; > + ret = 0; > + goto out; > + } > + } > + } What's the motivation for duplicating the code instead of keeping it an strcmp() || strcmp() like in the original place? > + > + out: > + 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/include/block/block.h b/include/block/block.h > index 9838c48..1e7f4a1 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -340,6 +340,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 *id, 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 304d1ef..dccaece 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -2029,28 +2029,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. > */ > @@ -2063,7 +2041,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name) > bs = NULL; > while ((bs = bdrv_next(bs))) { > if (bdrv_can_snapshot(bs) && > - bdrv_snapshot_find(bs, snapshot, name) >= 0) > + bdrv_snapshot_find(bs, snapshot, name, name) >= 0) > { > ret = bdrv_snapshot_delete(bs, name); > if (ret < 0) { > @@ -2123,7 +2101,7 @@ void do_savevm(Monitor *mon, const QDict *qdict) > sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock); > > if (name) { > - ret = bdrv_snapshot_find(bs, old_sn, name); > + ret = bdrv_snapshot_find(bs, old_sn, name, name); > if (ret >= 0) { > pstrcpy(sn->name, sizeof(sn->name), old_sn->name); > pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str); > @@ -2214,7 +2192,7 @@ int load_vmstate(const char *name) > } > > /* Don't even try to load empty VM states */ > - ret = bdrv_snapshot_find(bs_vm_state, &sn, name); > + ret = bdrv_snapshot_find(bs_vm_state, &sn, name, name); > if (ret < 0) { > return ret; > } else if (sn.vm_state_size == 0) { > @@ -2238,7 +2216,7 @@ int load_vmstate(const char *name) > return -ENOTSUP; > } > > - ret = bdrv_snapshot_find(bs, &sn, name); > + ret = bdrv_snapshot_find(bs, &sn, name, name); > if (ret < 0) { > error_report("Device '%s' does not have the requested snapshot '%s'", > bdrv_get_device_name(bs), name); > @@ -2344,7 +2322,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict) > > while ((bs1 = bdrv_next(bs1))) { > if (bdrv_can_snapshot(bs1) && bs1 != bs) { > - ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str); > + ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL); Here we searched for name or ID previously, now we search only for the ID. We will not miss any snapshot because we only search the snapshot that we already have and it will always the ID that it has. We could find duplicates before using the name, so this is in fact a bug fix and should be a separate patch. But then, I don't understand the whole logic in do_info_snapshots(). Why does list all snapshots and then check for each snapshot if it's really there by indirectly calling bdrv_snapshot_list() again? Doesn't make a whole lot of sense to me. Kevin
δΊ 2013-1-29 21:15, Kevin Wolf ει: > Am 24.01.2013 03:57, schrieb Wenchao Xia: >> This patch move it from savevm.c to block.c and export it. To make >> it clear about id and name in searching, the API was changed a bit >> to distinguish them. Caller can choose to search by id or name now. >> >> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > > Please keep code motion and semantic changes separate. One change per patch. > OK. >> --- >> block.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++ >> include/block/block.h | 2 + >> savevm.c | 32 ++++------------------------- >> 3 files changed, 58 insertions(+), 27 deletions(-) >> >> diff --git a/block.c b/block.c >> index 02c74dc..6631d9a 100644 >> --- a/block.c >> +++ b/block.c >> @@ -3395,6 +3395,57 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs, >> return -ENOTSUP; >> } >> >> +/* >> + * Try find an internal snapshot with @id or @name, @id have higher priority >> + * in searching. >> + * @bs block device to search on, must not be NULL. >> + * @sn_info snapshot information to be filled in, must not be NULL. >> + * @id snapshot id to search with, can be NULL. >> + * @name snapshot name to search with, can be NULL. >> + * returns 0 and @sn_info is filled with related information if found, >> + * otherwise it returns negative value, -ENOENT. >> + */ >> +int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, >> + const char *id, 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; > > Why don't you return the real error? > OK. >> + } >> + >> + /* search by id */ >> + if (id) { >> + for (i = 0; i < nb_sns; i++) { >> + sn = &sn_tab[i]; >> + if (!strcmp(sn->id_str, id)) { >> + *sn_info = *sn; >> + ret = 0; >> + goto out; >> + } >> + } >> + } >> + >> + /* search by name */ >> + if (name) { >> + for (i = 0; i < nb_sns; i++) { >> + sn = &sn_tab[i]; >> + if (!strcmp(sn->name, name)) { >> + *sn_info = *sn; >> + ret = 0; >> + goto out; >> + } >> + } >> + } > > What's the motivation for duplicating the code instead of keeping it an > strcmp() || strcmp() like in the original place? > The logic is slightly different: original code will try compare name before id comparation complete, make the caller harder to use. >> + >> + out: >> + 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/include/block/block.h b/include/block/block.h >> index 9838c48..1e7f4a1 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -340,6 +340,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 *id, 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 304d1ef..dccaece 100644 >> --- a/savevm.c >> +++ b/savevm.c >> @@ -2029,28 +2029,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. >> */ >> @@ -2063,7 +2041,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name) >> bs = NULL; >> while ((bs = bdrv_next(bs))) { >> if (bdrv_can_snapshot(bs) && >> - bdrv_snapshot_find(bs, snapshot, name) >= 0) >> + bdrv_snapshot_find(bs, snapshot, name, name) >= 0) >> { >> ret = bdrv_snapshot_delete(bs, name); >> if (ret < 0) { >> @@ -2123,7 +2101,7 @@ void do_savevm(Monitor *mon, const QDict *qdict) >> sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock); >> >> if (name) { >> - ret = bdrv_snapshot_find(bs, old_sn, name); >> + ret = bdrv_snapshot_find(bs, old_sn, name, name); >> if (ret >= 0) { >> pstrcpy(sn->name, sizeof(sn->name), old_sn->name); >> pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str); >> @@ -2214,7 +2192,7 @@ int load_vmstate(const char *name) >> } >> >> /* Don't even try to load empty VM states */ >> - ret = bdrv_snapshot_find(bs_vm_state, &sn, name); >> + ret = bdrv_snapshot_find(bs_vm_state, &sn, name, name); >> if (ret < 0) { >> return ret; >> } else if (sn.vm_state_size == 0) { >> @@ -2238,7 +2216,7 @@ int load_vmstate(const char *name) >> return -ENOTSUP; >> } >> >> - ret = bdrv_snapshot_find(bs, &sn, name); >> + ret = bdrv_snapshot_find(bs, &sn, name, name); >> if (ret < 0) { >> error_report("Device '%s' does not have the requested snapshot '%s'", >> bdrv_get_device_name(bs), name); >> @@ -2344,7 +2322,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict) >> >> while ((bs1 = bdrv_next(bs1))) { >> if (bdrv_can_snapshot(bs1) && bs1 != bs) { >> - ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str); >> + ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL); > > Here we searched for name or ID previously, now we search only for the > ID. We will not miss any snapshot because we only search the snapshot > that we already have and it will always the ID that it has. > > We could find duplicates before using the name, so this is in fact a bug > fix and should be a separate patch. > Yes, but since the API was changed, so I guess this would make build fail in this patch. > But then, I don't understand the whole logic in do_info_snapshots(). Why > does list all snapshots and then check for each snapshot if it's really > there by indirectly calling bdrv_snapshot_list() again? Doesn't make a > whole lot of sense to me. > That logic is trying to filter out the internal snapshots not exist on every block device, so I introduced a filter callback to do it more clear.:) > Kevin >
diff --git a/block.c b/block.c index 02c74dc..6631d9a 100644 --- a/block.c +++ b/block.c @@ -3395,6 +3395,57 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs, return -ENOTSUP; } +/* + * Try find an internal snapshot with @id or @name, @id have higher priority + * in searching. + * @bs block device to search on, must not be NULL. + * @sn_info snapshot information to be filled in, must not be NULL. + * @id snapshot id to search with, can be NULL. + * @name snapshot name to search with, can be NULL. + * returns 0 and @sn_info is filled with related information if found, + * otherwise it returns negative value, -ENOENT. + */ +int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, + const char *id, 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; + } + + /* search by id */ + if (id) { + for (i = 0; i < nb_sns; i++) { + sn = &sn_tab[i]; + if (!strcmp(sn->id_str, id)) { + *sn_info = *sn; + ret = 0; + goto out; + } + } + } + + /* search by name */ + if (name) { + for (i = 0; i < nb_sns; i++) { + sn = &sn_tab[i]; + if (!strcmp(sn->name, name)) { + *sn_info = *sn; + ret = 0; + goto out; + } + } + } + + out: + 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/include/block/block.h b/include/block/block.h index 9838c48..1e7f4a1 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -340,6 +340,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 *id, 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 304d1ef..dccaece 100644 --- a/savevm.c +++ b/savevm.c @@ -2029,28 +2029,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. */ @@ -2063,7 +2041,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name) bs = NULL; while ((bs = bdrv_next(bs))) { if (bdrv_can_snapshot(bs) && - bdrv_snapshot_find(bs, snapshot, name) >= 0) + bdrv_snapshot_find(bs, snapshot, name, name) >= 0) { ret = bdrv_snapshot_delete(bs, name); if (ret < 0) { @@ -2123,7 +2101,7 @@ void do_savevm(Monitor *mon, const QDict *qdict) sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock); if (name) { - ret = bdrv_snapshot_find(bs, old_sn, name); + ret = bdrv_snapshot_find(bs, old_sn, name, name); if (ret >= 0) { pstrcpy(sn->name, sizeof(sn->name), old_sn->name); pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str); @@ -2214,7 +2192,7 @@ int load_vmstate(const char *name) } /* Don't even try to load empty VM states */ - ret = bdrv_snapshot_find(bs_vm_state, &sn, name); + ret = bdrv_snapshot_find(bs_vm_state, &sn, name, name); if (ret < 0) { return ret; } else if (sn.vm_state_size == 0) { @@ -2238,7 +2216,7 @@ int load_vmstate(const char *name) return -ENOTSUP; } - ret = bdrv_snapshot_find(bs, &sn, name); + ret = bdrv_snapshot_find(bs, &sn, name, name); if (ret < 0) { error_report("Device '%s' does not have the requested snapshot '%s'", bdrv_get_device_name(bs), name); @@ -2344,7 +2322,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict) while ((bs1 = bdrv_next(bs1))) { if (bdrv_can_snapshot(bs1) && bs1 != bs) { - ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str); + ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL); if (ret < 0) { available = 0; break;
This patch move it from savevm.c to block.c and export it. To make it clear about id and name in searching, the API was changed a bit to distinguish them. Caller can choose to search by id or name now. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- block.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++ include/block/block.h | 2 + savevm.c | 32 ++++------------------------- 3 files changed, 58 insertions(+), 27 deletions(-)