Opened 10 years ago

Closed 10 years ago

#39 closed task (fixed)

review: src/bin/cmdctl

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. If there is already code review in process, please refer to the other ticket number.

Subtickets

Change History (6)

comment:1 Changed 10 years ago by zhanglikun

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

comment:2 Changed 10 years ago by jelte

  • Owner changed from zhanglikunq to jelte
  • Status changed from new to accepted

comment:3 Changed 10 years ago by jelte

  • Owner changed from jelte to zhanglikun
  • Status changed from accepted to assigned

First off, I made a few minor changes (for instance replacing "/" by os.sep in some path names) during this review, so I won't mention these further :) they're in rev. 1647)

In general it looks good (for now, keeping in mind the TODO file), I have a few suggestions for future work;

Documentation and docstrings need work (I can certainly do the docstrings part later, but I'd like to wait until after we merge back branches/trac90).

I'm quite unsure about the way cookies and session ids are used in the code, it's at a level where it works, but I have a loud nagging feeling that the security of it needs a really good looking at pretty soon. But IMO this does not have absolute priority right now, and I think this can be handled in a separate ticket).

Also, I think we should completely describe the full protocol that goes over HTTPS, all commands, the form, values and format of arguments, and those of responses. I suspect that once we have that we might see a lot of chances for improvements.

And an open question; do we want to use isc.config.ccsession.UICCSession and the corresponding isc.config.config_data.MultiConfigData? class to store module specifications and data? (currently all is stored in dicts, which are as good as equivalent but certainly not the same as ModuleSpec? classes which are contained in MultiConfigData?)

comment:4 Changed 10 years ago by jelte

Oh and I forgot one thing, all logic is in the .py.in file. Should we move it out and make the .in file a minimal one (like b10-cfgmgr.py.in)?

comment:5 Changed 10 years ago by zhanglikun

Note: I create a new ticket(ticket 127) for using http digest authentication to send username/password(from bindctl to cmdctl).

comment:6 Changed 10 years ago by jelte

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

copied to REVIEWED in r1717

Note: See TracTickets for help on using tickets.