> On Sept. 25, 2019, 10:01 p.m., Benjamin Mahler wrote: > > 3rdparty/stout/include/stout/posix/dynamiclibrary.hpp > > Line 31 (original), 32 (patched) > > <https://reviews.apache.org/r/71388/diff/1/?file=2163054#file2163054line32> > > > > it's ok to dlclose with nullptr if close was already called or open was > > never called or open failed?
Ups, thanks for seeing that. https://stackoverflow.com/questions/11412943/is-it-safe-to-call-dlclosenull seems to suggest that `dlclose`'ing a null handle is likely not a good idea. I check for that explicitly now. > On Sept. 25, 2019, 10:01 p.m., Benjamin Mahler wrote: > > 3rdparty/stout/include/stout/posix/dynamiclibrary.hpp > > Line 46 (original), 36 (patched) > > <https://reviews.apache.org/r/71388/diff/1/?file=2163054#file2163054line47> > > > > Hm.. this pattern suggests a `Try<DynamicLibrary> create(path)` pattern > > to avoid the not-yet-opened or failed open states? I guess we may still > > want the explicit close though for error reporting.. Completely agree on `create`. As for `close` I am not sure we can model that with RAII or whether this might introduce issues in e.g., library unloading. Right now all prod users of this class leak handles. I didn't tackle this here since my primary concern was to allow move construction (and above sugggestion likely would also involve lengthy rewrites of test code). > On Sept. 25, 2019, 10:01 p.m., Benjamin Mahler wrote: > > 3rdparty/stout/include/stout/posix/dynamiclibrary.hpp > > Lines 66 (patched) > > <https://reviews.apache.org/r/71388/diff/1/?file=2163054#file2163054line77> > > > > hm.. why is this needed? seems a little non obvious to me This is needed so we do not invoke the default deleter (which calls `dlclose`) when resetting `handle_`. I added a comment. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71388/#review217484 ----------------------------------------------------------- On Sept. 25, 2019, 11:23 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71388/ > ----------------------------------------------------------- > > (Updated Sept. 25, 2019, 11:23 p.m.) > > > Review request for mesos, Benjamin Mahler and Jan Schlicht. > > > Repository: mesos > > > Description > ------- > > This not only simplifies our implementation of `DynamicLibrary`, but > also removes the potential for accidental file descriptor leaks. > > > Diffs > ----- > > 3rdparty/stout/include/stout/posix/dynamiclibrary.hpp > 6a50592632246b06152bee177e933f65438c07ca > > > Diff: https://reviews.apache.org/r/71388/diff/2/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >
