Re: [PATCH v1 3/8] s390/sclp: rework sclp boundary and length checks

[Collin Walling]

On 5/13/20 3:00 AM, Cornelia Huck wrote:
> On Tue, 12 May 2020 10:55:56 -0400
> Collin Walling <address@hidden> wrote:
> 
>> On 5/12/20 3:21 AM, David Hildenbrand wrote:
>>> On 09.05.20 01:08, Collin Walling wrote:  
> 
>>>> +static bool check_sufficient_sccb_len(SCCB *sccb, int size)  
>>>
>>> "has_sufficient_sccb_len" ?
>>>   
>>>> +{
>>>> +    MachineState *ms = MACHINE(qdev_get_machine());
>>>> +    int required_len = size + ms->possible_cpus->len * sizeof(CPUEntry);  
>>>
>>> Rather pass in the number of cpus instead. Looking up the machine again
>>> in here is ugly.  
>>
>> prepare_cpu_entries also looks up the machine again. Should I squeeze
>> in a cleanup where we pass the machine to that function too (perhaps
>> in the "remove SCLPDevice" patch)?
> 
> sclp_read_cpu_info() does not have the machine handy, so you'd need to
> move machine lookup there; but I think it's worth getting rid of
> duplicate lookups.
> 

Sounds good, then. I'll propose a change to patch 1 that removes the
unused SCLPDevice param and accepts a MachineState instead. Should make
things a bit cleaner :)

>>
>>>   
>>>> +
>>>> +    if (be16_to_cpu(sccb->h.length) < required_len) {
>>>> +        sccb->h.response_code = 
>>>> cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
>>>> +        return false;
>>>> +    }
>>>> +    return true;
>>>> +}
>>>> +
> 
> 


-- 
--
Regards,
Collin

Stay safe and stay healthy