From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:34918) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rd0iX-0003RI-4e for qemu-devel@nongnu.org; Tue, 20 Dec 2011 09:31:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Rd0iR-0000cB-3X for qemu-devel@nongnu.org; Tue, 20 Dec 2011 09:30:57 -0500 Received: from mx1.redhat.com ([209.132.183.28]:13534) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rd0iQ-0000c7-Rm for qemu-devel@nongnu.org; Tue, 20 Dec 2011 09:30:51 -0500 Message-ID: <4EF09C14.9070102@redhat.com> Date: Tue, 20 Dec 2011 15:30:44 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1319735193-4718-1-git-send-email-mdroth@linux.vnet.ibm.com> <1319735193-4718-2-git-send-email-mdroth@linux.vnet.ibm.com> <4EF06D87.9000809@redhat.com> <4EF092A5.3070108@codemonkey.ws> In-Reply-To: <4EF092A5.3070108@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 01/10] qapi: add Visitor interfaces for uint*_t and int*_t List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Juan Quintela , qemu-devel , Michael Roth On 12/20/2011 02:50 PM, Anthony Liguori wrote: >> For saving, you would adapt your visitor-based vmstate "put" >> routines so that they put things in a dictionary with no regard for >> integer types (a bit ugly for uint64, but perfectly fine for >> everything else). > > I don't understand this. The visitor interface should expose the C > level primitives so that we can maintain fidelity when visiting > something. The fact that it only knows about "ints" today is a short > cut. Why does this need to be in Visitor? You can always embed C knowledge in an adaptor or decorator. Visitors only need to know about names and JSON types (well, they also distinguish int from double). We already have such an adaptor: QOM static properties know about names, JSON types, C type and struct offset. VMState fields know about all that plus QEMUFile encoding. QEMUFile encoding can be hidden in the decorator, it does not need to become visible to the concrete visitors. As always, you can implement that in many ways. However, I think the point of using Visitors is not to remove QEMUFile. It is to provide a backend-independent representation that backends can transform and that secondarily can be exposed by QOM. This is only half-true in Michael's code, because he relies on primitives that QMPInputVisitor and QMPOutputVisitor do not implement. Fixing this is quite easy, you only need to add a base-class implementation of the int8/int16/... primitives. On top of this the representation he passes to visitors is somewhat redundant. For example, VMState has "equal" fields; they are fields that are serialized but are really fixed at compile- or realize-time. Such fields should not be part of the backend-independent representation. With Michael's approach they are, and that's quite deep in the implementation. >> You take the dictionary from the output visitor and (with an input >> visitor) you feed it back to the "save" routines, which convert the >> dictionary to a QEMUFile. Both steps keep the types internal to >> vmstate. > > That doesn't make effective use of visitors. Visitors should preserve > as much type information as possible. I'm not really sure I > understand the whole QEMUFile tie in either. This series: > > 1) Makes a fully compatible QEMUFile input and output Visitor > > 2) Makes VMState no longer know about QEMUFile by using (1) > > (2) is really the end goal. If we have an interface that still uses > QEMUFile, we're doing something wrong IMHO. Yes, this is accurate, but I see the goals differently. We should: (1) First and foremost, provide a backend-independent representation of device state so that we can add other backends later. (2) Serialize this with QEMUFile, both for backwards-compatibility and to ensure that the whole thing works. Whether you do (2) directly with QEMUFile or, like Michael does, with QEMUFile*Visitors is secondary. I don't have big objections to either approach. However, the series is missing (1). Paolo