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

[Juergen Gross <jgross@xxxxxxxx>]

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?


Juergen


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