From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:37017) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RWqax-0001DK-6W for qemu-devel@nongnu.org; Sat, 03 Dec 2011 09:29:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RWqVd-0007gF-EB for qemu-devel@nongnu.org; Sat, 03 Dec 2011 09:24:19 -0500 Received: from mail-ee0-f45.google.com ([74.125.83.45]:63623) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RWqVc-0007fu-Sc for qemu-devel@nongnu.org; Sat, 03 Dec 2011 09:24:09 -0500 Received: by eeba50 with SMTP id a50so2076198eeb.4 for ; Sat, 03 Dec 2011 06:24:07 -0800 (PST) Sender: Paolo Bonzini Message-ID: <4EDA3104.8030304@redhat.com> Date: Sat, 03 Dec 2011 15:24:04 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <4ED98C06.5040405@codemonkey.ws> In-Reply-To: <4ED98C06.5040405@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 00/18] qom: dynamic properties and composition tree (v2) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: qemu-devel@nongnu.org On 12/03/2011 03:40 AM, Anthony Liguori wrote: > That is still true. The next step, inheritance, will pull the properties > into a base class. That base class can be used elsewhere outside of the > device model. > > But this is already a 20 patch series. If you want all of that in one > series, it's going to be 100 patches that are not terribly easy to > review at once. Without a design document and a roadmap, however, it's impossible to try to understand how the pieces will be together. 100 patches may require some time to digest, but 20 patches require a crystal ball to figure out what's ahead. >> Make sure that all required abstractions can be implemented in >> terms of a QOM composition tree. > > Not composition tree, you mean via the link graph. I mean both, but the link graph is already understandable (1-to-1 is easy). Right now I can hardly understand how the composition tree will work, for example: how do you constrain children to be subclasses of some class (or to implement some interface)? >> 2) Related to this, you have a lot of nice infrastructure, but (unlike >> what you did with QAPI) you haven't provided yet a clear plan for how >> to get rid of the old code. You also have only very limited uses of >> the infrastructure (see above). > > Old style properties go away as they're converted to new style properties. And how do they? How much code churn does that entail, in the devices and in general? In fact, why do they need conversion at all? Static properties are special enough to warrant something special. > If you want to see how this all is going to look, look at the old tree > :-) That's where we're going to end up. Let's write documentation on that already. >> It's better for various reasons--type safety and ease of use--even if >> it costs some boilerplate. For example the child property should be >> as simple as >> >> +struct ChildDeviceProperty { >> + DeviceProperty prop; >> + DeviceState *child; >> +} >> + >> +struct DevicePropertyInfo child_device_property_info = { >> + .size = sizeof(ChildDeviceProperty); >> + .get = qdev_get_child_property, >> +}; >> + >> void qdev_property_add_child(DeviceState *dev, const char *name, >> DeviceState *child, Error **errp) >> { >> gchar *type; >> >> type = g_strdup_printf("child<%s>", child->info->name); >> >> - qdev_property_add(dev, name, type, qdev_get_child_property, >> - NULL, NULL, child, errp); >> + prop = (ChildDeviceProperty *) >> + qdev_property_add(&child_device_property_info, >> + dev, name, type, errp); >> + >> + /* TODO: check errp, if it is NULL -> return immediately. */ >> + prop->child = child; > > This seems quite a bit uglier to me. I did say it costs some boilerplate, but it's once per type and you said there's a dozen of those. The type safety and more flexibility (an arbitrary struct for subclasses rather than an opaque pointer) is worth the ugliness. >> I think also that the type should not be part of DeviceProperty. >> Instead it should be defined by subclasses, with the >> DevicePropertyInfo providing a function to format the type to a >> string. > > I don't really know what you mean by this. Once the type is written as child, you've lost all info on it. Each property type should have its own representation for types (it can be implicit, or it can be an enum, or it can be a DeviceInfo), with a method to pretty-print it. > We jump too quickly at > doing conversions without thinking through the underlying interface. I hoped you had thought it through. ;) > You may find it odd to hear me say this, but I grow weary of adding too > much class hierarchy and inheritance with properties. Yes, we could make > it very sophisticated but in practice it doesn't have to be. Properties > should be one of two things: primitive types or link/childs. ... and interfaces. Accessing them by name as a property should work well. > This is where qdev goes very wrong. It mixes up user interface details > (how to format an integer on the command line) with implementation > details. There's no reason we should be indicating whether a property > should be display in hex or decimal in the device itself. That's totally true. But there's no reason why qdev properties cannot be split in two parts, a sane one and a legacy one. If you don't do this, you take the unmodifiable ABI and force its use as an API throughout all QOM. We can do better. >> For artificial properties you would need one-off classes; but you can >> still provide a helper that creates a specialized >> DeviceProperty+DevicePropertyInfo from the functions. I'm thinking of >> how people implement prototype-based OO on top of class-based OO, but >> it might be just a macro. > > I think you're over thinking the problem. There are going to be maybe a > dozen property types and that's it. They all with correspond exactly to > C types with the exception of links/children. I was thinking of one-off types such as rtc's struct tm. Also, besides C primitive types there will be property types for structs: for example virtio or SCSI requests, S/G lists, and so on. Paolo