Opened 10 years ago

Closed 9 years ago

#40 closed task (complete)

review: src/bin/bind10

Reported by: jreed Owned by: shane
Priority: very high Milestone: 04. 2nd Incremental Release: Early Adopters
Component: ~Boss of BIND (obsolete) Version:
Keywords: review Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: Add Hours to Ticket:
Total Hours: Internal?:

Description

Review what is in trunk as of revision 738 following code review procedures. If this is your code, please add notes here as appropriate. I initially assigned this to the code owner, but this ticket will be assigned to a reviewer. If there is already code review in process, please refer to the other ticket number.

Subtickets

Change History (7)

comment:1 Changed 10 years ago by shane

  • Owner changed from shane to jelte
  • Status changed from new to assigned

I was thinking there should be tests before review, but perhaps having review is better than not being reviewed.

comment:2 follow-up: Changed 10 years ago by jelte

  • Owner changed from jelte to shane

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.

comment:3 Changed 9 years ago by shane

  • Milestone changed from 02. Running, functional authoritative-only server to 04. 2nd Incremental Release

comment:4 Changed 9 years ago by shane

  • Component changed from Unclassified to Boss of BIND

comment:5 in reply to: ↑ 2 Changed 9 years ago by shane

  • Owner changed from shane to jelte
  • Status changed from assigned to reviewing

Replying to 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

comment:6 Changed 9 years ago by jelte

  • Owner changed from jelte to shane

Lookin' good. Ship it ;)

Actually the reason I mentioned the long startup function was that if we refactor it correctly the amount of work to run modules from configuration would be less (coz we need it for that anyway). But oh well :)

comment:7 Changed 9 years ago by shane

  • Resolution set to complete
  • Status changed from reviewing to closed

Okay, merged into trunk. Closing ticket (finally!).

Note: See TracTickets for help on using tickets.