diff --git a/src/dcc.py b/src/dcc.py index ba04b9f85..85fbd57a9 100644 --- a/src/dcc.py +++ b/src/dcc.py @@ -1,8 +1,13 @@ #!/usr/bin/env python -# You need a copyright notice here, azaroth. -# You might do well to put links to the documentation you're basing this on -# here. Then other people can help maintain it more easily. +# Copyright (c) Robert Sanderson All rights reserved. +# See LICENCE file in this distribution for details. + +# Documentation on DCC specification +# http://www.mirc.co.uk/help/dccresum.txt +# www.irchelp.org/irchelp/rfc/dccspec.html +# Also several other lesser known/implemented/used commands are floating around +# but not implemented in major clients. import os import time @@ -18,47 +23,48 @@ import ircutils import threading def isDCC(msg): - # This needs to be more specific, it would catch too many messages - # discussing DCC stuff. I added a test for this. - return msg.command == 'PRIVMSG' and msg.args[1][1:4] == 'DCC' - + return msg.command == 'PRIVMSG' and msg.args[1][:5] == '\x01DCC ' + + +conf.registerGlobalValue(conf.supybot.protocols.dcc, 'timeout', + registry.Integer(120, "Timeout on DCC sockets")) # ---- Out Handlers ---- class DCCHandler: - # You should reconsider whether all these arguments are necessary in the - # constructor. Long argument lists generally mean bad factoring. def __init__(self, irc, nick, hostmask, logger=None): self.irc = irc self.nick = nick self.hostmask = hostmask self.sock = None - # XXX () in if/while. - if (logger): - # XXX Log something specific + if logger: self.log = logger else: self.log = log - # XXX Why 120? Shouldn't that be configurable? - self.timeout = 120 + + self.timeout = conf.supybot.protocols.dcc.timeout() def start(self): - # What's this self.open? I don't see an open method. t = threading.Thread(target=self.open) world.threadsSpawned += 1 t.setDaemon(True) t.start() + def open(self): + # Override in subclasses to do something + pass + def clientConnected(self): + # Override in subclasses to do something when a client connects pass def clientClosed(self): + # Override to do something when a client closes connection pass class SendHandler(DCCHandler): """ Handle sending a file """ - # What I said about long argument lists in DCCHandler applies doubly here. def __init__(self, irc, nick, hostmask, filename, logger=None, start=0): DCCHandler.__init__(self, irc, nick, hostmask, logger) self.filename = filename @@ -76,167 +82,122 @@ class SendHandler(DCCHandler): def open(self): ip = conf.supybot.externalIP() - host = ircutils.hostFromHostmask(self.irc.prefix) - if not ip: - try: - ip = socket.gethostbyname(host) - except: # XXX Don't use blank excepts. - # XXX Be sure you mention who you are and why you couldn't do - # what you should. - self.log.warning('Could not determine IP address') - return try: self.filesize = os.path.getsize(self.filename) except OSError, e: - # XXX: There should be a log.warning or log.error here. - # Doesn't exist - # ^^^^^^^^^^^^^ What doesn't exist? + self.log.warning('Requested file does not exist: %r', + self.filename) return sock = utils.getSocket(ip) try: sock.bind((ip, 0)) except socket.error, e: - # XXX: There should be a log.warning or log.error here. + self.log.warning('Could not bind a socket to send.') return port = sock.getsockname()[1] self.port = port - self.registerPort() - i = ircutils.dccIP(ip) sock.listen(1) -## self.irc.queueMsg(ircmsgs.privmsg(self.nick, -## '\x01DCC SEND %s %s %d %d\x01' % (self.filename, i, port, -## self.filesize))) -## That formatting is bad. Try this instead: -## msg = ircmsgs.privmsg(self.nick, -## '\x01DCC SEND %s %s %s %s\x01' % \ -## (self.filename, i, port, self.filesize)) -## self.irc.queueMsg(msg) -## Even better, I'll define a function ircmsgs.py. msg = ircmsgs.dcc(self.nick, 'SEND', self.filename, i, port, self.filesize) self.irc.queueMsg(msg) - # Wait for possible RESUME - # That comment isn't enough, why are we really sleeping? Why only one - # second? A link to some documentation might help. + + # Wait for possible RESUME request to be handled which may change + # self.startPosition + # See Resume doc (URL in header) time.sleep(1) try: (realSock, addr) = sock.accept() except socket.error, e: sock.close() - # Remember, we dont' use % in log strings -- we just put the - # arguments there after the string. utils.exnToString(e) gives a - # prettier string form than just e; the latter doesn't show what the - # class of the exception is. - # XXX % in log. self.log.info('%r: Send init errored with %s', self.filename, utils.exnToString(e)) return - # XXX % in log. - self.log.info('%r: Sending to %s' % (self.filename, self.nick)) - # There shouldn't be blank space like this in a single function. But - # then again, a single function shouldn't be this long. Perhaps you - # can break this into multiple functions? - + self.log.info('%r: Sending to %s', self.filename, self.nick) self.sock = realSock fh = file(self.filename) - # Why no arguments to this clientConnected method? - self.clientConnected() - self.startTime = time.time() try: - slow = 0 - fh.seek(self.startPosition) - self.currentPosition = fh.tell() - # Again, please no parentheses around the conditions of while loops - # and if statements. - # XXX () in while/if - while (self.currentPosition < self.filesize): - data = fh.read(min(1024, self.filesize-self.currentPosition)) - self.sock.send(data) + self.clientConnected() + self.startTime = time.time() + packetSize = conf.supybot.protocols.dcc.packetSize() + try: + slow = 0 + fh.seek(self.startPosition) self.currentPosition = fh.tell() - self.bytesSent += 1024 - self.packetSent() - - except socket.error, e: - # Aborted/timedout chat - # XXX % in log. - self.log.info('%r: Send errored with %s' % - (self.filename, e)) - except SendException, e: - # XXX % in log. - self.log.info('%r: Send aborted with %s' % - (self.filename, e)) - # Wait for send to complete - self.endTime = time.time() - duration = self.endTime - self.startTime - # XXX. % in log. Also use %r here instead of %s. - self.log.info('\'%s\': Sent %s/%s' % - (self.filename, fh.tell(), self.filesize)) - # Why sleep here? - time.sleep(1.0) - # XXX These closes should go in a finally: block. - fh.close() - self.sock.close() - self.clientClosed() + while self.currentPosition < self.filesize: + data = fh.read(min(packetSize, self.filesize - \ + self.currentPosition)) + self.sock.send(data) + self.currentPosition = fh.tell() + self.bytesSent += 1024 + self.packetSent() + except socket.error, e: + exn = utils.exnToString(e) + self.log.info('%r: Send errored with %s', self.filename, exn) + except SendException, e: + exn = utils.exnToString(e) + self.log.info('%r: Send aborted with %s', self.filename, exn) + + self.endTime = time.time() + duration = self.endTime - self.startTime + self.log.info('%r: Sent %s/%s', self.filename, fh.tell(), + self.filesize) + # Sleep to allow client to finish reading data. + # This is needed as we'll be here immediately after the final + # packet + time.sleep(1.0) + self.clientClosed() + finally: + # Ensure that we're not leaking handles + fh.close() + self.sock.close() class ChatHandler(DCCHandler): """ Handle a DCC chat (initiate) """ - def handleLine(self, line): - # What's this for? Documentation! + def lineReceived(self, line): + # Override in subclasses to process a line of text pass def open(self): ip = conf.supybot.externalIP() - # XXX () in while/if - if (not ip): - # Try and find it with utils - # No, I'll fix externalIP to return something sane for you. - pass - sock = utils.getSocket(ip) + lineLength = conf.supybot.protocols.dcc.chatLineLength() try: sock.bind((ip, 0)) except socket.error, e: - # XXX Who are you, and why can't you? - self.irc.error('Unable to initiate DCC CHAT.') + self.log.error('Could not bind chat socket.') return port = sock.getsockname()[1] i = ircutils.dccIP(ip) sock.listen(1) - # XXX Use ircmsgs.dcc - self.irc.queueMsg(ircmsgs.privmsg(self.nick, - '\x01DCC CHAT chat %s %s\x01' % (i, port))) + msg = ircmsgs.dcc(self.nick, 'CHAT', 'chat', i, port) + self.irc.queueMsg(msg) try: (realSock, addr) = sock.accept() except socket.timeout: - # XXX % in log - self.log.info('CHAT to %s timed out' % self.nick) + sock.close() + self.log.info('CHAT to %s timed out', self.nick) return - # XXX % in log. - self.log.info('CHAT accepted from %s' % self.nick) + self.log.info('CHAT accepted from %s', self.nick) realSock.settimeout(self.timeout) self.startTime = time.time() self.sock = realSock try: self.clientConnected() - # XXX () in while/if - while (1): - # XXX Why 66000? - line = realSock.recv(66000) - # XXX Don't use <>; use !=. - if (line <> "\n"): - self.handleLine(line) + while 1: + line = realSock.recv(lineLength) + if line != "\n": + self.lineReceived(line) except socket.error, e: - # Aborted/timedout chat. Only way to end. - # Are you sure you don't still need to close the socket here? - # XXX % in log. Also, don't use parens around a single value. - self.log.info('CHAT ended with %s' % (self.nick)) - self.endTime = time.time() - self.clientClosed() + self.log.info('CHAT ended with %s', self.nick) + finally: + self.sock.close() + self.endTime = time.time() + self.clientClosed() # ---- In Handlers ---- @@ -247,8 +208,7 @@ class DCCReqHandler: self.irc = irc self.msg = msg self.args = args - # XXX: () in while/if - if (logger): + if logger: self.log = logger else: self.log = log @@ -259,112 +219,89 @@ class DCCReqHandler: t.setDaemon(True) t.start() - # Always put a blank line between methods, even if they just pass. def clientConnected(self): pass - # XXX No blank line between methods. + def clientClosed(self): pass class SendReqHandler(DCCReqHandler): """ We're being sent a file """ - # These should be setup in __init__ unless they have a real reason for - # being class variables. - ip = "" - filename = "" - port = 0 - filesize = 0 def __init__(self, *args, **kw): DCCReqHandler.__init__(self, *args, **kw) # This should be added to by subclasses self.incomingDir = conf.supybot.directories.data() - - def receivedPacket(self): - # What's this method for? Document. - pass - - def open(self): - # XXX: Should this be factored into a separate function? self.filename = self.args[0] self.ip = ircutils.unDccIP(int(self.args[1])) self.port = int(self.args[2]) self.filesize = int(self.args[3]) - # XXX () in while/if + + def receivedPacket(self): + # Override in subclass to do something with each packet received + pass + + def open(self): if (os.path.exists(self.filename)): currsize = os.path.getsize(self.filename) if (self.filesize > currsize): # Send RESUME DCC message and wait for ACCEPT - # XXX Line too long. - self.irc.queueMsg(ircmsgs.privmsg(self.msg.nick, - '\x01DCC RESUME %s %d %d\x01' % (self.filename, self.port, currsize))) - # URHG? - # What does "URGH?" mean? Details! + msg = ircutils.dcc(self.nick, 'RESUME', self.filename, + self.port, currsize) + self.irc.queueMsg(msg) return sock = utils.getSocket(self.ip) try: sock.connect((self.ip, self.port)) - except: - # XXX blank except, no log. + except socket.error, e: + self.log.info('File receive could not connect') return self.clientConnected() - - # XXX long line; also, what does "rootedName" really mean? - rootedName = os.path.abspath(os.path.join(self.incomingDir, self.filename)) - # XXX Use the startswith() method on strings. - if (rootedName[:len(self.incomingDir)] != self.incomingDir): - # XXX % in log. - self.log.warning('%s tried to send relative file' % self.msg.nick) + rootedName = os.path.abspath(os.path.join(self.incomingDir, + self.filename)) + if not rootedName.startswith(self.incomingDir): + self.log.warning('%s tried to send relative file', self.msg.nick) return - # XXX f is generally used for functions; use fh, or fd, or a name - # representative what file is being used. - f = file(rootedName, 'w') + fh = file(rootedName, 'w') self.bytesReceived = 0 self.startTime = time.time() - # XXX This is a src/ plugin. It shouldn't depend on any plugin. You - # definitely can't use any plugin's registry variables. This is the - # number 1 biggest problem in this file. - pktSize = conf.supybot.plugins.FServe.packetSize() - # XXX % in log, use %r - self.log.info('\'%s\': Send starting from %s' % - (self.filename, self.msg.nick)) + pktSize = conf.supybot.protocols.dcc.packetSize() + self.log.info('%r: Send starting from %s', self.filename, + self.msg.nick)) try: - # XXX () in while/if - while (self.bytesReceived < self.filesize): + while self.bytesReceived < self.filesize: amnt = min(self.filesize - self.bytesReceived, pktSize) d = sock.recv(amnt) self.bytesReceived += len(d) - # XXX What's this do? Document please :) + # Required to send back packed integer to acknowledge receive sock.send(struct.pack("!I", self.bytesReceived)) f.write(d) self.receivedPacket() except socket.error, e: - # % in log, use %r. - self.log.info('\'%s\': Send died with %s' % (filename, e)) - # XXX If you intend to fall through, i.e., not to return here, you - # should have a comment to that effect. - self.endTime = time.time() - # XXX Perhaps these closes should go in a finally: block? - sock.close() - f.close() - # XXX % in log, use %r. - self.log.info('\'%s\': Received %s/%s in %d seconds' % - (self.filename, self.bytesReceived, self.filesize, - self.endTime - self.startTime)) - self.clientClosed() + exn = utils.exnToString(e) + self.log.info('%r: Send died with %s', filename, exn) + finally: + self.endTime = time.time() + sock.close() + f.close() + self.log.info('%r: Received %s/%s in %d seconds', + self.filename, self.bytesReceived, self.filesize, + self.endTime - self.startTime) + self.clientClosed() class ResumeReqHandler(DCCReqHandler): def _getSendHandler(self): - # XXX Explain this comment more. - # This is bad. It will just start a new send. - # It needs to look up original + # This will work in theory, BUT note well, if you instantiate + # and do not override this to return the REAL SendHandler + # the client may still get the original startPosition of 0 + # See RESUME documentation URL in header hostmask = self.irc.state.nickToHostmask(self.msg.nick) h = SendHandler(self.irc, self.msg.nick, hostmask, self.filename, start=self.startPosition) @@ -372,22 +309,20 @@ class ResumeReqHandler(DCCReqHandler): def open(self): # filename is (apparently) ignored by mIRC + # so don't depend on it. self.filename = self.args[0] self.port = int(self.args[1]) self.startPosition = int(self.args[2]) - # XXX Use ircmsgs.dcc. - self.irc.queueMsg(ircmsgs.privmsg(self.msg.nick, - '\x01DCC ACCEPT %s %s %s\x01' % (self.filename, self.port, - self.startPosition))) + msg = ircutils.dcc(self.msg.nick, "ACCEPT", self.filename, self.port, + self.startPosition) + self.irc.queueMsg(msg) cxn = self._getSendHandler() cxn.startPosition = self.startPosition - # XXX % in log. - self.log.info('%r: RESUME received for %s' % - (self.filename, self.startPosition)) - - + self.log.info('%r: RESUME received for %s', self.filename, + self.startPosition) + # --- IGNORE FROM HERE DOWN --- def handleACCEPT(self): port = int(self.args[1])