From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=52172 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OnYJk-0006LF-5N for qemu-devel@nongnu.org; Mon, 23 Aug 2010 10:48:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OnY5I-0001y2-1U for qemu-devel@nongnu.org; Mon, 23 Aug 2010 10:33:13 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45466) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OnY5H-0001xn-RW for qemu-devel@nongnu.org; Mon, 23 Aug 2010 10:33:12 -0400 Message-ID: <4C72869A.3030302@redhat.com> Date: Mon, 23 Aug 2010 17:32:58 +0300 From: Avi Kivity MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup References: <4C6D86F9.3010602@codemonkey.ws> <4C6D98E7.9020109@codemonkey.ws> <4C6DA75D.40303@codemonkey.ws> <4C6ECBB7.7060608@codemonkey.ws> <4C718865.7010807@redhat.com> <4C719080.4030202@codemonkey.ws> <4C720B1F.3030206@redhat.com> <4C727646.3040903@codemonkey.ws> <4C727ACC.7080501@redhat.com> <4C727C43.2040704@codemonkey.ws> <4C727EF5.6060402@redhat.com> <4C72851E.50405@codemonkey.ws> In-Reply-To: <4C72851E.50405@codemonkey.ws> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Blue Swirl , "Liu >> \"Liu, Jinsong\"" , qemu-devel , Markus Armbruster , Paul Brook On 08/23/2010 05:26 PM, Anthony Liguori wrote: > On 08/23/2010 09:00 AM, Avi Kivity wrote: >> On 08/23/2010 04:48 PM, Anthony Liguori wrote: >>>>> The fundamental issue is: every function (minus trivial ones) in >>>>> the device models code should have a state reference. That state >>>>> reference should inherit from a DeviceState. If this statement >>>>> isn't true, then the device has been modelled in qdev incorrectly. >>>>> >>>>> Using this test, quite a lot of the "converted" devices are being >>>>> modelled incorrectly. >>>> >>>> Is a "state reference" allowed to have a pointer to the state, or >>>> reach it in some other way (for example, static storage for >>>> singleton devices)? >>> >>> >>> No. If this was C++, then the statement would be: device have to be >>> implemented in terms of objects that inherit from Device. Device is >>> our common base object. >> >> so, >> >> struct MyDevicestate { >> struct DeviceState device_state; >> bool *some_bit; >> }; >> >> bad, while >> >> struct MyDevicestate { >> struct DeviceState device_state; >> bool some_bit; >> }; >> >> good? > > And the next logical question is whether: > > struct MyDeviceState { > DeviceState qdev; > OtherState *s; > }; > > Is bad? The answer would depend on whether OtherState implemented > methods or not. If OtherState has no methods, it's fine. If it has > methods, it's bad. I don't see why. As long as you can manipulate all of MyDevice's state via MyDeviceState methods, why do you care about OtherState at all? > >> >>> >>>> Isn't "save/restore works" an equivalent statement to "device state >>>> is reachable from the DeviceState"? >>> >>> I'm not sure I can connect the dots here as I'm not sure what >>> follows if your assertion is true. >> >> If save/restore works then all state is reachable from one point? >> Presumable DeviceState? >> >> I really don't see why the state has to be in the DeviceState >> subclass. We're probably talking past each other here due to some >> confusion in terms. > > Probably. Let's say that 'structs' are containers of primatives that > have no methods associated with them. 'objects' are structs that have > methods and potentially constructors/destructors. > > Devices can contain references to structs and objects. If a Device > contains a reference to an object, the object should be stored in a > BusState which is a container of Devices. Therefore, the object > should inherit from Device. I disagree. It's up to the author to decide whether to split a Device into 1 or 15 objects. If one of the other objects is also a subclass of DeviceState, then you're probably violating that DeviceState's contract. But that's a different (and trivial) matter. (side point: in C no objects have constructors and methods. in C++ all objects have constructors and methods, even PODs) -- error compiling committee.c: too many arguments to function