On 2019-11-16, Al Viro viro@zeniv.linux.org.uk wrote:
On Sat, Nov 16, 2019 at 11:27:52AM +1100, Aleksa Sarai wrote:
- error = nd_jump_link(&path);
- if (error)
path_put(&path);
- error = nd_jump_link(&ns_path);
- if (error)
path_put(&ns_path);
- error = nd_jump_link(&path);
- if (error)
path_put(&path);3 calls. Exact same boilerplate in each to handle a failure case. Which spells "wrong calling conventions"; it's absolutely clear that we want that path_put() inside nd_jump_link().
The rule should be this: reference that used to be held in *path is consumed in any case. On success it goes into nd->path, on error it's just dropped, but in any case, the caller has the same refcounting environment to deal with.
If you need the same boilerplate cleanup on failure again and again, the calling conventions are wrong and need to be fixed.
Will do.
And I'm not sure that int is the right return type here, to be honest. void * might be better - return ERR_PTR() or NULL, so that the value could be used as return value of ->get_link() that calls that thing.
I don't agree, given that the few callers of ns_get_path() are inconsistent with regards to whether they should use IS_ERR() or check for NULL, not to mention that "void *error" reads to me as being very odd given how common "int error" is throughout the kernel. There's also the "error == ERR_PTR(-EAGAIN)" checks which also read as being quite odd too.
But the main motivating factor for changing it was that the one use where "void *" is useful (proc_ns_get_link) becomes needlessly ugly because of the "nd_jump_link() can return errors" change:
error = ERR_PTR(nd_jump_link(&ns_path));
Or probably (if you don't want to rely on ERR_PTR(0) == NULL):
int err = nd_jump_link(&ns_path); if (err) error = ERR_PTR(err);