Opened 10 years ago

Closed 10 years ago

#51 closed task (fixed)

Review b10-loadzone

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

It's checked in to branches/each-loadzone. Please review.

Subtickets

Change History (7)

comment:1 Changed 10 years ago by each

Oops, I forgot to mention branches/each-loadzone branched at r913.

comment:2 Changed 10 years ago by shane

  • Owner set to evan
  • Status changed from new to assigned

What code is supposed to be reviewed exactly?

shane@shane-asus-laptop:~/ISC/BIND10/svn/bind10/branches$ svn update
At revision 919.
shane@shane-asus-laptop:~/ISC/BIND10/svn/bind10/branches$ find each-loadzone/ -iname "*loadzone*"
each-loadzone/
shane@shane-asus-laptop:~/ISC/BIND10/svn/bind10/branches$

I don't see anything called b10-loadzone or anything like it...

comment:3 Changed 10 years ago by shane

  • Owner changed from evan to shane
  • Status changed from assigned to accepted

comment:4 follow-up: Changed 10 years ago by shane

  • Owner changed from shane to each
  • Status changed from accepted to assigned

Overall:

  • minimal or no checking, no safety net in face of invalid data (but this was the idea, so this is not a complaint, just an observation)
  • There are a lot of lines ending with semicolon. Try:
    grep '; *$' *
    

b10-loadzone.py:

  • do we need "-h", since it just does the same thing as -X
  • Should the usage go to stderr?
  • There is sys.exit() after usage(), but usage() also exits - pick one or the other
  • dbfile is /tmp/zone.sqlite3... ideally this would go to the configured location... I'm not sure how one would do such a thing
  • This construct seems odd to me:
    try:
        zonefile = args[0]
    except:
        usage()
    

I would write this as:

if len(args) != 1:
    usage()
zonefile = args[0]
  • This message:
    try:
        zone, zonedata = isc.auth.master.parse(zonefile, initial_origin)
    except Exception as e:
        print("Couldn't open database: " + str(e));
        exit(1)
    

Should probably say "error reading zone file".

sqlite3_ds.py:

  • FYI:
    class Sqlite3DSError(Exception):
        def __init__(self, value):
            self.value = value
        def __str__(self):
            return repr(self.value)
    

Can be written as:

class Sqlite3DSError(Exception):
    pass
  • According to the documentation, the sqlite3 Python module creates implicit BEGIN statements. Maybe we should document that we know and take advantage of this?

http://docs.python.org/library/sqlite3.html#sqlite3-controlling-transactions

  • Why do we do conn.commit() twice in load()?

master.py

  • See FYI above
  • The comment for isclass() says "istype"
  • Is the regex for isname() actually correct?
  • The comment for isttl() says "isname"
  • Does $ORIGIN actually use existing $ORIGIN in BIND 9? If so, then this is correct, otherwise it needs to be made to match.
  • Line numbers in errors might be nice, but not critical of course
  • In the two() function, what if the label name is a type?
        a A 1.2.3.4
    

comment:5 in reply to: ↑ 4 Changed 10 years ago by each

  • Owner changed from each to shane
  • do we need "-h", since it just does the same thing as -X

I prefer to be explicit; we may add arguments in the future and it would be bad to accidentally step on -h.

  • dbfile is /tmp/zone.sqlite3... ideally this would go to the configured location... I'm not sure how one would do such a thing

The current plan is, if you specify dbfile on the command line, it uses that; if not, it will essentially pretend to be an auth daemon and ask the configuration daemon where the dbfile is--but I'm waiting until the auth daemon has settled on a mechanism for this before I put it into loadzone. If the daemon isn't running, presumably loadzone could read the dbfile path directly from the config database, but I don't think there's an API for that yet. In any case, if nothing works at all, it will fail--we're not keeping the /tmp/zone.sqlite3 thing :)

  • According to the documentation, the sqlite3 Python module creates implicit BEGIN statements. Maybe we should document that we know and take advantage of this?

