On Mon, 2020-04-13 at 22:41 +0300, Dan Carpenter wrote: > On Mon, Apr 13, 2020 at 09:23:22AM -0400, Jeff Layton wrote: > > > > I don't see a problem with having a "free" routine ignore IS_ERR values > > > > just like it does NULL values. How about I just trim off the other > > > > deltas in this patch? Something like this? > > > > > > I think it encourages fragile code. Less so than functions that > > > return pointer, NULL or IS_ERR pointer, but still. You yourself > > > almost fell into one of these traps while editing debugfs.c ;) > > > > > > > We'll have to agree to disagree here. Having a free routine ignore > > ERR_PTR values seems perfectly reasonable to me. > > Freeing things which haven't been allocated is a constant source bugs. > > err: > kfree(foo->bar); > kfree(foo); > > Oops... "foo" wasn't allocated so the first line will crash. Every > other day someone commits code like that. > It's not clear to me whether you're advocating for Ilya's approach or mine (neither? both?). Which approach do you think is best? FWIW, my rationale for doing it this way is that the "allocator" for ceph_mdsc_free_path is ceph_mdsc_build_path. That routine returns an ERR_PTR value on failure, not a NULL pointer, so it makes sense to me to have the free routine accept and ignore those values. I don't quite follow the rationale that that encourages fragile code. -- Jeff Layton <jlayton@xxxxxxxxxx>
