[bind10-dev] config experiment 2

[JINMEI Tatuya / 神明達哉]

At Tue, 06 Oct 2009 12:25:38 +0200,
Jelte Jansen <jelte at isc.org> wrote:

> > - would we like to provide an accessor (get/set) to a specific config
> >   node (element)?
> 
> set/get_config_part do this, or i don't understand what it is you mean here

Ah, I realized I misunderstood the API a bit, but the main point when
I mad this comment still stands, which is this: since
get_config_part() makes a copy of the original configuration, if we
want to modify that part of the config we need to do set_config_part
explicitly.  But one may rather want to do something like this:

  config->get_config_part2("/module").set_value("foo");
  (get_config_part2() returns a reference to the corresponding config
  element of the original config tree)

to update the value of the "/module" part of the original config.

Perhaps a principal policy of how to change the configuration is "get
a local copy, modify it, and then commit it".  If that is the policy I
see the design of the current API and I'm fine with that.

> > - getConfigPart() requires the caller to delete its return value,
> >   assuming getConfigPart() allocates memory using new.  I think such
> >   an implicit requirement should be avoided.  
> 
> hmm, see my response to the exception thing below

I saw it, but I'm not sure if this comment is to be discussed in that
context.  My point is that this interface is an easy source of memory
leak.  Admittedly this is probably a matter of coding style
preference, but I'd personally like to:

- avoid requesting the need for explicitly deleting/freeing things
  whenever possible.  With C++ techniques like RAAI support this
  approach.  Or we could use boost's shared pointers if we agree on
  the dependency on boost.  Or, if this doesn't have to be a deep
  copy, we could change the return value of get_config_part() to a
  reference to the actual element (see also above).
- if this cannot be avoided, ensure the entity (a method, an object,
  etc) that allocates memory is responsible for freeing it as much as
  possible.  With this current API, we could do, for example:

  Config* config_copy = new Config(get_config_reference(id));
  // modify config_copy, set it back to the tree if necessary.
  // note that get_config_reference() can return a const reference if you
  // don't want to let the caller modify the referenced object.
  delete config_copy;

  With boost, there could also be a hybrid approach:

  typedef boost::shared_ptr<Config> ConfigPtr;
  ConfigPtr config_copy = ConfigPtr(new Config(get_config_reference(id)));
  // modify config_copy, set it back to the tree if necessary.
  // no need to delete config_copy explicitly.

Again, I understand it's more or less a matter of style.  If the above
argument isn't convincing enough, please just move forward with the
original design.

> > - I didn't understand this method:
> >         /*
> >          * Adds an empty element to the children of the current node
> >          */
> >         void addChild(std::string name);
> 
> I don't handle the root node separately; the api functions are agnostic as to
> which part of the whole tree you are currently using; while everything could be
> done with
[...]

I realized I misunderstood it.  I now understand it, and it's okay for
me.

> > - use of exception:

> I agree that always throwing an exception here isn't right, but i'm
> not sure we are all in sync in when to do throw one.
> 
> IMHO, a user making a typo when specifying an identifier directly it
> should be an exception (as opposed to the user specifying a correct
> identifier which is optional and happens to not exist, in which case
> no exception should be thrown).
> 
> yes, that would mean that it is also my opinion that unrecoverable
> malformed DNS messages should result in an exception from the parser
> as well;

Okay.  It's certainly a matter of opinion, and there's no single right
approach.  And, to be clear, I myself don't have a strong opinion
either way, right now.  I made this comment based on my understanding
of what I thought was the preliminary design consensus.
 
I also agree that we don't necessarily have to have a strict policy of
when/how to (not) use exceptions.  Still, I think it's better to have
a general guideline to make the overall coding style consistent (which
I believe helps improve code maintainability).

This should probably be discussed in a more generic context.  I'll
send a separate message focusing on this topic later.

BTW, I've added an open item about this point in the "coding style"
wiki page:
http://bind10.isc.org/wiki/CodingGuidelines
(see "Exceptions")

> (snip rest of your comments, all good points, and done)
> 
> (except one)
> > - style issue: would we allow C-style comments?
> 
> dunno, i like em, but no problem for me to go c++ only.

This is also an open topic, and we might rather not bother to do with
this level of minor style policy; however, I'd still like to make the
style as consistent as possible if we can easily reach a consensus.
This is also probably a general discussion item.  (I've noted this
point in the wiki, too)

---
JINMEI, Tatuya