Sure. Where did you have in mind documenting this?

  • Why do we do conn.commit() twice in load()?

Because I want to separate the transaction where the new zone data are loaded from the one where the old data are deleted. (Thinking about, perhaps I should also commit after loading the data and before changing the zone name in the zones table...)

  • Is the regex for isname() actually correct?

I think so. Is there a specific concern you have? It should match legal names, which is a slightly more permissive standard than legal hostnames.

  • Does $ORIGIN actually use existing $ORIGIN in BIND 9? If so, then this is correct, otherwise it needs to be made to match.

Not sure I understand. You mean, if the existing origin is "bar." and I set $ORIGIN to "foo" without a trailing dot, does it become "foo.bar."? Yes.

  • Line numbers in errors might be nice, but not critical of course

How do you get the line number from python?

  • In the two() function, what if the label name is a type?

Good catch. That needs to be treated as a name unless the label name is RRSIG, I think.

Comments addressed except where noted above, see change 1077.

comment:6 Changed 10 years ago by shane

  • Owner changed from shane to each
* do we need "-h", since it just does the same thing as -X

I prefer to be explicit; we may add arguments in the future and it would be bad to accidentally step on -h.

Okay, no worries.

    * dbfile is /tmp/zone.sqlite3... ideally this would go to the configured location... I'm not sure how one would do such a thing

The current plan is, if you specify dbfile on the command line, it uses that; if not, it will essentially pretend to be an auth daemon and ask the configuration daemon where the dbfile is--but I'm waiting until the auth daemon has settled on a mechanism for this before I put it into loadzone. If the daemon isn't running, presumably loadzone could read the dbfile path directly from the config database, but I don't think there's an API for that yet. In any case, if nothing works at all, it will fail--we're not keeping the /tmp/zone.sqlite3 thing :)

Okay, this is a reasonable plan.

    * According to the documentation, the sqlite3 Python module creates implicit BEGIN statements. Maybe we should document that we know and take advantage of this?

Sure. Where did you have in mind documenting this?

Probably in a note at the top of sqlite3_ds.py would be the right place.

    * Why do we do conn.commit() twice in load()?

Because I want to separate the transaction where the new zone data are loaded from the one where the old data are deleted. (Thinking about, perhaps I should also commit after loading the data and before changing the zone name in the zones table...)

Okay, so the idea is that it's better to get the new data in, and then if the cleanup fails it's not a tragedy. I more-or-less agree.

    * Is the regex for isname() actually correct?

I think so. Is there a specific concern you have? It should match legal names, which is a slightly more permissive standard than legal hostnames.

I have no specific concern, I just have a hard time reading regular expressions, and wanted you to look at it again. :)

    * Does $ORIGIN actually use existing $ORIGIN in BIND 9? If so, then this is correct, otherwise it needs to be made to match.

Not sure I understand. You mean, if the existing origin is "bar." and I set $ORIGIN to "foo" without a trailing dot, does it become "foo.bar."? Yes.

That was my question, and since that is the answer I think the code is good. Insane, but good, in the WWB9D? (What Would BIND 9 Do?) sense.

    * Line numbers in errors might be nice, but not critical of course

How do you get the line number from python?

I meant the line number from the zone file that was being parsed. I think it would involve a non-trivial refactoring, so we can leave this for when we have a loader using the DNS library for parsing and the Data Source Update module.

    * In the two() function, what if the label name is a type?

Good catch. That needs to be treated as a name unless the label name is RRSIG, I think.

Okay, given these answers consider this reviewed.

If this is not yet in the trunk, please merge there.

If this is already in the trunk, then no merging needs to happen. Please let Jeremy know this is reviewed so he can move to the release tree, okay? Thanks!

comment:7 Changed 10 years ago by each

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

I believe Jeremy has merged this to the Y1 branch now.

Note: See TracTickets for help on using tickets.