Re: [Qemu-devel] [PATCH 4/7] s390x: fix realize inheritance for kvm-flic

[Cornelia Huck]

On Wed, 5 Jul 2017 13:11:59 +0200
Cornelia Huck <address@hidden> wrote:

> On Wed, 5 Jul 2017 12:20:42 +0200
> Christian Borntraeger <address@hidden> wrote:
> 
> > On 07/04/2017 04:51 PM, Halil Pasic wrote:  
> > > 
> > > 
> > > On 07/04/2017 04:37 PM, Cornelia Huck wrote:    
> > >> On Tue,  4 Jul 2017 16:07:56 +0200
> > >> Christian Borntraeger <address@hidden> wrote:
> > >>    
> > >>> From: Halil Pasic <address@hidden>
> > >>>
> > >>> Commit f6f4ce4211 ("s390x: add property adapter_routes_max_batch",
> > >>> 2016-12-09) introduces a common realize (intended to be common for all
> > >>> the subclasses) for flic, but fails to make sure the kvm-flic which had
> > >>> it's own is actually calling this common realize.    
> > >>
> > >> s/it's/its/
> > >>    
> > > 
> > > Valid. Sorry.
> > >     
> > >>>
> > >>> This omission fortunately does not result in a grave problem. The common
> > >>> realize was only supposed to catch a possible programming mistake by
> > >>> validating a value of a property set via the compat machine macros. 
> > >>> Since
> > >>> there was no programming mistake we don't need this fixed for stable.
> > >>>
> > >>> Let's fix this problem by making sure kvm flic honors the realize of its
> > >>> parent class.
> > >>>
> > >>> Let us also improve on the error message we would hypothetically emit
> > >>> when the validation fails.
> > >>>
> > >>> Signed-off-by: Halil Pasic <address@hidden>
> > >>> Fixes: f6f4ce4211 ("s390x: add property adapter_routes_max_batch")
> > >>> Reviewed-by: Dong Jia Shi <address@hidden>
> > >>> Reviewed-by: Yi Min Zhao <address@hidden>
> > >>> Signed-off-by: Christian Borntraeger <address@hidden>
> > >>> ---
> > >>>  hw/intc/s390_flic.c     |  4 ++--
> > >>>  hw/intc/s390_flic_kvm.c | 17 +++++++++++++++++
> > >>>  2 files changed, 19 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
> > >>> index a99a350..837158b 100644
> > >>> --- a/hw/intc/s390_flic.c
> > >>> +++ b/hw/intc/s390_flic.c
> > >>> @@ -101,8 +101,8 @@ static void s390_flic_common_realize(DeviceState 
> > >>> *dev, Error **errp)
> > >>>      uint32_t max_batch = 
> > >>> S390_FLIC_COMMON(dev)->adapter_routes_max_batch;
> > >>>  
> > >>>      if (max_batch > ADAPTER_ROUTES_MAX_GSI) {
> > >>> -        error_setg(errp, "flic adapter_routes_max_batch too big"
> > >>> -                   "%d (%d allowed)", max_batch, 
> > >>> ADAPTER_ROUTES_MAX_GSI);
> > >>> +        error_setg(errp, "flic property adapter_routes_max_batch too 
> > >>> big"
> > >>> +                   " (%d > %d)", max_batch, ADAPTER_ROUTES_MAX_GSI);   
> > >>>  
> > >>
> > >> Unrelated message change?
> > >>    
> > > 
> > > I've mentioned it in the commit message. It was also introduced by the
> > > patch I'm fixing. But yes strictly it's two different problems.    
> > 
> > I will only fix the patch description ( s/it's/its/) and keep the other 
> > things
> > unchanged. Is that fine with you?  
> 
> Yup. Let's get this done.
> 
> With the changes,

s/changes/change/

One coffee is obviously not enough...

> 
> Reviewed-by: Cornelia Huck <address@hidden>