From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:46374) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1USgWy-0000Vk-2U for qemu-devel@nongnu.org; Thu, 18 Apr 2013 00:33:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1USgWt-00022W-UX for qemu-devel@nongnu.org; Thu, 18 Apr 2013 00:33:08 -0400 Received: from e23smtp08.au.ibm.com ([202.81.31.141]:51839) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1USgWt-000223-5a for qemu-devel@nongnu.org; Thu, 18 Apr 2013 00:33:03 -0400 Received: from /spool/local by e23smtp08.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 18 Apr 2013 14:30:41 +1000 Received: from d23relay05.au.ibm.com (d23relay05.au.ibm.com [9.190.235.152]) by d23dlp03.au.ibm.com (Postfix) with ESMTP id 752C53578053 for ; Thu, 18 Apr 2013 14:32:53 +1000 (EST) Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay05.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r3I4IYm39896426 for ; Thu, 18 Apr 2013 14:18:36 +1000 Received: from d23av04.au.ibm.com (loopback [127.0.0.1]) by d23av04.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r3I4WBUM016135 for ; Thu, 18 Apr 2013 14:32:11 +1000 Message-ID: <516F773E.80108@linux.vnet.ibm.com> Date: Thu, 18 Apr 2013 12:31:58 +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> In-Reply-To: <516EE688.1030704@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: Eric Blake Cc: lcapitulino@redhat.com, Pavel Hrdina , qemu-devel@nongnu.org, armbru@redhat.com 于 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) -- Best Regards Wenchao Xia