Re: [PATCH 0 of 3] create kvm_x86

Hollis Blanchard
2007-11-28T21:15:56Z

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?

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

> 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?

--
Hollis Blanchard
IBM Linux Technology Center