On May 20, 2017, at 3:49 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> We have never used this "err = PTR_ERR(p);" assignment and it annoys
> static checkers.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
>
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 13e8c092d4d2..d6f82ce288f4 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -261,10 +261,8 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
> size_t offs = m->count;
> loff_t next = pos;
> p = m->op->next(m, p, &next);
> - if (!p || IS_ERR(p)) {
> - err = PTR_ERR(p);
> + if (!p || IS_ERR(p))
> break;
> - }
> err = m->op->show(m, p);
> if (seq_has_overflowed(m) || err) {
> m->count = offs;
I'm guessing the static analysis is reporting that "err" is unused, because
it is clobbered after the end of the loop?
if (likely(err <= 0))
break;
}
pos = next;
}
m->op->stop(m, p);
n = min(m->count, size);
err = copy_to_user(buf, m->buf, n);
if (err)
goto Efault;
Unfortunately, this code dates back to the primordial "1da177e4" commit at
the start of Git history, so history to see how it got into this state.
Presumably, copy_to_user() is called unconditionally to return all data read
so far to userspace, and in that case "err" doesn't matter - something was
returned to userspace. I was thinking that copy_to_user() should be skipped
if "n == 0", so that the original "err" value is returned, but it may be that
this is never true in the "Fill:" case (ugh, upper-case labels).
n = min(m->count, size);
if (n == 0)
goto Done;
The logic isn't very clear here, so it isn't clear what the right change is.
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP
