On Fri, Dec 9, 2011 at 5:26 PM, David Sommerseth <openvpn.l...@topphemmelig.net> wrote: >> These changes, just to take into account some system somewhere that >> has a basename() implementation that modifies the argument string, are >> ugly, and prone to memleak-errors. Also, some other "basename(3)" man >> pages talk about basename() returning a pointer to an internal static >> storage which can be overwritten at the next call - so the code above >> would then not work on such a system (as the next call to basename() >> would modify the memory are where options->dev is pointing to). > > For the record, I did some valgrind checks on this change. And for some > odd reason, this doesn't leak. Even though --dev-node is explicitly set. > But it might be that the gc_* magic functions somehow catches this > allocation as well. > > Also note that init_options_dev() is only called once during the > execution, and this allocation *only* happens if --dev-node is used. > > And the only reason I say this is to try to avoid posting a v4 patch ;-) > >> So I'd propose to keep openvpn_basename() - it's simple and short >> enough, and has well-defined semantics that can be relied upon. > > The original openvpn_basename() is based upon the GNU basename() which is > not modifying any arguments, so this is definitely cleaner.
In my opinion, in order to avoid confusion there can be two options: 1. Use library functions as much as possible, providing the minimum common ground in compatibility layer if missing. This ensure applying fixes and functionality from various upstreams. 2. Use own implementation everywhere, taking responsibility and control over the implementation and maintenance. Mix is only confusing, and leads to even more errors. This also applied to related functions such as dirname/basename in this case, choosing own implementation of basename yield confusing as you do not expect that dirname taken from different implementation. Regards, Alon.
