Re: [Xen-devel] [PATCH] xen: fail gnttab_grow_table() in case of missing allocations

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/09/17 10:28, Juergen Gross wrote:
> On 29/09/17 11:22, Andrew Cooper wrote:
>> On 29/09/17 10:02, Juergen Gross wrote:
>>> On 29/09/17 10:55, Roger Pau Monné wrote:
>>>> On Fri, Sep 29, 2017 at 05:39:22AM +0000, Juergen Gross wrote:
>>>>> In case gnttab_grow_table() is being called without grant_table_init()
>>>>> having been called for the domain, e.g. in case of a toolstack error,
>>>>> fail the function instead of crashing the system.
>>>>>
>>>>> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
>>>>> ---
>>>>>  xen/common/grant_table.c | 6 +++++-
>>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>>>>> index 71706f5cba..f2598a902f 100644
>>>>> --- a/xen/common/grant_table.c
>>>>> +++ b/xen/common/grant_table.c
>>>>> @@ -1669,7 +1669,11 @@ gnttab_grow_table(struct domain *d, unsigned int 
>>>>> req_nr_frames)
>>>>>      struct grant_table *gt = d->grant_table;
>>>>>      unsigned int i, j;
>>>>>  
>>>>> -    ASSERT(gt->active);
>>>>> +    if ( unlikely(!gt->active) )
>>>>> +    {
>>>>> +        gdprintk(XENLOG_WARNING, "grant_table_set_limits() call 
>>>>> missing.\n");
>>>> In the commit message you mention 'grant_table_init', yet the error
>>>> message here says grant_table_set_limits. Shouldn't both mention the
>>>> same precursor function?
>>> Aah, sorry. Will update the commit message.
>>>
>>>> Also I think this might be better as gprintk instead of gdprintk.
>>> Okay.
>>>
>>>>> +        return 0;
>>>> This return 0 has confused me, I was going to ask to return ENODEV,
>>>> but then I saw this is actually treated like a boolean. Might be good
>>>> to make gnttab_grow_table return bool, or either make it return proper
>>>> error codes.
>>> I wanted to keep the patch small.
>>>
>>> I can either send a cleanup patch after 4.10 or do it in this patch (in
>>> which case I'd like to know whether this patch or the last one of my
>>> grant series will be applied first).
>>>
>>> Thoughts?
>> This function is new in 4.10.
> Umm, not really. Its rather old.

Oh - so it is.

>
>> I'm not concerned about absolute size of
>> the patch, but I would prefer not to introduce new (mis)uses of int
>> functions which actually have boolean properties.  (Wei and I have spent
>> a large quantity of time removing functions like this.)
> I'd rather return a proper error code.

Fine by me.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel