Re: [PATCH 0 of 3] create kvm_x86

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.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 <cotte-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>
>>>
>>>   
>>>       
>> 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