Re: [PATCH 0 of 3] create kvm_x86

Amit Shah
2007-11-21T09:32:17Z

* 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'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.

I was thinking the exact same thing. Having an arch-specific kvm as the
super-structure decouples 'struct kvm' from everything and it's extremely
clumsy to look at and program with. container_of() for getting struct kvm
might be better than getting the arch-specific thing.

>
> IIRC a downside was mentioned that it is easier to cause a build failure
> for another arch now.

How? I think it would work the same way in either case. Any arch-specific
stuff happen in kvm_arch and generic stuff in kvm.

>
> Opinions? In theory correctness should win over style every time, no?

But are those two conflicting here? And if you don't like what you're looking
at, it's going to be tougher to make it correct without making it look better

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.