Re: [PATCH v3] target/s390x: filter deprecated properties based on model expansion type

[David Hildenbrand]

On 26.07.24 21:11, Collin Walling wrote:
> On 7/26/24 3:15 AM, Markus Armbruster wrote:
> > Collin Walling <walling@linux.ibm.com> writes:
> >
> > > On 7/25/24 3:39 AM, David Hildenbrand wrote:
> > > > On 25.07.24 09:35, Markus Armbruster wrote:
> > > > > Markus Armbruster <armbru@redhat.com> writes:
> >
> > [...]
> >
> > > > > > Arguments that are silently ignored is bad interface design.
> > > > > >
> > > > > > Observe: when CpuModelInfo is an argument, @deprecated-props is always
> > > > > > ignored.  When it's a return value, absent means {}, and it can be
> > > > > > present only for certain targets (currently S390).
> > > > > >
> > > > > > The reason we end up with an argument we ignore is laziness: we use the
> > > > > > same type for both roles.  We can fix that easily:
> > > > > >
> > > > > >       { 'struct': 'CpuModel',
> > > > > >         'data': { 'name': 'str',
> > > > > >                   '*props': 'any' } }
> > > > > >
> > > > > >       { 'struct': 'CpuModelInfo',
> > > > > >         'base': 'CpuModel',
> > > > > >         'data': { '*deprecated-props': ['str'] } }
> > > > > >
> > > > > > Use CpuModel for arguments, CpuModelInfo for return values.
> > > > > >
> > > > > > Since @deprecated-props is used only by some targets, I'd make it
> > > > > > conditional, i.e. 'if': 'TARGET_S390X'.
> > > > >
> > > > > If we want just query-cpu-model-expansion return deprecated properties,
> > > > > we can instead move @deprecated-props from CpuModelInfo to
> > > > > CpuModelExpansionInfo.
> > > >
> > > > That might a bit more sense, because deprecated-props does not make any
> > > > sense as input parameter, for example.
> > >
> > > Will do.  Thanks for the feedback.  v4 in the works.
> >
> > We better get this into 9.1.  Plan B: mark @deprecated-props unstable to
> > avoid backward compatibility pain.
>
> Agreed, it would go a long way to squeeze this in before things get too
> messy.
>
> v4 is posted.  I think Thomas is unavailable, so if @David would not
> mind handling this?

Will do!

--
Cheers,

David / dhildenb