Opened 10 years ago

Closed 10 years ago

#37 closed task (fixed)

review: src/bin/bindctl

Reported by: jreed Owned by: zhanglikun
Priority: very high Milestone: 02. Running, functional authoritative-only server
Component: Unclassified 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.

Subtickets

Change History (5)

comment:1 Changed 10 years ago by zhanglikun

Ready for review. code is in trunk revision 992, /src/bin/bindctl.

comment:2 Changed 10 years ago by shane

  • Owner changed from zhanglikun to jinmei
  • Status changed from new to assigned

comment:3 Changed 10 years ago by shane

  • Owner changed from jinmei to jelte

comment:4 Changed 10 years ago by jelte

  • Owner changed from jelte to zhanglikun

On the whole it's looking good, I have some ideas for after the Y1
checkpoint, but we'll talk about those then :)

I did add and modify quite a few docstrings, but didn't think those
needed discussion so I committed those changes directly.

It could do with more unit test code coverage, but i see that's already
in the todo.

Some specific comments about files (the ones not mentioned here are
fine):

bindctl-source.py:

version shouldn't that be a number? (or possibly even set from
configure)

bindcmd.py:

help also shows a version hardcoded in the string that should come from
configure

What does CONST_COMMAND_NODE do?

Do we really need a client-side PEM certificate? (which isn't read from
the right location right now, i think this should have to be either
<install-prefix>/etc/bind10/ or /var/bind10, and from source tree
if run from source)

If we have a default user, we also provide a way to override that,
also storing the password plaintext might not be a great idea :)

SSL error needs to be something understandable (this is in TODO
already)

We need to think of what exactly to do with responses received to
commands (currently it simply prints the map)

comment:5 Changed 10 years ago by jelte

  • Resolution set to fixed
  • Status changed from assigned to closed

copied to REVIEWED branch in r1716

Note: See TracTickets for help on using tickets.