From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NSdBF-00057L-1L for qemu-devel@nongnu.org; Wed, 06 Jan 2010 16:12:37 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NSdBA-00053a-Hi for qemu-devel@nongnu.org; Wed, 06 Jan 2010 16:12:36 -0500 Received: from [199.232.76.173] (port=49725 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NSdB9-00053N-RC for qemu-devel@nongnu.org; Wed, 06 Jan 2010 16:12:31 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53476) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NSdB3-0001eL-Ej for qemu-devel@nongnu.org; Wed, 06 Jan 2010 16:12:26 -0500 Date: Wed, 6 Jan 2010 23:12:21 +0200 From: Gleb Natapov Subject: Re: [Qemu-devel] [PATCHv3] add "info ioapic" monitor command Message-ID: <20100106211221.GC24476@redhat.com> References: <20100106163502.GQ4905@redhat.com> <4B44E97D.5090806@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4B44E97D.5090806@codemonkey.ws> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: qemu-devel@nongnu.org, lcapitulino@redhat.com On Wed, Jan 06, 2010 at 01:50:21PM -0600, Anthony Liguori wrote: > On 01/06/2010 10:35 AM, Gleb Natapov wrote: > >Knowing ioapic configuration is very useful for the poor soles > >how need to debug guest occasionally. > > This needs to be implemented in terms of VMState. There is no > reason for it not to be. Except that this has nothing to do with VMState. > > I've just gone through the code in great detail. You just need to > implement a vmstate_get_field that looks up a field given an > instance name and instance id. You then need to add a to_str > callback in VMStateInfo. The vmstate_get_field code will look very > much like the vmstate_save code except it calls to_str(). > > The new monitor command should be 'device_info name [instance_id] [field]'. > What is instance_id? And 'field' is implementation detail that should not be exposed to user at all. And what happens when VMState will start to support subsections? > For the ioapic, you can introduce a new VMStateInfo type that > duplicates the vmstate_info_uint64 type except it has a different > to_str callback that decodes the apic fields. It's just then a > matter of a few macros and updating the VMState description for the > ioapic. > You just overdesigning! What is the point in all those to_str() callbacks instead of only one that knows device internals? What if a meaning of one field depends on values of other fields? If you want to have a way to dump VMState then just dump it in a "field_name = value" form. This is great for migration debugging. But to present device state in human readable from is much simpler done by having only one function that gets device and prints nice info about its current state. If anything, the devise state print callback should be part of a device structure. > It's the same or less code than your current patch and it lays the > ground work for a consistent command to look up arbitrary device > info. > Your description is almost the size of my patch, and you can't seriously say that what you propose is "the same or less code than my current patch" since all of my patch will be a subset of what you propose, because all it does is parse ioapic state into readable form. And this is really what my patch is about: parsing ioapic state into readable form. Now you are saying that we can call this code from a different place (that doesn't exist currently), or we can spread it over multiple to_str functions, but it can't just disappear. -- Gleb.