| Message ID | 13b9d1e79947b89982ec51c421b9b1bd0a7b587d.1366127809.git.phrdina@redhat.com |
|---|---|
| State | New |
| Headers | show |
Hi, Pavel I have implemented it at http://lists.nongnu.org/archive/html/qemu-devel/2013-04/msg02533.html in patch 1,2. Could u check if it satisfy your requirement, if yes maybe you can directly use them. > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > --- > savevm.c | 33 +++++++++++---------------------- > 1 file changed, 11 insertions(+), 22 deletions(-) > > diff --git a/savevm.c b/savevm.c > index 66753da..bc829a5 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -2195,7 +2195,7 @@ out: > } > > static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, > - const char *name, const char *id, bool old_match) > + const char *name, const char *id) > { > QEMUSnapshotInfo *sn_tab, *sn; > int nb_sns, i, found = 0; > @@ -2218,20 +2218,10 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, > 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; > - } > + if (!strcmp(sn->name, name)) { > + *sn_info = *sn; > + found = 1; > + break; > } > } else if (id) { > if (!strcmp(sn->id_str, id)) { > @@ -2290,7 +2280,7 @@ SnapshotInfo *qmp_vm_snapshot_save(const char *name, Error **errp) > sn->date_nsec = tv.tv_usec * 1000; > sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock); > > - if (bdrv_snapshot_find(bs, old_sn, name, NULL, false)) { > + if (bdrv_snapshot_find(bs, old_sn, name, NULL)) { > error_setg(errp, "snapshot '%s' exists", name); > goto the_end; > } else { > @@ -2388,7 +2378,7 @@ SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name, > } > > /* Don't even try to load empty VM states */ > - if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id, false)) { > + if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id)) { > return NULL; > } > > @@ -2413,7 +2403,7 @@ SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name, > return NULL; > } > > - if (!bdrv_snapshot_find(bs, &sn, name, id, false)) { > + if (!bdrv_snapshot_find(bs, &sn, name, id)) { > return NULL; > } > } > @@ -2484,7 +2474,7 @@ SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name, > return NULL; > } > > - if (!bdrv_snapshot_find(bs, &sn, name, id, false)) { > + if (!bdrv_snapshot_find(bs, &sn, name, id)) { > /* no need to set an error if snapshot doesn't exist */ > return NULL; > } > @@ -2501,7 +2491,7 @@ SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name, > bs = NULL; > while ((bs = bdrv_next(bs))) { > if (bdrv_can_snapshot(bs) > - && bdrv_snapshot_find(bs, &sn, name, id, false)) { > + && bdrv_snapshot_find(bs, &sn, name, id)) { > bdrv_snapshot_delete(bs, sn.name, errp); > if (error_is_set(errp)) { > return NULL; > @@ -2549,8 +2539,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict) > > while ((bs1 = bdrv_next(bs1))) { > if (bdrv_can_snapshot(bs1) && bs1 != bs) { > - if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str, > - true)) { > + if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str)) { > available = 0; > break; > } >
Hi Wenchao, unfortunately no. According to new design of savevm, loadvm and delvm I need also search for snapshots that have the specified name and id. I'm also touching bdrv_snapshot_list where I'm adding an Error parameter and changing the return value to be used only for getting a number of snapshots. So in case that there is some error, the return value will be 0. Pavel On 17.4.2013 04:53, Wenchao Xia wrote: > Hi, Pavel > I have implemented it at > http://lists.nongnu.org/archive/html/qemu-devel/2013-04/msg02533.html > in patch 1,2. Could u check if it satisfy your requirement, if yes maybe > you can directly use them. > >> Signed-off-by: Pavel Hrdina <phrdina@redhat.com> >> --- >> savevm.c | 33 +++++++++++---------------------- >> 1 file changed, 11 insertions(+), 22 deletions(-) >> >> diff --git a/savevm.c b/savevm.c >> index 66753da..bc829a5 100644 >> --- a/savevm.c >> +++ b/savevm.c >> @@ -2195,7 +2195,7 @@ out: >> } >> >> static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, >> - const char *name, const char *id, bool old_match) >> + const char *name, const char *id) >> { >> QEMUSnapshotInfo *sn_tab, *sn; >> int nb_sns, i, found = 0; >> @@ -2218,20 +2218,10 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, >> 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; >> - } >> + if (!strcmp(sn->name, name)) { >> + *sn_info = *sn; >> + found = 1; >> + break; >> } >> } else if (id) { >> if (!strcmp(sn->id_str, id)) { >> @@ -2290,7 +2280,7 @@ SnapshotInfo *qmp_vm_snapshot_save(const char *name, Error **errp) >> sn->date_nsec = tv.tv_usec * 1000; >> sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock); >> >> - if (bdrv_snapshot_find(bs, old_sn, name, NULL, false)) { >> + if (bdrv_snapshot_find(bs, old_sn, name, NULL)) { >> error_setg(errp, "snapshot '%s' exists", name); >> goto the_end; >> } else { >> @@ -2388,7 +2378,7 @@ SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name, >> } >> >> /* Don't even try to load empty VM states */ >> - if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id, false)) { >> + if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id)) { >> return NULL; >> } >> >> @@ -2413,7 +2403,7 @@ SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name, >> return NULL; >> } >> >> - if (!bdrv_snapshot_find(bs, &sn, name, id, false)) { >> + if (!bdrv_snapshot_find(bs, &sn, name, id)) { >> return NULL; >> } >> } >> @@ -2484,7 +2474,7 @@ SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name, >> return NULL; >> } >> >> - if (!bdrv_snapshot_find(bs, &sn, name, id, false)) { >> + if (!bdrv_snapshot_find(bs, &sn, name, id)) { >> /* no need to set an error if snapshot doesn't exist */ >> return NULL; >> } >> @@ -2501,7 +2491,7 @@ SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name, >> bs = NULL; >> while ((bs = bdrv_next(bs))) { >> if (bdrv_can_snapshot(bs) >> - && bdrv_snapshot_find(bs, &sn, name, id, false)) { >> + && bdrv_snapshot_find(bs, &sn, name, id)) { >> bdrv_snapshot_delete(bs, sn.name, errp); >> if (error_is_set(errp)) { >> return NULL; >> @@ -2549,8 +2539,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict) >> >> while ((bs1 = bdrv_next(bs1))) { >> if (bdrv_can_snapshot(bs1) && bs1 != bs) { >> - if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str, >> - true)) { >> + if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str)) { >> available = 0; >> break; >> } >> > >
于 2013-4-17 15:52, Pavel Hrdina 写道: > Hi Wenchao, > > unfortunately no. According to new design of savevm, loadvm and delvm I > need also search for snapshots that have the specified name and id. > It seems the logic in your function, is same with mine... > I'm also touching bdrv_snapshot_list where I'm adding an Error parameter I looked it before, but it needs all call back in ./block support it, so is it really necessary? > and changing the return value to be used only for getting a number of > snapshots. So in case that there is some error, the return value will be 0. > > Pavel > > On 17.4.2013 04:53, Wenchao Xia wrote: >> Hi, Pavel >> I have implemented it at >> http://lists.nongnu.org/archive/html/qemu-devel/2013-04/msg02533.html >> in patch 1,2. Could u check if it satisfy your requirement, if yes maybe >> you can directly use them. >> >>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com> >>> --- >>> savevm.c | 33 +++++++++++---------------------- >>> 1 file changed, 11 insertions(+), 22 deletions(-) >>> >>> diff --git a/savevm.c b/savevm.c >>> index 66753da..bc829a5 100644 >>> --- a/savevm.c >>> +++ b/savevm.c >>> @@ -2195,7 +2195,7 @@ out: >>> } >>> >>> static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, >>> - const char *name, const char *id, bool old_match) >>> + const char *name, const char *id) >>> { >>> QEMUSnapshotInfo *sn_tab, *sn; >>> int nb_sns, i, found = 0; >>> @@ -2218,20 +2218,10 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, >>> 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; >>> - } >>> + if (!strcmp(sn->name, name)) { >>> + *sn_info = *sn; >>> + found = 1; >>> + break; >>> } >>> } else if (id) { >>> if (!strcmp(sn->id_str, id)) { >>> @@ -2290,7 +2280,7 @@ SnapshotInfo *qmp_vm_snapshot_save(const char *name, Error **errp) >>> sn->date_nsec = tv.tv_usec * 1000; >>> sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock); >>> >>> - if (bdrv_snapshot_find(bs, old_sn, name, NULL, false)) { >>> + if (bdrv_snapshot_find(bs, old_sn, name, NULL)) { >>> error_setg(errp, "snapshot '%s' exists", name); >>> goto the_end; >>> } else { >>> @@ -2388,7 +2378,7 @@ SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name, >>> } >>> >>> /* Don't even try to load empty VM states */ >>> - if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id, false)) { >>> + if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id)) { >>> return NULL; >>> } >>> >>> @@ -2413,7 +2403,7 @@ SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name, >>> return NULL; >>> } >>> >>> - if (!bdrv_snapshot_find(bs, &sn, name, id, false)) { >>> + if (!bdrv_snapshot_find(bs, &sn, name, id)) { >>> return NULL; >>> } >>> } >>> @@ -2484,7 +2474,7 @@ SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name, >>> return NULL; >>> } >>> >>> - if (!bdrv_snapshot_find(bs, &sn, name, id, false)) { >>> + if (!bdrv_snapshot_find(bs, &sn, name, id)) { >>> /* no need to set an error if snapshot doesn't exist */ >>> return NULL; >>> } >>> @@ -2501,7 +2491,7 @@ SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name, >>> bs = NULL; >>> while ((bs = bdrv_next(bs))) { >>> if (bdrv_can_snapshot(bs) >>> - && bdrv_snapshot_find(bs, &sn, name, id, false)) { >>> + && bdrv_snapshot_find(bs, &sn, name, id)) { >>> bdrv_snapshot_delete(bs, sn.name, errp); >>> if (error_is_set(errp)) { >>> return NULL; >>> @@ -2549,8 +2539,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict) >>> >>> while ((bs1 = bdrv_next(bs1))) { >>> if (bdrv_can_snapshot(bs1) && bs1 != bs) { >>> - if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str, >>> - true)) { >>> + if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str)) { >>> available = 0; >>> break; >>> } >>> >> >> > >
On 17.4.2013 12:19, Wenchao Xia wrote: > 于 2013-4-17 15:52, Pavel Hrdina 写道: >> Hi Wenchao, >> >> unfortunately no. According to new design of savevm, loadvm and delvm I >> need also search for snapshots that have the specified name and id. >> > It seems the logic in your function, is same with mine... It is not the same. Your logic: if id is set: if there is snapshot with that id: end searching if name set (search also if id is set but nothing found): if there is snapshot with that name: end searching My logic: if name is set and id is set: if there is snapshot with than name and with that id: end searching else if name is set (means that only name is set): if there is snapshot with that name: end searching else if id is set (means that only id is set): if there is snapshot with that id: end searching > >> I'm also touching bdrv_snapshot_list where I'm adding an Error parameter > I looked it before, but it needs all call back in ./block support it, > so is it really necessary? I think it is better if this function internally set appropriate error message based on used disk image format (qcow2, sheepdog, rbd). Adding to CC Eric for his opinion. > >> and changing the return value to be used only for getting a number of >> snapshots. So in case that there is some error, the return value will be 0. >> >> Pavel >> >> On 17.4.2013 04:53, Wenchao Xia wrote: >>> Hi, Pavel >>> I have implemented it at >>> http://lists.nongnu.org/archive/html/qemu-devel/2013-04/msg02533.html >>> in patch 1,2. Could u check if it satisfy your requirement, if yes maybe >>> you can directly use them. >>> >>>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com> >>>> --- >>>> savevm.c | 33 +++++++++++---------------------- >>>> 1 file changed, 11 insertions(+), 22 deletions(-) >>>> >>>> diff --git a/savevm.c b/savevm.c >>>> index 66753da..bc829a5 100644 >>>> --- a/savevm.c >>>> +++ b/savevm.c >>>> @@ -2195,7 +2195,7 @@ out: >>>> } >>>> >>>> static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, >>>> - const char *name, const char *id, bool old_match) >>>> + const char *name, const char *id) >>>> { >>>> QEMUSnapshotInfo *sn_tab, *sn; >>>> int nb_sns, i, found = 0; >>>> @@ -2218,20 +2218,10 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, >>>> 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; >>>> - } >>>> + if (!strcmp(sn->name, name)) { >>>> + *sn_info = *sn; >>>> + found = 1; >>>> + break; >>>> } >>>> } else if (id) { >>>> if (!strcmp(sn->id_str, id)) { >>>> @@ -2290,7 +2280,7 @@ SnapshotInfo *qmp_vm_snapshot_save(const char *name, Error **errp) >>>> sn->date_nsec = tv.tv_usec * 1000; >>>> sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock); >>>> >>>> - if (bdrv_snapshot_find(bs, old_sn, name, NULL, false)) { >>>> + if (bdrv_snapshot_find(bs, old_sn, name, NULL)) { >>>> error_setg(errp, "snapshot '%s' exists", name); >>>> goto the_end; >>>> } else { >>>> @@ -2388,7 +2378,7 @@ SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name, >>>> } >>>> >>>> /* Don't even try to load empty VM states */ >>>> - if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id, false)) { >>>> + if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id)) { >>>> return NULL; >>>> } >>>> >>>> @@ -2413,7 +2403,7 @@ SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name, >>>> return NULL; >>>> } >>>> >>>> - if (!bdrv_snapshot_find(bs, &sn, name, id, false)) { >>>> + if (!bdrv_snapshot_find(bs, &sn, name, id)) { >>>> return NULL; >>>> } >>>> } >>>> @@ -2484,7 +2474,7 @@ SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name, >>>> return NULL; >>>> } >>>> >>>> - if (!bdrv_snapshot_find(bs, &sn, name, id, false)) { >>>> + if (!bdrv_snapshot_find(bs, &sn, name, id)) { >>>> /* no need to set an error if snapshot doesn't exist */ >>>> return NULL; >>>> } >>>> @@ -2501,7 +2491,7 @@ SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name, >>>> bs = NULL; >>>> while ((bs = bdrv_next(bs))) { >>>> if (bdrv_can_snapshot(bs) >>>> - && bdrv_snapshot_find(bs, &sn, name, id, false)) { >>>> + && bdrv_snapshot_find(bs, &sn, name, id)) { >>>> bdrv_snapshot_delete(bs, sn.name, errp); >>>> if (error_is_set(errp)) { >>>> return NULL; >>>> @@ -2549,8 +2539,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict) >>>> >>>> while ((bs1 = bdrv_next(bs1))) { >>>> if (bdrv_can_snapshot(bs1) && bs1 != bs) { >>>> - if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str, >>>> - true)) { >>>> + if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str)) { >>>> available = 0; >>>> break; >>>> } >>>> >>> >>> >> >> > >
On 04/17/2013 04:51 AM, Pavel Hrdina wrote: > On 17.4.2013 12:19, Wenchao Xia wrote: >> 于 2013-4-17 15:52, Pavel Hrdina 写道: >>> Hi Wenchao, >>> >>> unfortunately no. According to new design of savevm, loadvm and delvm I >>> need also search for snapshots that have the specified name and id. >>> >> It seems the logic in your function, is same with mine... > > It is not the same. Consider a qcow2 file with the following snapshots: id tag 1 2 2 1 3 none Existing logic: only one string is passed, and only one loop is performed. If it matches id or name, end searching search for 1 - finds id 1 tag 2 search for 2 - finds id 1 tag 2 search for 3 - finds id 3 no tag search for 4 - finds nothing no way to find id 2 tag 1 The last point proves that the current algorithm is not ideal, and that we are justified in changing it. But there are several ways to change it, and a consideration of whether we should preserve backwards compatibility in HMP by making the search itself have backwards compatibility, or by making the QMP search follow strict rules where HMP can emulate some measure of back-compat by calling into QMP more than once. > > Your logic: [Wenchao] > if id is set: > if there is snapshot with that id: > end searching > if name set (search also if id is set but nothing found): > if there is snapshot with that name: > end searching > > My logic: [Pavel] > if name is set and id is set: > if there is snapshot with than name and with that id: > end searching > else if name is set (means that only name is set): > if there is snapshot with that name: > end searching > else if id is set (means that only id is set): > if there is snapshot with that id: > end searching Best is a side-by-side comparison; when comparing to existing, I showed three different choices of expanding a single name into a two-argument find call. search for: finds compared to id name Wenchao Pavel existing name -> find(id, NULL) 1 NULL id 1 tag 2 id 1 tag 2 id 1 tag 2 * 2 NULL id 2 tag 1 id 2 tag 1 id 1 tag 2 3 NULL id 3 no tag id 3 no tag id 3 no tag 4 NULL nothing nothing nothing name -> find(NULL, tag) * NULL 1 id 2 tag 1 id 2 tag 1 id 1 tag 2 NULL 2 id 1 tag 2 id 1 tag 2 id 1 tag 2 * NULL 3 nothing nothing id 3 no tag NULL 4 nothing nothing nothing not possible 1 2 id 1 tag 2 id 1 tag 2 -- 2 1 id 2 tag 1 id 2 tag 1 -- name -> find(id, tag) * 1 1 id 1 tag 2 nothing id 1 tag 2 * 2 2 id 2 tag 1 nothing id 1 tag 2 * 3 3 id 3 no tag nothing id 3 no tag 4 4 nothing nothing nothing The two proposed approaches both allow access to a lookup that the original could not provide (namely, id 2 tag 1), so they are an improvement on that front. But the two approaches differ on behavior when both id and name are specified (Wenchao's behaves the same as an id-only lookup, regardless of whether the name matches; Pavel's requires a double match). The other thing to note is that the old code allowed a single name to match an anonymous snapshot, but both proposals fail to find a nameless snapshot without an id search. Can I put yet another proposed algorithm on the table? First, written with four loops over the source (although at most two are taken): if name is set and id is set: if there is snapshot with than name and with that id (loop 1): end searching else if name is set (means that only name is set): if there is snapshot with that name (loop 2): end searching if there is snapshot with that id (loop 3): end searching else if id is set (means that only id is set): if there is snapshot with that id (loop 4): end searching Or, written another way, to implement the same results in only two coded loops: if name is set: if there is a snapshot with that name (loop 1): if id is set: if id matches: end searching successfully else: fail else: end searching successfully else if id is not set: set id to name if there is a snapshot with id (loop 2): end searching successfully And the resulting comparison table, again with three possibilities of how to convert one-argument lookup into two-argument: search for: finds compared to id name mine existing name -> find(id, NULL) 1 NULL id 1 tag 2 id 1 tag 2 * 2 NULL id 2 tag 1 id 1 tag 2 3 NULL id 3 no tag id 3 no tag 4 NULL nothing nothing name -> find(NULL, tag) * NULL 1 id 2 tag 1 id 1 tag 2 NULL 2 id 1 tag 2 id 1 tag 2 NULL 3 id 3 no tag id 3 no tag NULL 4 nothing nothing not possible 1 2 id 1 tag 2 -- 2 1 id 2 tag 1 -- name -> find(id, tag) * 1 1 nothing id 1 tag 2 * 2 2 nothing id 1 tag 2 * 3 3 nothing id 3 no tag 4 4 nothing nothing With that adjusted table, I would favor converting any single name lookup into find(NULL, tag). Only the new QMP command that lets us do an explicit id lookup can search for an id regardless of another name matching the id; but we at least have the benefit of being able to access ALL named snapshots (better than the original code unable to access tag 2), as well as the ability to access unambiguous ids without extra effort (ability to access id 3 without having to change the command line). It keeps the aspect of Pavel's code that specifying both fields must match both fields, but otherwise favors names over ids (generally good, since names are generally not numeric, except when we are talking about corner cases). So, was I convincing enough in arguing that we want something different from the approach of both existing patch series? > >> >>> I'm also touching bdrv_snapshot_list where I'm adding an Error parameter >> I looked it before, but it needs all call back in ./block support it, >> so is it really necessary? > > I think it is better if this function internally set appropriate error > message based on used disk image format (qcow2, sheepdog, rbd). I like the idea of find() setting errp on lookup failure. Callers can ignore errors in situations where a failed lookup triggers a reasonable fallback, but in case the caller wants to report an error, the lower down that we generate an error message, the more likely the error message is to contain full context rather than being dumbed down by generating it higher on the call stack.
On 04/17/2013 12:14 PM, Eric Blake wrote: > Or, written another way, to implement the same results in only two coded > loops: Missed a line: > > if name is set: > if there is a snapshot with that name (loop 1): > if id is set: > if id matches: > end searching successfully > else: > fail > else: > end searching successfully > else if id is not set: > set id to name These two lines should instead be: else if id is set: fail set id to name > if there is a snapshot with id (loop 2): > end searching successfully and of course, all of the algorithms have an implied fail if you make it through all of the loops without a successful search end. The tweak above is needed so that a find(id, tag) that fails a tag lookup does not then succeed on an id lookup.
于 2013-4-18 2:14, Eric Blake 写道: > On 04/17/2013 04:51 AM, Pavel Hrdina wrote: >> On 17.4.2013 12:19, Wenchao Xia wrote: >>> 于 2013-4-17 15:52, Pavel Hrdina 写道: >>>> Hi Wenchao, >>>> >>>> unfortunately no. According to new design of savevm, loadvm and delvm I >>>> need also search for snapshots that have the specified name and id. >>>> >>> It seems the logic in your function, is same with mine... >> >> It is not the same. > > Consider a qcow2 file with the following snapshots: > > id tag > 1 2 > 2 1 > 3 none > > Existing logic: > > only one string is passed, and only one loop is performed. If it > matches id or name, end searching > > search for 1 - finds id 1 tag 2 > search for 2 - finds id 1 tag 2 > search for 3 - finds id 3 no tag > search for 4 - finds nothing > > no way to find id 2 tag 1 > > The last point proves that the current algorithm is not ideal, and that > we are justified in changing it. But there are several ways to change > it, and a consideration of whether we should preserve backwards > compatibility in HMP by making the search itself have backwards > compatibility, or by making the QMP search follow strict rules where HMP > can emulate some measure of back-compat by calling into QMP more than once. > >> >> Your logic: > [Wenchao] >> if id is set: >> if there is snapshot with that id: >> end searching >> if name set (search also if id is set but nothing found): >> if there is snapshot with that name: >> end searching >> >> My logic: > [Pavel] >> if name is set and id is set: >> if there is snapshot with than name and with that id: >> end searching >> else if name is set (means that only name is set): >> if there is snapshot with that name: >> end searching >> else if id is set (means that only id is set): >> if there is snapshot with that id: >> end searching > > Best is a side-by-side comparison; when comparing to existing, I showed > three different choices of expanding a single name into a two-argument > find call. > > search for: finds compared to > id name Wenchao Pavel existing > name -> find(id, NULL) > 1 NULL id 1 tag 2 id 1 tag 2 id 1 tag 2 > * 2 NULL id 2 tag 1 id 2 tag 1 id 1 tag 2 > 3 NULL id 3 no tag id 3 no tag id 3 no tag > 4 NULL nothing nothing nothing > name -> find(NULL, tag) > * NULL 1 id 2 tag 1 id 2 tag 1 id 1 tag 2 > NULL 2 id 1 tag 2 id 1 tag 2 id 1 tag 2 > * NULL 3 nothing nothing id 3 no tag > NULL 4 nothing nothing nothing > not possible > 1 2 id 1 tag 2 id 1 tag 2 -- > 2 1 id 2 tag 1 id 2 tag 1 -- > name -> find(id, tag) > * 1 1 id 1 tag 2 nothing id 1 tag 2 > * 2 2 id 2 tag 1 nothing id 1 tag 2 > * 3 3 id 3 no tag nothing id 3 no tag > 4 4 nothing nothing nothing > > The two proposed approaches both allow access to a lookup that the > original could not provide (namely, id 2 tag 1), so they are an > improvement on that front. But the two approaches differ on behavior > when both id and name are specified (Wenchao's behaves the same as an > id-only lookup, regardless of whether the name matches; Pavel's requires > a double match). The other thing to note is that the old code allowed a > single name to match an anonymous snapshot, but both proposals fail to > find a nameless snapshot without an id search. > > Can I put yet another proposed algorithm on the table? First, written > with four loops over the source (although at most two are taken): > > if name is set and id is set: > if there is snapshot with than name and with that id (loop 1): > end searching > else if name is set (means that only name is set): > if there is snapshot with that name (loop 2): > end searching > if there is snapshot with that id (loop 3): > end searching > else if id is set (means that only id is set): > if there is snapshot with that id (loop 4): > end searching > > Or, written another way, to implement the same results in only two coded > loops: > > if name is set: > if there is a snapshot with that name (loop 1): > if id is set: > if id matches: > end searching successfully > else: > fail > else: > end searching successfully > else if id is not set: > set id to name > if there is a snapshot with id (loop 2): > end searching successfully > > And the resulting comparison table, again with three possibilities of > how to convert one-argument lookup into two-argument: > > search for: finds compared to > id name mine existing > name -> find(id, NULL) > 1 NULL id 1 tag 2 id 1 tag 2 > * 2 NULL id 2 tag 1 id 1 tag 2 > 3 NULL id 3 no tag id 3 no tag > 4 NULL nothing nothing > name -> find(NULL, tag) > * NULL 1 id 2 tag 1 id 1 tag 2 > NULL 2 id 1 tag 2 id 1 tag 2 > NULL 3 id 3 no tag id 3 no tag > NULL 4 nothing nothing > not possible > 1 2 id 1 tag 2 -- > 2 1 id 2 tag 1 -- > name -> find(id, tag) > * 1 1 nothing id 1 tag 2 > * 2 2 nothing id 1 tag 2 > * 3 3 nothing id 3 no tag > 4 4 nothing nothing > > > With that adjusted table, I would favor converting any single name > lookup into find(NULL, tag). Only the new QMP command that lets us do > an explicit id lookup can search for an id regardless of another name > matching the id; but we at least have the benefit of being able to > access ALL named snapshots (better than the original code unable to > access tag 2), as well as the ability to access unambiguous ids without > extra effort (ability to access id 3 without having to change the > command line). It keeps the aspect of Pavel's code that specifying both > fields must match both fields, but otherwise favors names over ids > (generally good, since names are generally not numeric, except when we > are talking about corner cases). > > So, was I convincing enough in arguing that we want something different > from the approach of both existing patch series? > Plenty of details, thanks, Eric. I think Pavel's logic is better, which increase the capability of it, and I don't think a more complex logic should be there, since it is an internal function need clear meaning. >> >>> >>>> I'm also touching bdrv_snapshot_list where I'm adding an Error parameter >>> I looked it before, but it needs all call back in ./block support it, >>> so is it really necessary? >> >> I think it is better if this function internally set appropriate error >> message based on used disk image format (qcow2, sheepdog, rbd). > > I like the idea of find() setting errp on lookup failure. Callers can > ignore errors in situations where a failed lookup triggers a reasonable > fallback, but in case the caller wants to report an error, the lower > down that we generate an error message, the more likely the error > message is to contain full context rather than being dumbed down by > generating it higher on the call stack. > I understand it helps, but now I need just a good bdrv_snapshot_find() to use, so could this improvement work be postponded later? I think neither my or Pavel's work is depending on it. Pavel, to make us progress, I'd like a small serial change bdrv_snapshot_find() first, then we can rebase on it, are u OK with it? block/qapi.c: int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, const char *name, const char *id)
>> So, was I convincing enough in arguing that we want something different >> from the approach of both existing patch series? >> > Plenty of details, thanks, Eric. I think Pavel's logic is better, > which increase the capability of it, and I don't think a more complex > logic should be there, since it is an internal function need clear > meaning. > > >>> >> > I understand it helps, but now I need just a good > bdrv_snapshot_find() to use, so could this improvement work > be postponded later? I think neither my or Pavel's work > is depending on it. > Pavel, to make us progress, I'd like a small serial change > bdrv_snapshot_find() first, then we can rebase on it, are u OK > with it? > > block/qapi.c: > int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, > const char *name, const char *id) > > > Actually my serial, two condition are "or" relationship, but in Pavel's they are "and" relationship. From caller, I think "and" is better, and taking two parameter in one function make caller easier to use, in most case. I'd like a clean serial go for it, hope to know Pavel and Markus's idea.
On 18.4.2013 06:31, Wenchao Xia wrote: > 于 2013-4-18 2:14, Eric Blake 写道: >> On 04/17/2013 04:51 AM, Pavel Hrdina wrote: >>> On 17.4.2013 12:19, Wenchao Xia wrote: >>>> 于 2013-4-17 15:52, Pavel Hrdina 写道: >>>>> Hi Wenchao, >>>>> >>>>> unfortunately no. According to new design of savevm, loadvm and >>>>> delvm I >>>>> need also search for snapshots that have the specified name and id. >>>>> >>>> It seems the logic in your function, is same with mine... >>> >>> It is not the same. >> >> Consider a qcow2 file with the following snapshots: >> >> id tag >> 1 2 >> 2 1 >> 3 none >> >> Existing logic: >> >> only one string is passed, and only one loop is performed. If it >> matches id or name, end searching >> >> search for 1 - finds id 1 tag 2 >> search for 2 - finds id 1 tag 2 >> search for 3 - finds id 3 no tag >> search for 4 - finds nothing >> >> no way to find id 2 tag 1 >> >> The last point proves that the current algorithm is not ideal, and that >> we are justified in changing it. But there are several ways to change >> it, and a consideration of whether we should preserve backwards >> compatibility in HMP by making the search itself have backwards >> compatibility, or by making the QMP search follow strict rules where HMP >> can emulate some measure of back-compat by calling into QMP more than >> once. >> >>> >>> Your logic: >> [Wenchao] >>> if id is set: >>> if there is snapshot with that id: >>> end searching >>> if name set (search also if id is set but nothing found): >>> if there is snapshot with that name: >>> end searching >>> >>> My logic: >> [Pavel] >>> if name is set and id is set: >>> if there is snapshot with than name and with that id: >>> end searching >>> else if name is set (means that only name is set): >>> if there is snapshot with that name: >>> end searching >>> else if id is set (means that only id is set): >>> if there is snapshot with that id: >>> end searching >> >> Best is a side-by-side comparison; when comparing to existing, I showed >> three different choices of expanding a single name into a two-argument >> find call. >> >> search for: finds compared to >> id name Wenchao Pavel existing >> name -> find(id, NULL) >> 1 NULL id 1 tag 2 id 1 tag 2 id 1 tag 2 >> * 2 NULL id 2 tag 1 id 2 tag 1 id 1 tag 2 >> 3 NULL id 3 no tag id 3 no tag id 3 no tag >> 4 NULL nothing nothing nothing >> name -> find(NULL, tag) >> * NULL 1 id 2 tag 1 id 2 tag 1 id 1 tag 2 >> NULL 2 id 1 tag 2 id 1 tag 2 id 1 tag 2 >> * NULL 3 nothing nothing id 3 no tag >> NULL 4 nothing nothing nothing >> not possible >> 1 2 id 1 tag 2 id 1 tag 2 -- >> 2 1 id 2 tag 1 id 2 tag 1 -- >> name -> find(id, tag) >> * 1 1 id 1 tag 2 nothing id 1 tag 2 >> * 2 2 id 2 tag 1 nothing id 1 tag 2 >> * 3 3 id 3 no tag nothing id 3 no tag >> 4 4 nothing nothing nothing >> >> The two proposed approaches both allow access to a lookup that the >> original could not provide (namely, id 2 tag 1), so they are an >> improvement on that front. But the two approaches differ on behavior >> when both id and name are specified (Wenchao's behaves the same as an >> id-only lookup, regardless of whether the name matches; Pavel's requires >> a double match). The other thing to note is that the old code allowed a >> single name to match an anonymous snapshot, but both proposals fail to >> find a nameless snapshot without an id search. >> >> Can I put yet another proposed algorithm on the table? First, written >> with four loops over the source (although at most two are taken): >> >> if name is set and id is set: >> if there is snapshot with than name and with that id (loop 1): >> end searching >> else if name is set (means that only name is set): >> if there is snapshot with that name (loop 2): >> end searching >> if there is snapshot with that id (loop 3): >> end searching >> else if id is set (means that only id is set): >> if there is snapshot with that id (loop 4): >> end searching >> >> Or, written another way, to implement the same results in only two coded >> loops: >> >> if name is set: >> if there is a snapshot with that name (loop 1): >> if id is set: >> if id matches: >> end searching successfully >> else: >> fail >> else: >> end searching successfully >> else if id is not set: >> set id to name >> if there is a snapshot with id (loop 2): >> end searching successfully >> >> And the resulting comparison table, again with three possibilities of >> how to convert one-argument lookup into two-argument: >> >> search for: finds compared to >> id name mine existing >> name -> find(id, NULL) >> 1 NULL id 1 tag 2 id 1 tag 2 >> * 2 NULL id 2 tag 1 id 1 tag 2 >> 3 NULL id 3 no tag id 3 no tag >> 4 NULL nothing nothing >> name -> find(NULL, tag) >> * NULL 1 id 2 tag 1 id 1 tag 2 >> NULL 2 id 1 tag 2 id 1 tag 2 >> NULL 3 id 3 no tag id 3 no tag >> NULL 4 nothing nothing >> not possible >> 1 2 id 1 tag 2 -- >> 2 1 id 2 tag 1 -- >> name -> find(id, tag) >> * 1 1 nothing id 1 tag 2 >> * 2 2 nothing id 1 tag 2 >> * 3 3 nothing id 3 no tag >> 4 4 nothing nothing >> >> >> With that adjusted table, I would favor converting any single name >> lookup into find(NULL, tag). Only the new QMP command that lets us do >> an explicit id lookup can search for an id regardless of another name >> matching the id; but we at least have the benefit of being able to >> access ALL named snapshots (better than the original code unable to >> access tag 2), as well as the ability to access unambiguous ids without >> extra effort (ability to access id 3 without having to change the >> command line). It keeps the aspect of Pavel's code that specifying both >> fields must match both fields, but otherwise favors names over ids >> (generally good, since names are generally not numeric, except when we >> are talking about corner cases). >> >> So, was I convincing enough in arguing that we want something different >> from the approach of both existing patch series? >> > Plenty of details, thanks, Eric. I think Pavel's logic is better, > which increase the capability of it, and I don't think a more complex > logic should be there, since it is an internal function need clear > meaning. > I also thank you Eric for that detailed description. I must agree with Wenchao that internal function needs clear meaning. Why would I need to find snapshot by id using name parameter if I have the id parameter? I know that name is in almost all cases some string and not a number and that it could be for some cases easier just find snapshot by id using name parameter, but I think that we must be strict about this logic. > >>> >>>> >>>>> I'm also touching bdrv_snapshot_list where I'm adding an Error >>>>> parameter >>>> I looked it before, but it needs all call back in ./block >>>> support it, >>>> so is it really necessary? >>> >>> I think it is better if this function internally set appropriate error >>> message based on used disk image format (qcow2, sheepdog, rbd). >> >> I like the idea of find() setting errp on lookup failure. Callers can >> ignore errors in situations where a failed lookup triggers a reasonable >> fallback, but in case the caller wants to report an error, the lower >> down that we generate an error message, the more likely the error >> message is to contain full context rather than being dumbed down by >> generating it higher on the call stack. >> > I understand it helps, but now I need just a good > bdrv_snapshot_find() to use, so could this improvement work > be postponded later? I think neither my or Pavel's work > is depending on it. > Pavel, to make us progress, I'd like a small serial change > bdrv_snapshot_find() first, then we can rebase on it, are u OK > with it? > > block/qapi.c: > int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, > const char *name, const char *id) > I think that my whole series is almost done and it would be quickly accepted and applied upstream. I'll send today the v2 and we will see. Pavel
于 2013-4-18 18:22, Pavel Hrdina 写道: > On 18.4.2013 06:31, Wenchao Xia wrote: >> 于 2013-4-18 2:14, Eric Blake 写道: >>> On 04/17/2013 04:51 AM, Pavel Hrdina wrote: >>>> On 17.4.2013 12:19, Wenchao Xia wrote: >>>>> 于 2013-4-17 15:52, Pavel Hrdina 写道: >>>>>> Hi Wenchao, >>>>>> >>>>>> unfortunately no. According to new design of savevm, loadvm and >>>>>> delvm I >>>>>> need also search for snapshots that have the specified name and id. >>>>>> >>>>> It seems the logic in your function, is same with mine... >>>> >>>> It is not the same. >>> >>> Consider a qcow2 file with the following snapshots: >>> >>> id tag >>> 1 2 >>> 2 1 >>> 3 none >>> >>> Existing logic: >>> >>> only one string is passed, and only one loop is performed. If it >>> matches id or name, end searching >>> >>> search for 1 - finds id 1 tag 2 >>> search for 2 - finds id 1 tag 2 >>> search for 3 - finds id 3 no tag >>> search for 4 - finds nothing >>> >>> no way to find id 2 tag 1 >>> >>> The last point proves that the current algorithm is not ideal, and that >>> we are justified in changing it. But there are several ways to change >>> it, and a consideration of whether we should preserve backwards >>> compatibility in HMP by making the search itself have backwards >>> compatibility, or by making the QMP search follow strict rules where HMP >>> can emulate some measure of back-compat by calling into QMP more than >>> once. >>> >>>> >>>> Your logic: >>> [Wenchao] >>>> if id is set: >>>> if there is snapshot with that id: >>>> end searching >>>> if name set (search also if id is set but nothing found): >>>> if there is snapshot with that name: >>>> end searching >>>> >>>> My logic: >>> [Pavel] >>>> if name is set and id is set: >>>> if there is snapshot with than name and with that id: >>>> end searching >>>> else if name is set (means that only name is set): >>>> if there is snapshot with that name: >>>> end searching >>>> else if id is set (means that only id is set): >>>> if there is snapshot with that id: >>>> end searching >>> >>> Best is a side-by-side comparison; when comparing to existing, I showed >>> three different choices of expanding a single name into a two-argument >>> find call. >>> >>> search for: finds compared to >>> id name Wenchao Pavel existing >>> name -> find(id, NULL) >>> 1 NULL id 1 tag 2 id 1 tag 2 id 1 tag 2 >>> * 2 NULL id 2 tag 1 id 2 tag 1 id 1 tag 2 >>> 3 NULL id 3 no tag id 3 no tag id 3 no tag >>> 4 NULL nothing nothing nothing >>> name -> find(NULL, tag) >>> * NULL 1 id 2 tag 1 id 2 tag 1 id 1 tag 2 >>> NULL 2 id 1 tag 2 id 1 tag 2 id 1 tag 2 >>> * NULL 3 nothing nothing id 3 no tag >>> NULL 4 nothing nothing nothing >>> not possible >>> 1 2 id 1 tag 2 id 1 tag 2 -- >>> 2 1 id 2 tag 1 id 2 tag 1 -- >>> name -> find(id, tag) >>> * 1 1 id 1 tag 2 nothing id 1 tag 2 >>> * 2 2 id 2 tag 1 nothing id 1 tag 2 >>> * 3 3 id 3 no tag nothing id 3 no tag >>> 4 4 nothing nothing nothing >>> >>> The two proposed approaches both allow access to a lookup that the >>> original could not provide (namely, id 2 tag 1), so they are an >>> improvement on that front. But the two approaches differ on behavior >>> when both id and name are specified (Wenchao's behaves the same as an >>> id-only lookup, regardless of whether the name matches; Pavel's requires >>> a double match). The other thing to note is that the old code allowed a >>> single name to match an anonymous snapshot, but both proposals fail to >>> find a nameless snapshot without an id search. >>> >>> Can I put yet another proposed algorithm on the table? First, written >>> with four loops over the source (although at most two are taken): >>> >>> if name is set and id is set: >>> if there is snapshot with than name and with that id (loop 1): >>> end searching >>> else if name is set (means that only name is set): >>> if there is snapshot with that name (loop 2): >>> end searching >>> if there is snapshot with that id (loop 3): >>> end searching >>> else if id is set (means that only id is set): >>> if there is snapshot with that id (loop 4): >>> end searching >>> >>> Or, written another way, to implement the same results in only two coded >>> loops: >>> >>> if name is set: >>> if there is a snapshot with that name (loop 1): >>> if id is set: >>> if id matches: >>> end searching successfully >>> else: >>> fail >>> else: >>> end searching successfully >>> else if id is not set: >>> set id to name >>> if there is a snapshot with id (loop 2): >>> end searching successfully >>> >>> And the resulting comparison table, again with three possibilities of >>> how to convert one-argument lookup into two-argument: >>> >>> search for: finds compared to >>> id name mine existing >>> name -> find(id, NULL) >>> 1 NULL id 1 tag 2 id 1 tag 2 >>> * 2 NULL id 2 tag 1 id 1 tag 2 >>> 3 NULL id 3 no tag id 3 no tag >>> 4 NULL nothing nothing >>> name -> find(NULL, tag) >>> * NULL 1 id 2 tag 1 id 1 tag 2 >>> NULL 2 id 1 tag 2 id 1 tag 2 >>> NULL 3 id 3 no tag id 3 no tag >>> NULL 4 nothing nothing >>> not possible >>> 1 2 id 1 tag 2 -- >>> 2 1 id 2 tag 1 -- >>> name -> find(id, tag) >>> * 1 1 nothing id 1 tag 2 >>> * 2 2 nothing id 1 tag 2 >>> * 3 3 nothing id 3 no tag >>> 4 4 nothing nothing >>> >>> >>> With that adjusted table, I would favor converting any single name >>> lookup into find(NULL, tag). Only the new QMP command that lets us do >>> an explicit id lookup can search for an id regardless of another name >>> matching the id; but we at least have the benefit of being able to >>> access ALL named snapshots (better than the original code unable to >>> access tag 2), as well as the ability to access unambiguous ids without >>> extra effort (ability to access id 3 without having to change the >>> command line). It keeps the aspect of Pavel's code that specifying both >>> fields must match both fields, but otherwise favors names over ids >>> (generally good, since names are generally not numeric, except when we >>> are talking about corner cases). >>> >>> So, was I convincing enough in arguing that we want something different >>> from the approach of both existing patch series? >>> >> Plenty of details, thanks, Eric. I think Pavel's logic is better, >> which increase the capability of it, and I don't think a more complex >> logic should be there, since it is an internal function need clear >> meaning. >> > > I also thank you Eric for that detailed description. I must agree with > Wenchao that internal function needs clear meaning. Why would I need to > find snapshot by id using name parameter if I have the id parameter? I > know that name is in almost all cases some string and not a number and > that it could be for some cases easier just find snapshot by id using > name parameter, but I think that we must be strict about this logic. > >> >>>> >>>>> >>>>>> I'm also touching bdrv_snapshot_list where I'm adding an Error >>>>>> parameter >>>>> I looked it before, but it needs all call back in ./block >>>>> support it, >>>>> so is it really necessary? >>>> >>>> I think it is better if this function internally set appropriate error >>>> message based on used disk image format (qcow2, sheepdog, rbd). >>> >>> I like the idea of find() setting errp on lookup failure. Callers can >>> ignore errors in situations where a failed lookup triggers a reasonable >>> fallback, but in case the caller wants to report an error, the lower >>> down that we generate an error message, the more likely the error >>> message is to contain full context rather than being dumbed down by >>> generating it higher on the call stack. >>> >> I understand it helps, but now I need just a good >> bdrv_snapshot_find() to use, so could this improvement work >> be postponded later? I think neither my or Pavel's work >> is depending on it. >> Pavel, to make us progress, I'd like a small serial change >> bdrv_snapshot_find() first, then we can rebase on it, are u OK >> with it? >> >> block/qapi.c: >> int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, >> const char *name, const char *id) >> > > I think that my whole series is almost done and it would be quickly > accepted and applied upstream. I'll send today the v2 and we will see. > > Pavel > OK, I'll review v2 too to make it faster.
>> >> I think that my whole series is almost done and it would be quickly >> accepted and applied upstream. I'll send today the v2 and we will see. >> >> Pavel >> > OK, I'll review v2 too to make it faster. > Hi, Pavel Sorry for query your progress, I hope to catch up qemu 1.5, so wonder whether V2 is ready. If you agree, I will use your logic in my bdrv_snapshot_find() to progress. But still, if you can upstream your version quickly, or form a clean patch for this function, I am happy to rebase on it.:>
On 24.4.2013 05:51, Wenchao Xia wrote: > >>> >>> I think that my whole series is almost done and it would be quickly >>> accepted and applied upstream. I'll send today the v2 and we will see. >>> >>> Pavel >>> >> OK, I'll review v2 too to make it faster. >> > Hi, Pavel > Sorry for query your progress, I hope to catch up qemu 1.5, so > wonder whether V2 is ready. > If you agree, I will use your logic in my bdrv_snapshot_find() to > progress. But still, if you can upstream your version quickly, or form > a clean patch for this function, I am happy to rebase on it.:> > Hi Wenchao, np, I have the v2 almost ready and today I'll send that v2 into upstream for review.
diff --git a/savevm.c b/savevm.c index 66753da..bc829a5 100644 --- a/savevm.c +++ b/savevm.c @@ -2195,7 +2195,7 @@ out: } static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, - const char *name, const char *id, bool old_match) + const char *name, const char *id) { QEMUSnapshotInfo *sn_tab, *sn; int nb_sns, i, found = 0; @@ -2218,20 +2218,10 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, 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; - } + if (!strcmp(sn->name, name)) { + *sn_info = *sn; + found = 1; + break; } } else if (id) { if (!strcmp(sn->id_str, id)) { @@ -2290,7 +2280,7 @@ SnapshotInfo *qmp_vm_snapshot_save(const char *name, Error **errp) sn->date_nsec = tv.tv_usec * 1000; sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock); - if (bdrv_snapshot_find(bs, old_sn, name, NULL, false)) { + if (bdrv_snapshot_find(bs, old_sn, name, NULL)) { error_setg(errp, "snapshot '%s' exists", name); goto the_end; } else { @@ -2388,7 +2378,7 @@ SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name, } /* Don't even try to load empty VM states */ - if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id, false)) { + if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id)) { return NULL; } @@ -2413,7 +2403,7 @@ SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name, return NULL; } - if (!bdrv_snapshot_find(bs, &sn, name, id, false)) { + if (!bdrv_snapshot_find(bs, &sn, name, id)) { return NULL; } } @@ -2484,7 +2474,7 @@ SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name, return NULL; } - if (!bdrv_snapshot_find(bs, &sn, name, id, false)) { + if (!bdrv_snapshot_find(bs, &sn, name, id)) { /* no need to set an error if snapshot doesn't exist */ return NULL; } @@ -2501,7 +2491,7 @@ SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name, bs = NULL; while ((bs = bdrv_next(bs))) { if (bdrv_can_snapshot(bs) - && bdrv_snapshot_find(bs, &sn, name, id, false)) { + && bdrv_snapshot_find(bs, &sn, name, id)) { bdrv_snapshot_delete(bs, sn.name, errp); if (error_is_set(errp)) { return NULL; @@ -2549,8 +2539,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict) while ((bs1 = bdrv_next(bs1))) { if (bdrv_can_snapshot(bs1) && bs1 != bs) { - if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str, - true)) { + if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str)) { available = 0; break; }
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- savevm.c | 33 +++++++++++---------------------- 1 file changed, 11 insertions(+), 22 deletions(-)