BIND 10 #473: ModuleCCSession only validates config data, not commands

[BIND 10 Development]

#473: ModuleCCSession only validates config data, not commands
-------------------------------------+-------------------------------------
                 Reporter:  jelte    |                Owner:  jinmei
                     Type:           |               Status:  reviewing
  enhancement                        |            Milestone:
                 Priority:  major    |           Resolution:
                Component:           |            Sensitive:  0
  configuration                      |  Add Hours to Ticket:  0
                 Keywords:           |          Total Hours:  0
Estimated Number of Hours:  2.0      |
                Billable?:  1        |
                Internal?:  0        |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 proposed changelog:

 [bug] Jelte
       Command arguments were not validated internally against their
       specifications. This change fixes that.


 (comments left out were taken over and fixed)

 Replying to [comment:3 jinmei]:
 > '''General comments'''
 >  - how about the python part? shouldn't we validate commands in
 >    ccsession.py, too?  Note, however, that xfrin and zonemgr (and
 >    perhaps others) depend on "hidden" commands not listed in the
 >    formal spec.  anyway, if we want to validate commands in python,
 >    too, I think it should go to a separate ticket.
 >

 Yes, I think it was proposed a while ago to add the optional flag
 'public' (or otherway around, 'hidden') to commands, i.e. hide
 commands that are in the specification to user interfaces. If we have
 that we can add this there as well.

 > '''ccsession.cc'''
 >  - parseCommand
 >   - (not related to this change) is the comment above the function
 >     still valid?  it seems that if the given 'command' doesn't look like
 >     a command it throws an exception.  in any case, IMO this
 >     description (with correction if necessary) should be merged in the
 >     description in the header file.

 ohright, well removed it in favour of the now slightly modified
 documentation in ccsession.h

 >  - ModuleCCSession::checkCommand()
 >   - should the new logic move to a separate function, e.g.,
 >     handleCommand()?  this is largely a matter of preference, but due
 >     to the additional logic checkCommand() now looks quite long, and
 >     mostly beyond an understandable level at a glance.

 Yeah it was at about the limit :) Split it up, added two private
 methods checkConfigUpdateCommand, and checkModuleCommand

 >   - I hate to see a magic number:-)  What is this "3"? (in that sense
 >     I don't like the hardcoded "1" below this line, either)
 > {{{
 >                             answer = createAnswer(3, ss.str());
 > }}}
 >

 Any value that is not zero :)
 (yes we should formalize this and make a list of errors that can occur
 in internal messages that expect responses, but i think that is more
 general than just this and deserves its own ticket)

 > '''ccsession.h'''
 >  - (not directly related to this change) I think the description could
 >    be more detailed.  for example, an example would be useful.  I'd
 >    also to know which exceptions can be thrown, if any (and there are
 >    some, actually).

 You mean for the entire class? I'll do that separately then (so we can
 progress this ticket).

 >
 > '''module_spec.cc (): ModuleSpec::validate_command()'''
 >  - validate_command() should be named validateCommand() according to
 >    the guideline

 ack, there were more of those :p Fixed those as well.

-- 
Ticket URL: <http://bind10.isc.org/ticket/473#comment:5>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development