From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55466) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1USlz6-0005BP-Vx for qemu-devel@nongnu.org; Thu, 18 Apr 2013 06:22:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1USlz2-0002w5-Ls for qemu-devel@nongnu.org; Thu, 18 Apr 2013 06:22:32 -0400 Received: from mx1.redhat.com ([209.132.183.28]:31781) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1USlz2-0002vY-Bi for qemu-devel@nongnu.org; Thu, 18 Apr 2013 06:22:28 -0400 Message-ID: <516FC95F.6010908@redhat.com> Date: Thu, 18 Apr 2013 12:22:23 +0200 From: Pavel Hrdina MIME-Version: 1.0 References: <13b9d1e79947b89982ec51c421b9b1bd0a7b587d.1366127809.git.phrdina@redhat.com> <516E0E9A.9050607@linux.vnet.ibm.com> <516E54AF.8070206@redhat.com> <516E774D.8080809@linux.vnet.ibm.com> <516E7EB1.8060806@redhat.com> <516EE688.1030704@redhat.com> <516F773E.80108@linux.vnet.ibm.com> In-Reply-To: <516F773E.80108@linux.vnet.ibm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 11/11] savevm: remove backward compatibility from bdrv_snapshot_find() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wenchao Xia Cc: armbru@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com On 18.4.2013 06:31, Wenchao Xia wrote: > =E4=BA=8E 2013-4-18 2:14, Eric Blake =E5=86=99=E9=81=93: >> On 04/17/2013 04:51 AM, Pavel Hrdina wrote: >>> On 17.4.2013 12:19, Wenchao Xia wrote: >>>> =E4=BA=8E 2013-4-17 15:52, Pavel Hrdina =E5=86=99=E9=81=93: >>>>> 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 tha= t >> 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 H= MP >> 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 showe= d >> 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 requir= es >> 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 cod= ed >> 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 withou= t >> extra effort (ability to access id 3 without having to change the >> command line). It keeps the aspect of Pavel's code that specifying bo= th >> 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 differen= t >> 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=20 Wenchao that internal function needs clear meaning. Why would I need to=20 find snapshot by id using name parameter if I have the id parameter? I=20 know that name is in almost all cases some string and not a number and=20 that it could be for some cases easier just find snapshot by id using=20 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 erro= r >>> 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 reasonabl= e >> 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=20 accepted and applied upstream. I'll send today the v2 and we will see. Pavel