Re: [PATCH v4 2/2] pinctrl: pinmux: Add pinmux-select debugfs file

[Drew Fustini]

On Thu, Feb 11, 2021 at 10:39:38AM +0300, Dan Carpenter wrote:
> On Wed, Feb 10, 2021 at 11:24:23PM -0800, Joe Perches wrote:
> > On Thu, 2021-02-11 at 10:11 +0300, Dan Carpenter wrote:
> > > On Wed, Feb 10, 2021 at 02:28:54PM -0800, Drew Fustini wrote:
> > > > +	ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
> > > > +	if (ret < 0) {
> > > > +		dev_err(pctldev->dev, "failed to copy buffer from userspace");
> > > > +		goto free_gname;
> > > > +	}
> > > > +	buf[len-1] = '\0';
> > > > +
> > > > +	ret = sscanf(buf, "%s %s", fname, gname);
> > > > +	if (ret != 2) {
> > > > +		dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> > > > +		goto free_gname;
> > > 
> > > We need a "ret = -EINVAL;" before the goto.  sscanf doesn't return error
> > > codes.  Normally we would write it like so:
> > > 
> > > 	if (sscanf(buf, "%s %s", fname, gname) != 2) {
> > > 		dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> > > 		ret = -EINVAL;
> > > 		goto free_gname;
> > > 	}
> > > 
> > > I'm going to write a Smatch check for this today.
> > 
> > It's a pretty frequently used style:
> > 
> > $ git grep -P '\w+\s*=\s+sscanf\b' | wc -l
> > 327
> 
> Yeah.  That's true.  I looked through a couple of those and they were
> fine.  (Sample size 2)  But the other format is more common.
> 
> $ git grep sscanf | grep = | wc -l
> 803
> 
> I have written a Smatch check to complain whenever we propogate the
> return value from sscanf.  I'll let you know tomorrow how that goes.
> 
> I should write another check which says "On this error path, we know
> sscanf was not equal to the value we wanted but we are still returning
> success".
> 
> regards,
> dan carpenter
> 

Thank you for comments regarding sscanf().  And also thank you for the
LF mentorship session on smatch this morning.  It helped me understand
it much better.

Based on further comments, it seems there are better ways for me to pull
the strings out of the write buffer, but I will keep this in mind if I
need to use sscanf() in the future.

-Drew