Re: [PATCH v3 3/6] staging: r8188eu: add error handling of rtw_read8

Pavel Skripkin
2021-08-27T08:23:02Z

On 8/27/21 11:14 AM, David Laight wrote:
> From: Dan Carpenter
>> Sent: 26 August 2021 12:21
> ...
>> > > > ...
>> > > > > + len += snprintf(page + len, count - len,
>> > > > > "rtw_read8(0x%x)=0x%x\n",
>> > > > > + proc_get_read_addr, (u8) tmp);
>> > > >
>> > > > That is broken.
>> > > >
>> > >
>> > > Don't get it, sorry. Previous code did exactly the same thing, but
>> > > didn't check if read() was successful.
>> >
>> > Look up the return value of snprintf().
>> >
>>
>> It's hard to understand what you are saying. I think you are confusing
>> libc snprintf with the kernel snprintf? In libc the snprintf function
>> can return negatives but in the kernel it cannot. This is not going
>> to change. Any code which checks for negative snprintf returns in the
>> kernel is wrong and should be fixed.
>>
>> Anyway, the code works fine. snprintf here is going to return a number
>> between 18-32 range. It's not going to overflow the PAGE_SIZE buffer.
>
> IIRC it is also in a loop ...
>
> Maybe, but the idiom is just broken.
> Largely the result of snprintf() is never the value you are looking
> for and should be ignored.
>

AFAIK, snprintf return value is largely used in sysfs at least.

$ grep -Ir "= snprintf" | wc -l
1200

Anyway, I will convert this dead code to sysfs interface and maybe
snprintf will go away...

Thank you for your help and comments

> Userspace fprintf() is even worse.
> If you care about the write failing you need to call fflush()
> and then ferror() (typically before fclose()).
>
> Fortunately I've never seen a 'must check' attribute on it.
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>

With regards,
Pavel Skripkin