From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:49284) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rd6DW-00089f-07 for qemu-devel@nongnu.org; Tue, 20 Dec 2011 15:23:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Rd6DU-0004Hr-Cv for qemu-devel@nongnu.org; Tue, 20 Dec 2011 15:23:17 -0500 Received: from e33.co.us.ibm.com ([32.97.110.151]:38164) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rd6DU-0004HJ-4e for qemu-devel@nongnu.org; Tue, 20 Dec 2011 15:23:16 -0500 Received: from /spool/local by e33.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 20 Dec 2011 13:23:13 -0700 Received: from d03av05.boulder.ibm.com (d03av05.boulder.ibm.com [9.17.195.85]) by d03relay03.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id pBKKMs3n067822 for ; Tue, 20 Dec 2011 13:22:59 -0700 Received: from d03av05.boulder.ibm.com (loopback [127.0.0.1]) by d03av05.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id pBKKMrcB007358 for ; Tue, 20 Dec 2011 13:22:53 -0700 Message-ID: <4EF0EE84.3090509@linux.vnet.ibm.com> Date: Tue, 20 Dec 2011 14:22:28 -0600 From: Michael Roth 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> <4EF09C14.9070102@redhat.com> In-Reply-To: <4EF09C14.9070102@redhat.com> 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: Paolo Bonzini Cc: qemu-devel , Juan Quintela On 12/20/2011 08:30 AM, Paolo Bonzini wrote: > 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). The main goal is to abstract away data serialization schemes (QObject->JSON, C->QEMUFile, etc). In the case of a JSON-based serialization, the visitor interface for fixed-with types would end up serializing everything as int64_t/double, but QEMUFile requires byte-length affinity to remain backward-compatible, so that information must be passed on to the Visitor interface when we call it. And beyond QEMUFile, we'd like to eventually move to a serialization scheme that is self-describing in the types of the fields it stores so that we can do stricter checking in the deserialization/input visitor routines, or just plain be able to make sense of the serialized data without any outside information, since those schemes would eventually be used directly in implementing a new migration wire protocol and/or device state introspection. > > 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. And these are both requirements to implementing a robust, flexible serialization/Visitor interface with pluggable back-ends, but if those interface throw away the type/field names then the only way to get them back is to deserialize, which isn't useful for introspection, and volatile for migration (since type errors can be silently missed in a lot of cases) > > 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. Agreed, it's just a matter of wanting to maintain that information from start to finish. > > 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. Yup, that's the plan. These patches are a bit lazy in that regard. I agree that if we get into the habit of adding interfaces for a specific back-end without mapping these to the base-class implementations in other backends things will get out of hand quickly. Fortunately we haven't yet hit a situation where one backend ends up adding an interface that other backends cant' handle in some form. > > 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 mean, for instance, put_int32()/get_int32_equal()? If so, I'm not sure I follow. In that case we use a Visitor purely to serialize/deserialize an int32, vmstate adds the *_equal() interface as helper function on top of that, but it's not part of the Visitor interfaces. > >>> 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). I'll fix up the existing non-QEMUFile Visitor backends with base-class implementations for all the new interfaces. Beyond that, is there anything else missing to achieve 1)? > > Paolo >