| Message ID | 17626585faf740878394cb2cbdc733cfb0c535e8.1366127809.git.phrdina@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/16/2013 10:05 AM, Pavel Hrdina wrote: > Finding snapshot by a name which could also be an id isn't best way > how to do it. There will be rewrite of savevm, loadvm and delvm to > improve the behavior of these commands. The savevm and loadvm will > have their own patch series. > > Now bdrv_snapshot_find takes more parameters. The name parameter will > be matched only against the name of the snapshot and the same applies > to id parameter. > > There is one exception. If you set the last parameter, the name parameter > will be matched against the name or the id of a snapshot. This exception > is only for backward compatibility for other commands and it will be > dropped after all commands will be rewritten. Fair enough, even if it is a bit of churn on the signature and callers. > > We only need to know if that snapshot exists or not. We don't care > about any error message. If snapshot exists it returns 1 otherwise > it returns 0. If you are only returning a yes/no result, bool is better than int. > > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > --- > savevm.c | 73 ++++++++++++++++++++++++++++++++++++++++++++-------------------- > 1 file changed, 50 insertions(+), 23 deletions(-) > > diff --git a/savevm.c b/savevm.c > index 6af84fd..96a2340 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -2197,25 +2197,55 @@ out: > } > > static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, Return bool... > - const char *name) > + const char *name, const char *id, bool old_match) > { > QEMUSnapshotInfo *sn_tab, *sn; > - int nb_sns, i, ret; > + int nb_sns, i, found = 0; ... and initialize 'bool found = false'. > + > + if (!name && !id) { > + return found; > + } It may be appropriate to do assert(name || id), if you want to guarantee that all callers supply at least one of the two arguments (and that the QMP code where both arguments are optional does its own sanity checking that the user supplied an argument. > > - ret = -ENOENT; > nb_sns = bdrv_snapshot_list(bs, &sn_tab); > - if (nb_sns < 0) > - return ret; > + if (nb_sns < 0) { > + return found; Calling the return value 'found' makes the code read awkwardly when reporting failure; maybe keeping it named 'ret' is better after all. > + } > + > for(i = 0; i < nb_sns; i++) { As long as you are touching this code, you could insert the space after 'for'. > sn = &sn_tab[i]; > - if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) { > - *sn_info = *sn; > - ret = 0; > - break; > + if (name && id) { > + if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) { > + *sn_info = *sn; > + found = 1; > + break; > + } > + } else if (name) { > + /* for compatibility for old bdrv_snapshot_find call > + * will be removed */ > + if (old_match) { > + if (!strcmp(sn->id_str, id) || !strcmp(sn->name, name)) { > + *sn_info = *sn; > + found = 1; > + break; > + } > + } else { > + if (!strcmp(sn->name, name)) { > + *sn_info = *sn; > + found = 1; > + break; > + } > + } > + } else if (id) { > + if (!strcmp(sn->id_str, id)) { > + *sn_info = *sn; > + found = 1; > + break; > + } Note that this says that if both name and id are specified, then that takes priority, no matter what was specified for old_match. > } > } > + > g_free(sn_tab); > - return ret; > + return found; > } > > /* > @@ -2229,8 +2259,8 @@ 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) > + if (bdrv_can_snapshot(bs) > + && bdrv_snapshot_find(bs, snapshot, name, name, true)) Needless style change. The old style was sufficient, with: if (bdrv_can_snapshot(bs) && bdrv_snapshot_find(bs, snapshot, name, name, true)) Except that bdrv_snapshot_find(bs, snapshot, name, name, anything) results in a lookup that requires sn->id_str and sn->name be the same string, which is not what you intended. You really want to be using bdrv_snapshot_find(bs, snapshot, NULL, name, true), if you want the old_match parameter to make a difference. ... > @@ -2474,7 +2501,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict) > { > BlockDriverState *bs, *bs1; > QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s; > - int nb_sns, i, ret, available; > + int nb_sns, i, available; > int total; > int *available_snapshots; > char buf[256]; > @@ -2505,8 +2532,8 @@ 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); > - if (ret < 0) { > + if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str, > + true)) { This was the only conversion that passed both id and name that looked correct. All the other conversions to 'name, name, true' need fixing.
Am 16.04.2013 um 18:05 hat Pavel Hrdina geschrieben: > Finding snapshot by a name which could also be an id isn't best way > how to do it. There will be rewrite of savevm, loadvm and delvm to > improve the behavior of these commands. The savevm and loadvm will > have their own patch series. > > Now bdrv_snapshot_find takes more parameters. The name parameter will > be matched only against the name of the snapshot and the same applies > to id parameter. > > There is one exception. If you set the last parameter, the name parameter > will be matched against the name or the id of a snapshot. This exception > is only for backward compatibility for other commands and it will be > dropped after all commands will be rewritten. > > We only need to know if that snapshot exists or not. We don't care > about any error message. If snapshot exists it returns 1 otherwise > it returns 0. Sounds like it should be true/false rather than 1/0. Also, isn't there a difference between "there is no snapshot foo" and "we couldn't find out if there is a snapshot foo because there was an error"? > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > --- > savevm.c | 73 ++++++++++++++++++++++++++++++++++++++++++++-------------------- > 1 file changed, 50 insertions(+), 23 deletions(-) > > diff --git a/savevm.c b/savevm.c > index 6af84fd..96a2340 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -2197,25 +2197,55 @@ out: > } > > static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, > - const char *name) > + const char *name, const char *id, bool old_match) > { > QEMUSnapshotInfo *sn_tab, *sn; > - int nb_sns, i, ret; > + int nb_sns, i, found = 0; > + > + if (!name && !id) { > + return found; > + } Why not just an explicit return 0? Or actually, while you touch the prototype you could make the function return a bool and return false. > > - ret = -ENOENT; > nb_sns = bdrv_snapshot_list(bs, &sn_tab); > - if (nb_sns < 0) > - return ret; > + if (nb_sns < 0) { > + return found; > + } Same here. > + > for(i = 0; i < nb_sns; i++) { You can add a space after 'for' while touching the function. > sn = &sn_tab[i]; > - if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) { > - *sn_info = *sn; > - ret = 0; > - break; > + if (name && id) { > + if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) { > + *sn_info = *sn; > + found = 1; > + break; > + } > + } else if (name) { > + /* for compatibility for old bdrv_snapshot_find call > + * will be removed */ > + if (old_match) { > + if (!strcmp(sn->id_str, id) || !strcmp(sn->name, name)) { > + *sn_info = *sn; > + found = 1; > + break; > + } > + } else { > + if (!strcmp(sn->name, name)) { > + *sn_info = *sn; > + found = 1; > + break; > + } > + } > + } else if (id) { > + if (!strcmp(sn->id_str, id)) { > + *sn_info = *sn; > + found = 1; > + break; > + } > } > } > + > g_free(sn_tab); > - return ret; > + return found; > } > > /* > @@ -2229,8 +2259,8 @@ 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) > + if (bdrv_can_snapshot(bs) > + && bdrv_snapshot_find(bs, snapshot, name, name, true)) It would only retain the old behaviour if you specified NULL for id. Now _both_ name and id must match, which will happen almost never. How did you test this patch? Sounds like something that a qemu-iotests case using the Python QMP methods could easily test. > { > bdrv_snapshot_delete(bs, name, &local_err); > if (error_is_set(&local_err)) { > @@ -2289,8 +2319,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); > - if (ret >= 0) { > + if (bdrv_snapshot_find(bs, old_sn, name, name, true)) { Same problem. > pstrcpy(sn->name, sizeof(sn->name), old_sn->name); > pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str); > } else { > @@ -2380,9 +2409,8 @@ int load_vmstate(const char *name) > } > > /* Don't even try to load empty VM states */ > - ret = bdrv_snapshot_find(bs_vm_state, &sn, name); > - if (ret < 0) { > - return ret; > + if (!bdrv_snapshot_find(bs_vm_state, &sn, name, name, true)) { And again. > + return -ENOENT; We're losing the real error message here. The old code said exactly what happened, the new one always claims that the snapshot doesn't exist even in case of I/O errors or anything. > } else if (sn.vm_state_size == 0) { > error_report("This is a disk-only snapshot. Revert to it offline " > "using qemu-img."); > @@ -2404,11 +2432,10 @@ int load_vmstate(const char *name) > return -ENOTSUP; > } > > - ret = bdrv_snapshot_find(bs, &sn, name); > - if (ret < 0) { > + if (!bdrv_snapshot_find(bs, &sn, name, name, true)) { > error_report("Device '%s' does not have the requested snapshot '%s'", > bdrv_get_device_name(bs), name); > - return ret; > + return -ENOENT; > } > } > > @@ -2474,7 +2501,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict) > { > BlockDriverState *bs, *bs1; > QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s; > - int nb_sns, i, ret, available; > + int nb_sns, i, available; > int total; > int *available_snapshots; > char buf[256]; > @@ -2505,8 +2532,8 @@ 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); > - if (ret < 0) { > + if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str, > + true)) { Is it intention that both id and name must match here? I suspect that no because you set old_match == true, which is however ignored because both id and name are set. > available = 0; > break; > } Kevin
diff --git a/savevm.c b/savevm.c index 6af84fd..96a2340 100644 --- a/savevm.c +++ b/savevm.c @@ -2197,25 +2197,55 @@ out: } static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, - const char *name) + const char *name, const char *id, bool old_match) { QEMUSnapshotInfo *sn_tab, *sn; - int nb_sns, i, ret; + int nb_sns, i, found = 0; + + if (!name && !id) { + return found; + } - ret = -ENOENT; nb_sns = bdrv_snapshot_list(bs, &sn_tab); - if (nb_sns < 0) - return ret; + if (nb_sns < 0) { + return found; + } + 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; + if (name && id) { + if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) { + *sn_info = *sn; + found = 1; + break; + } + } else if (name) { + /* for compatibility for old bdrv_snapshot_find call + * will be removed */ + if (old_match) { + if (!strcmp(sn->id_str, id) || !strcmp(sn->name, name)) { + *sn_info = *sn; + found = 1; + break; + } + } else { + if (!strcmp(sn->name, name)) { + *sn_info = *sn; + found = 1; + break; + } + } + } else if (id) { + if (!strcmp(sn->id_str, id)) { + *sn_info = *sn; + found = 1; + break; + } } } + g_free(sn_tab); - return ret; + return found; } /* @@ -2229,8 +2259,8 @@ 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) + if (bdrv_can_snapshot(bs) + && bdrv_snapshot_find(bs, snapshot, name, name, true)) { bdrv_snapshot_delete(bs, name, &local_err); if (error_is_set(&local_err)) { @@ -2289,8 +2319,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); - if (ret >= 0) { + if (bdrv_snapshot_find(bs, old_sn, name, name, true)) { pstrcpy(sn->name, sizeof(sn->name), old_sn->name); pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str); } else { @@ -2380,9 +2409,8 @@ int load_vmstate(const char *name) } /* Don't even try to load empty VM states */ - ret = bdrv_snapshot_find(bs_vm_state, &sn, name); - if (ret < 0) { - return ret; + if (!bdrv_snapshot_find(bs_vm_state, &sn, name, name, true)) { + return -ENOENT; } else if (sn.vm_state_size == 0) { error_report("This is a disk-only snapshot. Revert to it offline " "using qemu-img."); @@ -2404,11 +2432,10 @@ int load_vmstate(const char *name) return -ENOTSUP; } - ret = bdrv_snapshot_find(bs, &sn, name); - if (ret < 0) { + if (!bdrv_snapshot_find(bs, &sn, name, name, true)) { error_report("Device '%s' does not have the requested snapshot '%s'", bdrv_get_device_name(bs), name); - return ret; + return -ENOENT; } } @@ -2474,7 +2501,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict) { BlockDriverState *bs, *bs1; QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s; - int nb_sns, i, ret, available; + int nb_sns, i, available; int total; int *available_snapshots; char buf[256]; @@ -2505,8 +2532,8 @@ 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); - if (ret < 0) { + if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str, + true)) { available = 0; break; }
Finding snapshot by a name which could also be an id isn't best way how to do it. There will be rewrite of savevm, loadvm and delvm to improve the behavior of these commands. The savevm and loadvm will have their own patch series. Now bdrv_snapshot_find takes more parameters. The name parameter will be matched only against the name of the snapshot and the same applies to id parameter. There is one exception. If you set the last parameter, the name parameter will be matched against the name or the id of a snapshot. This exception is only for backward compatibility for other commands and it will be dropped after all commands will be rewritten. We only need to know if that snapshot exists or not. We don't care about any error message. If snapshot exists it returns 1 otherwise it returns 0. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- savevm.c | 73 ++++++++++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 50 insertions(+), 23 deletions(-)