Re: [Qemu-arm] [Qemu-devel] [PATCH for-2.10 07/23] pc: add node-id property to CPU

[Eduardo Habkost]

On Thu, Apr 27, 2017 at 07:25:23PM +0200, Igor Mammedov wrote:
[...]
> > >  #
> > >  # A discriminated record of NUMA options. (for OptsVisitor)
> > >  #
> > > +# For 'cpu' type as arguments use a set of cpu properties returned
> > > +# by query-hotpluggable-cpus[].props, where node-id could be used
> > > +# to override default node mapping. Since: 2.10
> > > +#
> > >  # Since: 2.1
> > >  ##
> > >  { 'union': 'NumaOptions',
> > >    'base': { 'type': 'NumaOptionsType' },
> > >    'discriminator': 'type',
> > >    'data': {
> > > -    'node': 'NumaNodeOptions' }}
> > > +    'node': 'NumaNodeOptions',
> > > +    'cpu' : 'CpuInstanceProperties' }}
> > 
> > I worry about not being able to add extra options to "-numa cpu"
> > in the future without affecting HotpluggableCPU.props too. Being
> > able to document the semantics of -numa cpu inside a dedicated
> > NumaCpuOptions struct would be nice too.
> > 
> > I believe this can be addressed by defing "NumaCpuOptions" with
> > "CpuInstanceProperties" as base:
> > 
> >  { 'union': 'NumaOptions',
> >    'base': { 'type': 'NumaOptionsType' },
> >    'discriminator': 'type',
> >    'data': {
> >      'node': 'NumaNodeOptions',
> >      'cpu' : 'NumaCpuOptions' }}
> > 
> > ##
> > # Options for -numa cpu,...
> > #
> > # "-numa cpu" accepts the same set of cpu properties returned by
> > # query-hotpluggable-cpus[].props, where node-id could be used to
> > # override default node mapping.
> > #
> > # Since: 2.10
> > ##
> > { 'struct': 'NumaCpuOptions',
> >   'base': 'CpuInstanceProperties' }
> is it inheritance or encapsulation?

If I understood the docs correctly, it's inheritance. I didn't
test it, though.

> if it's encapsulation, wouldn't look nice, but we can
> duplicate fields from CpuInstanceProperties in NumaCpuOptions
> like you proposed below and marshal them into CpuInstanceProperties
> inside of parse_numa() where needed.

I think inheritance will work. But if it doesn't, I don't mind
either: we can duplicate the fields like you suggest, or use
CpuInstanceProperties directly like you did above.

-- 
Eduardo