Code critique xmlrpclib

[Nick Craig-Wood]

flagg <ianand0204 at gmail.com> wrote:
>  This xmlrpc server is designed to parse dns zone files and then
>  perform various actions on said files. \
>  It uses dnspython, and xmlrpclib
>   I'd like to know what some of the more experienced python users
>  think. Where I could improve code, make it more efficient, whatever.
>  All suggestions are welcome.  This is my first functional python
>  program,  and I have tons of questions to go along with whatever
>  suggestions.   How could I do sanity checking; right now nothing
>  checks the input to these functions, error-handling in this program is
>  almost non-existant how do you integrate decent error handling into a
>  piece of software......ill shut up now.

I made a few comments, some about error handling!  See below

>  import dns.zone
>  from time import localtime, strftime, time
>  import os, sys
>  from dns.rdataclass import *
>  from dns.rdatatype import *
>  from string import Template
>  import xmlrpclib
>  from SimpleXMLRPCServer import SimpleXMLRPCServer
>  from SimpleXMLRPCServer import SimpleXMLRPCRequestHandler
> 
> 
>  zoneDir = "/dns"
> 
>  def openZoneFile(zoneFile):
>      """
>      Opens a zone file.  Then reads in zone file to dnspython zone
>  object.
>      """
>      global zone
>      global host
>      global domain
>      domain = ".".join(zoneFile.split('.')[1:])
>      host = ".".join(zoneFile.split('.')[:1])
> 
>      try:
>              zone = dns.zone.from_file(zoneDir + domain + '.zone',
>  domain)
>      except dns.exception, e:
>              print e.__class__, e

Don't use global variables!

This function should probably return 3 things

    return zone, host, domain

And whenever you use it, say

    zone, host, domain = openZoneFile(xxx)

It should probably be a method of DNSFunctions.  I'd avoid setting
self.zone etc as that is making a different sort of global data which
I'd avoid too.

Also if the dns.zone.from_file didn't work (raises dns.exception) you
set the host and domain but the zone is left at its previous value
which probably isn't what you wanted.  I'd let the error propagate
which I think will do something sensible for the xmlrpc.

>  class DNSFunctions:
> 
>       # Not exposed to xml-rpc calls, used internally only.
>      def _writeToZoneFile(self, record):
>          """
>          Checks if zone file exists, if it does it write values to zone
>  file
>          """
> 
>          for (name, ttl, rdata) in zone.iterate_rdatas(SOA):
>              new_serial = int(strftime('%Y%m%d00', localtime(time())))
>              if new_serial <= rdata.serial:
>                  new_serial = rdata.serial + 1
>              rdata.serial = new_serial
> 
>          if os.path.exists(zoneDir + str(zone.origin) + "zone"):
>              f = open(zoneDir + str(zone.origin) + "zone", "w")
>              zone.to_file(f)
>              f.close()
>          else:
>              print "Zone: " + zone.origin + " does not exist, please
>  add first"

This should probably be raising an exception to be caught or
propagated back to the client via xmlrpc.

> 
>      def showRecord(self, record):
>          """
>          Shows a record for a given zone. Prints out
>          TTL, IN, Record Type, IP
>          """
> 
>          openZoneFile(record)
> 
>          try:
>              rdataset = zone.get_node(host)
>              for rdata in rdataset:
>                  return str(rdata)
>          except:
>              raise Exception("Record not found")
> 
> 
>      def changeRecord(self, record, type, target):
>          """
>          Changes a dns entry.
>          @param record: which record to chance
>          @param type:  what type of record, A, MX, NS, CNAME
>          @target: if CNAME target points to HOSTNAME, if A record
>  points to IP
>          """
>          openZoneFile(record)
>          try:
>              rdataset = zone.find_rdataset(host, rdtype=type)
>          except:
>              raise Exception("You must enter a valid record and type.
>  See --usage for examples")

Don't catch all exceptions - find out which exceptions are thrown.
Consider just letting it propagate - hopefully the find_rdataset error
is descriptive enough.

-- 
Nick Craig-Wood <nick at craig-wood.com> -- http://www.craig-wood.com/nick