On Wed, May 16, 2007 at 09:55:15PM -0400, Brad House wrote:
> Since the parameters aren't touched by ares_init_options() doesn't
> mean that all the members of the struct should be const. It
> means that the struct itself passed to ares_init_options() should
> be const.
The parameters of the only function (that I could find) that takes
an ares_options are always copied and never modified. i.e. All
receivers of an ares_options structure require only immutable access
to the pointed-to resources.
> Think of if someone is copying args from an initialized channel that
> may be closed before the options struct is used. If you didn't
> options.lookups = strdup(channel->lookups), and instead did
> options.lookups = channel->lookups , when the channel is
> destroyed, the pointer would disappear. And if you did strdup(),
> with your const settings, you wouldn't be able to free() that
> memory without first casting off the 'const', which is definitely
> considered a bad practice.
If, to prepare an ares_options structure to be passed into
ares_init_options(), I were to do
options.lookups = strdup(channel->lookups);
that wouldn't make much sense because the semantics of the call are
that the string pointed to by the lookups member is always copied.
Of course, if I needed to build up the pointed-to string dynamically,
then perhaps I have allocated a buffer to hold it dynamically (or
maybe not), but that is independent of the interface semantics of
ares_init_options() and it shouldn't be ares_options' job to support
the particular allocation strategy I've chosen. My allocation is my
problem and I need to manage it:
char *mine = /* dynamically allocate and build string */;
options.lookups = mine; /* ok even if lookups is "const char *" */
ares_init_options(...);
free(mine);
With this perspective, the ares_options structure is basically there
to allow ares_init_options() to easily take a large number of
arguments and the members of the structure should be of the same types
that normal function arguments would be.
> If you look at the patch I recently submitted, you'll see the
> use argument for this (ares_save_options() along with associated
> ares_destroy_options()), as well as marking the options parameter
> as const for ares_init_options().
I don't agree that the fact that you've chosen to overload the
ares_options structure for storage in your patch means that other
uses and other needs are invalid. The existing ares_options structure
does not include any ownership semantics, and giving it ownership in
some circumstances (i.e. when set ares_save_options()) but not others
(existing usage in channel initialization) seems problematic.
Making the options argument to ares_init_options() const is good,
because it guarantees that the function does not modify anything
directly stored in the structure. However, it does _not_ guarantee
that the pointed-to data (in the lookups string, for example) is not
modified, so it is insufficient to advertise that we can safely use
the same ares_options structure to initialize multiple channels:
struct ares_options
{
// ...
char *lookups;
};
// Are options and everything it points to always the same on exit?
void f(const struct ares_options * const options)
{
// Oops! This is allowed even though the options pointer is
// const and we only have immutable access to the structure.
options->lookups[0] = 'x';
}
By leaving lookups mutable, you're forcing people who only have
constant access to a string to copy it in order to pass it to a
function that never changes it! It means that code like this:
void start_ares(const char * const lookup_rules)
{
/* ... */
options.lookup = lookup_rules;
ares_init_options(...);
}
is an error. And, even worse, for C++ users (including me), it means
that
// Note: mutable std::string object but c_str()
// returns const char *
std::string lookup_rules("abc");
options.lookup = lookup_rules.c_str();
// or even
// String literals are of type const char[] in C++
options.lookup = "abc";
are both errors. :(
-- ---------------------------------------------------------- Brad Spencer - spencer@jacknife.org - http://jacknife.org/