Re: [PATCH 11/14] staging: android: ion: Allow heap name to be null

[Andrew F. Davis]

On 1/16/19 9:28 AM, Brian Starkey wrote:
> Hi Andrew,
> 
> On Fri, Jan 11, 2019 at 12:05:20PM -0600, Andrew F. Davis wrote:
>> The heap name can be used for debugging but otherwise does not seem
>> to be required and no other part of the code will fail if left NULL
>> except here. We can make it required and check for it at some point,
>> for now lets just prevent this from causing a NULL pointer exception.
> 
> I'm not so keen on this one. In the "new" API with heap querying, the
> name string is the only way to identify the heap. I think Laura
> mentioned at XDC2017 that it was expected that userspace should use
> the strings to find the heap they want.
> 

Right now the names are only for debug. I accidentally left the name
null once and got a kernel crash. This is the only spot where it is
needed so I fixed it up. The other option is to make the name mandatory
and properly error out, I don't want to do that right now until the
below discussion is had to see if names really do matter or not.

> I'd actually be in favor of making the string a more strict UAPI than
> allowing it to be empty (at least, if heap name strings is the API we
> decide on for identifying heaps - which is another discussion).
> 

I would think identifying heaps by name would be less portable, but as
you said that is a whole different discussion..

Thanks,
Andrew

> Cheers,
> -Brian
> 
>>
>> Signed-off-by: Andrew F. Davis <afd@xxxxxx>
>> ---
>>  drivers/staging/android/ion/ion.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
>> index bba5f682bc25..14e48f6eb734 100644
>> --- a/drivers/staging/android/ion/ion.c
>> +++ b/drivers/staging/android/ion/ion.c
>> @@ -467,7 +467,7 @@ static int ion_query_heaps(struct ion_heap_query *query)
>>  	max_cnt = query->cnt;
>>  
>>  	plist_for_each_entry(heap, &dev->heaps, node) {
>> -		strncpy(hdata.name, heap->name, MAX_HEAP_NAME);
>> +		strncpy(hdata.name, heap->name ?: "(null)", MAX_HEAP_NAME);
>>  		hdata.name[sizeof(hdata.name) - 1] = '\0';
>>  		hdata.type = heap->type;
>>  		hdata.heap_id = heap->id;
>> -- 
>> 2.19.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel