From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 0 of 3] create kvm_x86 Date: Fri, 30 Nov 2007 09:26:15 +0200 Message-ID: <474FBB17.6080800@qumranet.com> References: <4743F5AE.8090707@de.ibm.com> <4743F7DF.4000107@qumranet.com> <1196284556.9247.22.camel@basalt> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: carsteno-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org, kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org, kvm-ppc-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org, Xiantao To: Hollis Blanchard Return-path: In-Reply-To: <1196284556.9247.22.camel@basalt> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: kvm.vger.kernel.org Hollis Blanchard wrote: > On Wed, 2007-11-21 at 11:18 +0200, Avi Kivity wrote: > >> Carsten Otte wrote: >> >>> Hollis Blanchard wrote: >>> >>> >>>> These patches are based on Xiantao's work to create struct kvm_x86. Patch 1 replaces his "KVM Portability split: Splitting kvm structure (V2)", and patches 2 and 3 build on it. >>>> >>>> >>> Looks like a clean approach with to to_kvm_x86 macro. Whole series: >>> Acked-by: Carsten Otte >>> >>> >>> >> Well, I hate to say it, but the resulting code doesn't look too well >> (all the kvm_x86 variables), and it's entirely my fault as I recommended >> this approach. Not like it was difficult to predict. >> > > I guess we still have reached no conclusion on this question? > > Right. Thanks for re-raising it. >> I'm thinking again of >> >> struct kvm { >> struct kvm_arch a; >> ... >> } >> >> Where each arch defines its own kvm_arch. Now the changes look like a >> bunch of "kvm->blah" to "kvm->a.blah" conversions. >> > > The simplest "container" changes would be a bunch of "kvm->blah" to > "to_x86(kvm)->blah" conversions. How is that worse? If it's the > "kvm_x86" variables you're objecting to, it would be easy enough to > remove them in favor of this approach. > > It's horribly obfuscated. I'm accessing a member of a structure but that is hidden in a no-op function call. >> IIRC a downside was mentioned that it is easier to cause a build failure >> for another arch now. >> >> Opinions? In theory correctness should win over style every time, no? >> > > Which approach is not correct? > > The nicer one: struct kvm { struct kvm_arch arch; // common fields } or the similar struct kvm { struct kvm_common s; // arch specific fields } "not correct" is an exaggeration; more prone to breaking the build is more accurate. Maybe we can set up an hourly cross-compile to compensate. Code clarity is important to me. -- Do not meddle in the internals of kernels, for they are subtle and quick to panic. ------------------------------------------------------------------------- SF.Net email is sponsored by: The Future of Linux Business White Paper from Novell. From the desktop to the data center, Linux is going mainstream. Let it simplify your IT future. http://altfarm.mediaplex.com/ad/ck/8857-50307-18918-4