Code critique xmlrpclib
Nick Craig-Wood
nick at craig-wood.com
Wed Feb 4 04:31:58 EST 2009
flagg <ianand0204 at gmail.com> wrote:
> On Feb 3, 7:32?am, Nick Craig-Wood <n... at craig-wood.com> wrote:
> > flagg <ianand0... 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.
> >
>
> Thanks for taking the time to reply I appreciate it.
No problems.
> Are global variables "bad"? Or just inefficient?
They make code a lot more difficult to follow because you have to
wonder where the variable has been set. If you create multiple
instances of DNSFunctions (more than likely in a multi-user server)
you'll find that global variables will cause your program to go wrong
quite badly!
In general only ever use global variables in quick and dirty scripts.
For static configuration which you might think globals are good for
you can easily do that in a module namespace or in a Config object
which you only have one of.
> Also when you say:
>
> "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."
>
> Could you elaborate on that. Im not following what you mean by
> "propagate"
I mean don't catch the exception. If you raise an exception while in
an RPC it will get propagated back to the client which is exactly what
you want.
Eg, run this in one window
import SimpleXMLRPCServer
class SimpleTest:
def divide(self, x, y):
"""Does an integer divide"""
print "About to divide %r by %r" % (x, y)
return x // y
server = SimpleXMLRPCServer.SimpleXMLRPCServer(("localhost", 8000))
server.register_instance(SimpleTest())
server.serve_forever()
And this in another
import xmlrpclib
server = xmlrpclib.Server('http://localhost:8000')
print server.divide(6, 3)
print server.divide(6, 2)
print server.divide(6, 0)
The client will print
2
3
Traceback (most recent call last):
File "xmlrpc_client.py", line 6, in <module>
print server.divide(6, 0)
File "/usr/lib/python2.5/xmlrpclib.py", line 1147, in __call__
return self.__send(self.__name, args)
File "/usr/lib/python2.5/xmlrpclib.py", line 1437, in __request
verbose=self.__verbose
File "/usr/lib/python2.5/xmlrpclib.py", line 1201, in request
return self._parse_response(h.getfile(), sock)
File "/usr/lib/python2.5/xmlrpclib.py", line 1340, in
_parse_response
return u.close()
File "/usr/lib/python2.5/xmlrpclib.py", line 787, in close
raise Fault(**self._stack[0])
xmlrpclib.Fault: <Fault 1: "<type 'exceptions.ZeroDivisionError'>:integer division or modulo by zero">
Whereas the server will keep running with
About to divide 6 by 3
localhost - - [04/Feb/2009 09:01:56] "POST /RPC2 HTTP/1.0" 200 -
About to divide 6 by 2
localhost - - [04/Feb/2009 09:01:56] "POST /RPC2 HTTP/1.0" 200 -
About to divide 6 by 0
localhost - - [04/Feb/2009 09:01:56] "POST /RPC2 HTTP/1.0" 200 -
So in general if the error is caused by the client, send it back to
the client by not catching it in the server. The server will keep
running and the client will know it has done something wrong.
--
Nick Craig-Wood <nick at craig-wood.com> -- http://www.craig-wood.com/nick
More information about the Python-list
mailing list