On Mon, Feb 10, 2014 at 11:39:51PM +0100, Andreas Färber wrote:
> Am 10.02.2014 11:21, schrieb Eduardo Habkost:
> > +static const TypeInfo x86_cpu_host_type_info = {
> > + .name = CPU_CLASS_NAME("host"),
> > + .parent = TYPE_X86_CPU,
> > + .instance_size = sizeof(X86CPU),
> > + .instance_init = x86_cpu_instance_init_host,
> > + .abstract = false,
> > + .class_size = sizeof(X86CPUClass),
> > + .class_init = x86_cpu_class_init_host,
> > +};
>
> This looks broken: .class_data is not set but the loading of the cpudef
> happens in the TYPE_X86_CPU initfn. My preferred solution would be to
> move the cpudef-loading from TYPE_X86_CPU's instance_init to a separate
> one specified for the models only, allowing non-cpudef-based models. Not
> finished investigating yet.
class_data is not set because x86_cpu_class_init_host doesn't use it.
x86_cpu_class_init_host() simply points fills host_cpu_def and makes
xcc->cpu_def point to it. Why would we need a separate instance_init
only for the other models? x86_cpu_initfn() already works for everything
except the ->features field (that is then handled by
x86_cpu_instance_init_host()).
>
> For now I've prepended a patch implementing my generalized
> CPUClass::class_by_name instead of a custom x86_cpu_class_by_name().
Sounds good to me.
>
> Other style nits that I'm working on cleaning up are declarations in the
> middle of blocks, keeping _class_init naming convention (pretty sure my
> patches always had the most-specific-to-least-specific naming), strictly
> distinguishing between type and class, adding to my gtk-style
> documentation rather than new custom comments, placing struct
> documentation in the header and keeping the diff nicely readable AFAP.
> I'd further like to keep some other conventions from previous CPU
> subclasses, like pulling the model for loop out of the model
> registration function.
>
> My patches had always tried to turn what is now x86_cpu_load_def() into
> an instance_init function rather than calling it from one - did you have
> reasons not to?
I like to keep the "simply move stuff from X86CPUDefinition to X86CPU"
logic in a separate place. I think it makes the code more readable (and
it also made the code movements more obvious because I didn't have to
move all the code that's inside load_def(), just the load_def() call.
But if you want to inline it inside instance_init, I wouldn't mind.
>
> Did you consider converting the host model in a first step to make the
> patch smaller?
Converting the host model would require moving some logic to
instance_init but keeping the strcmp(name, "host") hack. I thought it
would involve more complex intermediate steps so I chose not to try it.
--
Eduardo
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list