Re: gearmand - PATH_MAX patch review

["Philipp A. Hartmann" <ph@sorgh.de>]

Samuel, Barry,

it's C++, therefore we should probably use appropriate C++ features
instead of platform-specific things like strdupa().

Alternative sketched below, untested of course.

On 18/06/12 09:50, Samuel Thibault wrote:
> Barry deFreese, le Mon 18 Jun 2012 00:20:17 -0400, a écrit :
>> +#ifdef _GNU_SOURCE
> 
> _GNU_SOURCE is not something you can test. It's the application which is
> supposed to define it. Test __GLIBC__ instead
> 
>> +        char *getcwd_buf= get_current_dir_name();

            std::string const getcwd_str = get_current_dir_name();
            char const *      getcwd_buf = getcwd_str.c_str();

>> +#else
>>          char buf[PATH_MAX];
>>          char *getcwd_buf= getcwd(buf, sizeof(buf));

The getcwd_buf pointer should probably be const as well.

>> +#endif
>>          throw libtest::fatal(LIBYATL_DEFAULT_PARAM,
>>                               "Unable to open pidfile in %s for: %s stderr:%s",
>>                               getcwd_buf ? getcwd_buf : "",
>>                               _running.c_str(),
>>                               _app.stderr_c_str());
>> +#ifdef _GNU_SOURCE
>> +        free(getcwd_buf);
>> +#endif

This hunk then not needed.  getcwd_str is cleaned up automatically.
This should work correctly, since the libtest::fatal constructor copies
the provided strings internally.

> AIUI, the throw call is like "return", i.e. free will not be reached,
> and thus getcwd_buf never freed. You can use strdupa to duplicate the
> string into the stack, and then free the buffer returned by
> get_current_dir_name before calling throw.

IMHO, this approach would clutter the code without any benefits.
In C++ you clean up your resources in appropriate destructors, which are
perfectly exception safe.

My 2¢,
  Philipp