On Thu, Jul 18, 2019 at 1:25 PM Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote:
>
> git://git.linux-nfs.org/projects/trondmy/linux-nfs.git tags/nfs-for-5.3-1
This got a conflict with the debugfs "don't behave differently on
failures" changes in net/sunrpc/debugfs.c.
See commit 0a0762c6c604 ("sunrpc: no need to check return value of
debugfs_create functions") by Greg, but I suspect you were already
aware of this.
I did a hack-and-slash "remove the error handling", and the end result
looks sane. Except I left the "if the snprintf overflows" error
handling in place, even if nothing then cares about the returned
error.
I think my merge resolution makes sense, but I thought I'd mention it
in case you had something else in mind. Honestly, the snprintf()
checks in do_xprt_debugfs() look kind o fpointless, but the comment is
also wrong:
char link[9]; /* enough for 8 hex digits + NULL */
that comment was copied from the "name[]" array in
rpc_clnt_debugfs_register(), but it's bogus, since you actually use
len = snprintf(link, sizeof(link), "xprt%d", *nump);
on the thing.
And you know what? If you have so many links that "xprt%d" doesn't fit
in 8 chars plus NUL, maybe you don't really care?
But it's also worth noting that the whole snprintf() overflow check is
*wrong* to begin with. When you do
if (len > sizeof(link))
return -1;
you're testing the wrong thing entirely. The returned "len" is the
length that would have been printed _without_ the ending NUL
character, so you actually had a truncation even if it returns
"sizeof(link)" - because then the NUL character was written instead of
the last character.
So the overflow test *should* have been
if (len >= sizeof(link))
return -1;
but I suspect the correct thing to do is to just say "we don't care"
and remove that error check entirely. Same goes for the other case
("len > sizeof(name)").
At some point error handling doesn't actually add value, as long as
the error itself isn't fatal. And when the error handling itself is
wrong, it's doubly suspect.
But as mentioned, I did *not* remove this part of the error handling.
I only removed the debugfs parts. The error handling may be wrong, but
it is what it is, and it doesn't really matter.
Linus
