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

[Pavel Skripkin]

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