From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37074) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1USzDU-0004bu-FA for qemu-devel@nongnu.org; Thu, 18 Apr 2013 20:30:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1USzDT-0006LP-0g for qemu-devel@nongnu.org; Thu, 18 Apr 2013 20:30:16 -0400 Received: from e23smtp05.au.ibm.com ([202.81.31.147]:50243) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1USzDS-0006L7-71 for qemu-devel@nongnu.org; Thu, 18 Apr 2013 20:30:14 -0400 Received: from /spool/local by e23smtp05.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 19 Apr 2013 10:25:12 +1000 Received: from d23relay05.au.ibm.com (d23relay05.au.ibm.com [9.190.235.152]) by d23dlp03.au.ibm.com (Postfix) with ESMTP id 46297357804E for ; Fri, 19 Apr 2013 10:30:05 +1000 (EST) Received: from d23av02.au.ibm.com (d23av02.au.ibm.com [9.190.235.138]) by d23relay05.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r3J0FvRW52428998 for ; Fri, 19 Apr 2013 10:15:57 +1000 Received: from d23av02.au.ibm.com (loopback [127.0.0.1]) by d23av02.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r3J0TYer025893 for ; Fri, 19 Apr 2013 10:29:34 +1000 Message-ID: <51708F9B.2050703@linux.vnet.ibm.com> Date: Fri, 19 Apr 2013 08:28:11 +0800 From: Wenchao Xia 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> <516FC95F.6010908@redhat.com> In-Reply-To: <516FC95F.6010908@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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: Pavel Hrdina Cc: armbru@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com 于 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. -- Best Regards Wenchao Xia