On 2019-11-16, Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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); -- Aleksa Sarai Senior Software Engineer (Containers) SUSE Linux GmbH <https://www.cyphar.com/>
Attachment:
signature.asc
Description: PGP signature
