----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20955/#review43288 -----------------------------------------------------------
Ship it! Thanks for fixing the thread safety issues! Just some minor cleanup related things, feel free to commit as I won't be taking another look. 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp <https://reviews.apache.org/r/20955/#comment77382> How about 'size' instead of 'bufsize'? Here and below. Also, how about avoiding the assignment and equality together in the 'if': int size = sysconf(_SC_GETPW_R_SIZE_MAX); if (size == -1) { return ...; } 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp <https://reviews.apache.org/r/20955/#comment77381> From the opengroup page, it seems like errno wouldn't actually be set here? "A call to sysconf(_SC_GETPW_R_SIZE_MAX) returns either -1 without changing errno or an initial value suggested for the size of this buffer." http://pubs.opengroup.org/onlinepubs/9699919799/functions/getpwnam.html 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp <https://reviews.apache.org/r/20955/#comment77383> How are you able to allocate on the stack when bufsize is determined at runtime via sysconf? Did this compile? Looks like this buffer needs to be on the heap. 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp <https://reviews.apache.org/r/20955/#comment77384> We'll avoid the '*' against the variable as opposed to the type, and mixing in assignment in this statement is a bit esoteric for our code. Can you split these up: passwd passwd; passwd* result; 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp <https://reviews.apache.org/r/20955/#comment77385> Can you wrap comments at 70 characters going forward? At least we've documented this one ;) https://github.com/apache/mesos/blob/master/docs/mesos-c%2B%2B-style-guide.md#comments 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp <https://reviews.apache.org/r/20955/#comment77386> Ditto in this method for my comments above. 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp <https://reviews.apache.org/r/20955/#comment77387> Please drop a TODO to refactor this to return a Try and not log internally. 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp <https://reviews.apache.org/r/20955/#comment77389> Ditto comments above. 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp <https://reviews.apache.org/r/20955/#comment77390> The result == NULL case is not possible here, right? Should we differentiate that by printing out a different message? 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp <https://reviews.apache.org/r/20955/#comment77378> Please add the include for ostringstream and use '_' as suffixes in general :) - Ben Mahler On May 16, 2014, 10:08 p.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20955/ > ----------------------------------------------------------- > > (Updated May 16, 2014, 10:08 p.m.) > > > Review request for mesos, Ben Mahler, Jie Yu, and Vinod Kone. > > > Repository: mesos-git > > > Description > ------- > > These will be used for a later review where I need to pass uid and gid to a > forked child which will use the async signal safe ::setgid and ::setuid. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp > 38756617ead1b6651e44ab989871ea3e68d130df > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp > 3e511355e1faf7160442799e1e08d59b6f3b2f37 > > Diff: https://reviews.apache.org/r/20955/diff/ > > > Testing > ------- > > > Thanks, > > Ian Downes > >
