BIND 10 #175: Loadzone: Add verbose options to exactly what is happening with loadzone
BIND 10 Development
do-not-reply at isc.org
Tue Jun 1 08:56:39 UTC 2010
#175: Loadzone: Add verbose options to exactly what is happening with loadzone
------------------------+---------------------------------------------------
Reporter: zhanglikun | Owner: zhanglikun
Type: task | Status: reviewing
Priority: major | Milestone: 04. 2nd Incremental Release: Early Adopters
Component: loadzone | Resolution:
Keywords: | Sensitive: 0
------------------------+---------------------------------------------------
Changes (by shane):
* owner: shane => zhanglikun
Comment:
I haven't ordered these items. Some are minor, some require quite a bit of
reworking. I'm not sure the best strategy given that we have a code freeze
today. :(
----
Can you double-check the TODO file? With this update:
* you can add origin to $INCLUDE
* any line can end with a comment (this is done by cleanup() in
master.py)
* the code now issues an error if it can't figure out the origin (rather
than using '.')
* the verbose option gives lots of information
I think the last two TODO items are still open.
----
I think that there should be a newline, "\n", at the end of the output
line:
{{{
Loading file "ttl1.db"
}}}
Otherwise this disappears immediately.
----
The output:
{{{
including "inclsub.db" from "include.db"
}}}
Should be capitalized:
{{{
Including "inclsub.db" from "include.db"
}}}
To make it consistent with the rest of the output.
----
I'm not so happy with how the tests work. I'd like to be able to run quick
tests, but that is not possible with the current testing framework.
* The tests do not run out of the source tree (maybe they make a
directory in /tmp and use that via the "-d" option?)
* There is no "pytest" target in Makefile.am (see
src/bin/bind10/Makefile.am for a possible example)
* I am not sure we should install these, so probably the EXTRA_DIST
should be removed from the Makefile.am
* Using "dig" to check the results means we have to have a server
running. I think it would be best to check these using the data source
API. It also means we need to have "xfrout" installed. I realize we don't
have a Python wrapper now so this is not easy, so I guess this is okay for
now.
* Typo in tests: extenstion -> extension
I think some of these can be fixed easily, and should be.
I see that there is a separate ticket for this, #174. I guess some minor
fixes from the above list should be done, and the rest of the work should
go on in that ticket (copying this there).
----
b10-loadzone.py.in:
The "Committing changes... done" output is bogus. The idea was to start
the output before committing, then finish it when the output was done:
{{{
sys.stdout.write("Committing changes...")
sys.sdtout.flush()
# do the work of committing changes... the stuff at the end of
sqlite3_ds.py
sys.stdout.write("done.\n")
}}}
We can either refactor the code to make the final write to disk a separate
function, or output directly from that code (not recommended), or simply
remove this output. I think that removing is the best option for now.
----
master.py:
Typo: formal -> format
The parse_ttl() function should probably use the re.findall() method
rather than re.split(), maybe something like:
{{{
def parse_ttl(s):
if not is_ttl(s):
raise MasterFileError('Invalid TTL: ' + s)
for ttl_expr in re.findall('\d+[wdhms]?', s, re.I):
ttl = ttl_expr[:-1]
suffix = ttl_expr[-1].lower()
if suffix == 'w':
.
.
.
}}}
It's simpler to read, I think.
In the `records()` function, I don't understand why the `oldsize` variable
is needed. Since size is never modified, can't you just do something like
the following?
{{{
for line in input:
size = len(line)
.
.
.
yield ret, size
}}}
There may be something about how yield works that I don't understand of
course...
In the `__init__()` function for the !MasterFile class, we should use
`fstat()` rather than `stat()`. There are two reasons for this:
1. The file may be overwritten with a new one in between the time the
`stat()` and `open()` are called.
1. More importantly, there may be an error with the `stat()` function.
All sorts of file system errors could happen here - permissions problems,
file not found, and so on. With `fstat()` you are much less likely to have
a problem.
I recommend removing the `stat()` call and replacing it with something
like:
{{{
try:
self.__zonefile = open(filename, 'r')
except:
raise MasterFileError("Could not open " + filename)
self.__filesize = os.fstat(self.__zonefile.fileno()).st_size
}}}
In the `__status()` function, you may have a problem if the file is 0
bytes. This is because of this line:
{{{
percent = (self.__cur * 100)/self.__filesize
}}}
This should probably be something like:
{{{
if self.__filesize == 0:
percent = 100
else:
percent = (self.__cur * 100)/self.__filesize
}}}
This same logic also applies to the division in the `zonedata()` function.
There should be a space after the word "second(s)", everywhere it is used:
{{{
10 RR(s) loaded in 0.44 second(s)(100.00% of ttl1.db)
^^^
a space here please
}}}
Using a separate thread to do the output is probably a bad idea. It's not
that complicated, but it is more complicated than is necessary, and can
lead to race conditions unless one is careful. For example, consider the
following case:
* The verbose() thread outputs 80 spaces
* The main thread takes over and outputs 80 spaces
* The verbose() thread outputs its update message
* The main thread outputs that a new include has arrived
In this case, the output is completely ugly and not at all what is wanted.
So I propose the code be refactored to not use threads:
* Remove the threading stuff from the `__status()` function
* Remove the verbose() function, and the call to it
* Remove the icky "\r%s % (80 * " ") output everywhere, which can be
better written as "\r" + (80 * " ") anyway. ;)
* In the `zonedata()` function, call `__status()` periodically:
{{{
def zonedata(self):
name = ''
last_status = 0.0
for record, size in records(self.__zonefile):
now = time.time()
if now - last_status >= 1.0:
self.__status()
last_status = now
}}}
The `__include()` function will not work as written. For example, if you
have a file name like: "This file name has spaces.db" then it will break.
I suggest something like:
{{{
__include_syntax1 = re.compile('\s+(\S+)(?:\s+(\S+))?$', re.I)
__include_syntax2 = re.compile('\s+"([^"]+)"(?:\s+(\S+))?$', re.I)
__include_syntax3 = re.compile("\s+'([^']+)'(?:\s+(\S+))?$", re.I)
def __include(self, s):
if not s.lower().startswith('$include'):
return "", ""
m = __include_syntax1.match(s)
if not m:
m = __include_syntax2.match(s)
if not m:
m = __include_syntax3.match(s)
if not m:
raise MasterFileError('Invalid $include format')
file = m.group(1)
if m.group(2):
if not isname(m.group(2))
raise MasterFileError('Invalid $include format (invalid
origin)')
origin = self.statedname(m.group(2), s)
else:
origin = self.__origin
return file, origin
}}}
The code uses class variables a lot, with `MasterFile.foo` instead of
instance variables, like `self.foo`. This will work, but effectively
defines a global variable. This is wrong. :)
It was in the old version of the code, and it was *sometimes* apropriate,
but not always. For example, `__maxttl` can be a class variable - although
it should be written in all caps since it is a constant, `__MAXTTL`.
However, `__ttl`, `__lastttl`, `__zonefile`, `__name`, and so on, are all
actually instance variables and should be declared as such. I think this
is okay for now, but I'd like to see a separate ticket opened to refactor
this.
This expression is used a lot:
{{{
MasterFile.__ttl or Masterfile.__lastttl
}}}
It might be better to make a function for this:
{{{
def getTTL(self):
return self.__ttl or self.__lastttl
}}}
Just to make it a bit shorter... not necessary though!
This code, at line 491:
{{{
# if not ttl:
# raise MasterFileError("No TTL specified; zone rejected")
}}}
Should be deleted.
--
Ticket URL: <https://bind10.isc.org/ticket/175#comment:6>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list