BIND 10 #40: review: src/bin/bind10

[BIND 10 Development]

#40: review: src/bin/bind10
--------------------------+-------------------------------------------------
 Reporter:  jreed         |        Owner:  jelte                                      
     Type:  task          |       Status:  reviewing                                  
 Priority:  blocker       |    Milestone:  04. 2nd Incremental Release: Early Adopters
Component:  Boss of BIND  |   Resolution:                                             
 Keywords:  review        |    Sensitive:  0                                          
--------------------------+-------------------------------------------------
Changes (by shane):

  * owner:  shane => jelte
  * status:  assigned => reviewing


Comment:

 Replying to [comment:2 jelte]:
 >
 > bind10.py.in:
 >
 > First off, weren't we supposed to move everything except the __main__,
 > option parsing, and signal catching outside of the .in file?

 This seems vaguely true. It also seems like a lot of work, so I've decided
 to document this as ticket #212 rather than implementing it.

 > If we were, this hasn't happened yet, and should go into TODO (it's
 > a non-essential, but non-trivial change)
 >
 >
 > 37: # TODO: start up statistics thingy
 >
 > That line can be removed (I did this originally then decided that it
 > would screw up line numbers later, so reverted that change)

 Removed.

 > I also don't get the joke at 53 :p

 It's not funny... just the name "BoB" made me think of it. Removed
 because, well, it's silly.

 > style nit: I personally prefer having the __init__ method be the first
 > one defined in every class. I'm not sure if there is an 'official'
 > order here, but if not I would like to propose to move the __init__ in
 > ProcessInfo up.

 I've moved this. It's not in our coding guidelines, but I will propose
 adding it on bind10-dev.

 > ProcessInfo also does not necessarily need op open() /dev/null (which
 > it always does right now)

 This is a class variable, so this is shared among all instances of the
 class, so it only gets opened once per process.

 I did change this to use os.devnull rather than the hard-coded
 "/dev/null".

 > 216: perhaps add a small comment here that session==none is checked
 > later (so this is not something to immediately fail on)

 Actually on that particular spot, we *want* session to be None. I think it
 has to be, but maybe a further check is necessary?

 I've added a comment to indicate that the exception is the "good" case.

 > 243: dead line of code

 Removed.

 > 258: that TODO has been done, so remove those 2 lines of comments
 > (not the rest! second todo is still todo)

 Removed.

 > I notice you sometimes use sys.stdout.write() and sometimes print(),
 > probably a good idea to replace one of those with the other.

 Personally, I never use print(). It's a habit I got when print() was a
 annoying built-in back in the old Python 2.x days.

 I've converted the print() into the One True Output method.

 > startup() is a LONG function. Split it up?

 It's long now because each process is hard-coded. I've created ticket #213
 to capture this issue.

 > 426-479: dead code

 Removed.

 > 493-496: dead code

 Removed.

 > 617: If we have integrated the ccsession queuing, we also need a
 > check_command() outside of the select loop (since ccs_fd may have no
 > data, but there might still be queued messages) But this is only
 > relevant once we also have a function somewhere that asks for specific
 > answers from the ccsession (since that is the only way messages get
 > queued)

 Does this exist yet?

 > bob.spec:
 >
 > We need to remove those fake commands :) (also from the implementation
 > then)

 Done and done.

 > TODO file:
 >
 > looks good, the first entry we'll have to do in another way though
 > (msgq cannot get its configuration from cfgmgr, this is one of those
 > 'hard config' things that is needed to even start the system) but
 > that is something for a different ticket.

 Yeah, put in Trac #213, as mention. I added this to the TODO file.

 Can you have a quick look and make sure this is sane, then I'll merge into
 the trunk.

 {{{
 $ svn diff -r 2003:2004
 }}}

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