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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.  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.)

~Andrew

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