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

[BIND 10 Development]

#40: review: src/bin/bind10
--------------------------+-------------------------------------------------
 Reporter:  jreed         |        Owner:  shane                                            
     Type:  task          |       Status:  assigned                                         
 Priority:  blocker       |    Milestone:  02. Running, functional authoritative-only server
Component:  Unclassified  |   Resolution:                                                   
 Keywords:  review        |    Sensitive:  0                                                
--------------------------+-------------------------------------------------
Changes (by jelte):

  * owner:  jelte => shane


Comment:

 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?

 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)

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

 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.

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

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

 243: dead line of code

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

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

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

 426-479: dead code

 493-496: dead code

 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)


 bob.spec:

 We need to remove those fake commands :) (also from the implementation
 then)


 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.

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