Opened 10 years ago

Closed 9 years ago

#220 closed enhancement (complete)

suggestions to bindctl

Reported by: jinmei Owned by: zhanglikun
Priority: low Milestone: 05. 3rd Incremental Release: Serious Secondary
Component: ~bind-ctl (obsolete) Version:
Keywords: 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

See trac #219 for background.

Specific suggestions are:

  • I don't think it makes sense to expect a specific format as acceptable "parameter values" as it's very likely that we add more variations of parameters. I believe the command parser should only do minimal check like "parameter value is a sequecne of non space characters", "quoted string" is an string surrounded by quotation marks (such marks themselves can be part of the string in an escaped form).
  • I susepct "quota_str" and "QUOTA_PATTERN" are typo, and should be "quoted"/"QUOATED"
  • the error message is unfirendly. it should at least identify which part of the input causes the error. A better message would indicate what's expected in the erroneous part. Example:
> Xfrin retransfer master = ns.jinmei.org zone_name = jinmei.org
Error!  Invalid parameter value for "master".  It must be a numeric IPv6 (or IPv4 if you live in the 20th century) address.

These are not urgent. I'm giving this to Likun.

Subtickets

Change History (11)

comment:1 Changed 10 years ago by jinmei

  • Type changed from defect to enhancement

comment:2 Changed 10 years ago by shane

  • Milestone changed from feature backlog item to 05. 3rd Incremental Release: Serious Secondary

comment:3 Changed 10 years ago by zhanglikun

  • Owner changed from zhanglikun to jreed
  • Status changed from new to reviewing

The code has been committed to branches/trac220, r2108.

  1. Suggestion 1 and 3 have been done.

Since I refactored the function login_to_cmdctl() in class BindCmdInterpreter?, and Jeremy had add some code to it, so I plan to let Jeremy review my change first, then let jinmei check If I did as he suggested.

comment:4 Changed 10 years ago by jreed

I can't remember my related change to check. But I do have some comments for r2108:

  • what if HOME isn't defined?
  • FAIL_TO_CONNEC_WITH_CMDCTL has typo (CONNECT end with "T")
  • error message is not accurate "Cannot determine location of $HOME. Not storing default user" since there it is not checking for HOME. Maybe the check earlier for $HOME should also first try getpwnam (to get pw_dir) -- nevertheless I don't know python nor portable way to do that, but is more accurate than just relying on environment variable which may not be set by some automated tasks (non-login) for example.
  • also error "Cannot write ~/.bind10/default_user.csv; default user is not stored" -- may be more accurate if it printed the dir + file_name instead of hardcoded value

(I didn't run code yet.)

comment:5 follow-up: Changed 9 years ago by zhanglikun

Hi Jeremy, I have did the change according your review result, please see r2165.

comment:6 in reply to: ↑ 5 Changed 9 years ago by jreed

Replying to zhanglikun:

Hi Jeremy, I have did the change according your review result, please see r2165.

That looks fine to me. I didn't mean to remove $HOME support, but to just if it is not defined to use getpwnam. Nevertheless, it is fine the way it is now. Note I didn't do a complete code review. I did run cmdctl and bindctl though.

comment:7 Changed 9 years ago by jreed

  • Owner changed from jreed to zhanglikun

comment:8 Changed 9 years ago by zhanglikun

  • Owner changed from zhanglikun to jinmei

Hi Jinmei, would you like to have a review about the change I did before I merge them to trunk, since the suggestion was proposed by you, thank you.

comment:9 Changed 9 years ago by shane

  • Owner changed from jinmei to zhanglikun

This looks good. I have a few *more* suggestions to bindctl, but I put those in ticket #260 for future work.

Please merge.

comment:10 Changed 9 years ago by zhanglikun

The code has been committed to trunk in r2356, so close this ticket.

comment:11 Changed 9 years ago by shane

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

Thanks Likun, I'll close it.

Note: See TracTickets for help on using tickets.