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

[Halil Pasic]

On 07/05/2017 12:20 PM, Christian Borntraeger 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?

It's fine with me, but I guess you probably asked Connie. Thanks!