Simple UDP server/client written in Python to report back IP address

Question

I'd like to get some feedback/criticism on my attempt to write a simple client/server application that reports back the (client's) global IP address.

It would be nice if you could comment on the UDP socket setup, general security or any other drawbacks you were able to spot. Since this is my first attempt in writing Python network code (or Python in general other than tinkering) please be harsh!

#! /usr/bin/env python
# grip 0.0.1, external IP address lookup utility.
# usage: grip hostname [port]
# grip -l [hostname] [port]
# grip -h
from __future__ import print_function
import sys
from socket import *
MODE_SERVER = 0
MODE_CLIENT = 1
READ_BUFFER = 15
NULL_BUFFER = 1
DEFAULT_HOST = ''
DEFAULT_PORT = 20815
EXIT_SUCCESS = 0
EXIT_FAILURE = 1
EXIT_SYNTAX = 2
USAGE = '''\
grip 0.0.1, external IP address lookup utility.
usage: grip hostname [port]
 grip -l [hostname] [port]
 grip -h
Options:
 -h display this help message and exit
 -l run in listen mode, to report back remote hosts' IP addresses
Request mode:
 The hostname may be provided in either IP address or DNS format.
 The port number - if unspecified - defaults to 20815.
Listen mode:
 In listen mode the program will try to serve requests on all
 available interfaces unless explicitly told not to.\
'''
def num(n):
 try:
 return int(n)
 except ValueError:
 return None
class Server:
 """Reports back the remote host's IP address"""
 def __init__(self, socket):
 self.socket = socket
 def listen(self, host, port):
 self.socket.bind((host, port))
 try:
 while 1:
 data, address = self.socket.recvfrom(NULL_BUFFER)
 self.socket.sendto(address[0], address)
 except KeyboardInterrupt:
 pass
 return EXIT_SUCCESS
class Client:
 """xyz"""
 def __init__(self, socket):
 self.socket = socket
 def request(self, host, port):
 code = EXIT_SUCCESS
 self.socket.sendto('', (host,port))
 self.socket.settimeout(2)
 try:
 data, saddr = self.socket.recvfrom(READ_BUFFER)
 print(data)
 except:
 code = EXIT_FAILURE;
 print('Error: Request timed out', file=sys.stderr)
 self.socket.close()
 return code
def main():
 args = list(sys.argv[1:])
 argsno = len(args)
 mode = MODE_CLIENT
 hostname = DEFAULT_HOST
 port = DEFAULT_PORT
 # Mere invocation is NOT supported
 if argsno == 0:
 print(USAGE, file=sys.stderr)
 sys.exit(EXIT_SYNTAX)
 # Display a helpfull message when asked
 if '-h' in args:
 print(USAGE)
 sys.exit(EXIT_SUCCESS)
 if '-l' in args:
 mode = MODE_SERVER
 args.remove('-l')
 argsno -= 1
 if mode == MODE_SERVER:
 if argsno > 1:
 hostname = args[0]
 port = num(args[1])
 if argsno == 1:
 if num(args[0]) is not None:
 port = num(args[0])
 else:
 hostname = args[0]
 try:
 serv = Server(socket(AF_INET, SOCK_DGRAM))
 retval = serv.listen(hostname, port)
 except:
 sys.exit(EXIT_FAILURE)
 if mode == MODE_CLIENT:
 hostname = args[0]
 if argsno > 1:
 port = num(args[1])
 try:
 cl = Client(socket(AF_INET, SOCK_DGRAM))
 retval = cl.request(hostname, port)
 except:
 sys.exit(EXIT_FAILURE)
 sys.exit(retval)
if __name__ == "__main__":
 main()

Answer 1

At first glance it looks good with the one exception that Python code should be indented by four spaces, not two; but it's consistent, so there's that. Good job for the first "real" script!

The main function is a bit big. In general it'd be nicer to split the argument processing off and use the argparse library(!) to remove the amount of boilerplate code. If you read through the docs for that you'll see that the argument parsing, which takes up about a full screen for me, can be drastically compressed and made more expressive at the same time.

I'm also not sure if you really want to hide the exceptions so much as the error message would give some insight into why the tool isn't working.

The constants READ_BUFFER and NULL_BUFFER are easy to misunderstand, I'd suggest maybe a _LENGTH suffix or so to make it clear that these aren't actually buffers as the names would imply. The EXIT_* constants are already defined in the os module, so I'd suggest you use that instead.

• return is the same as return None.
• Although valid, I'd recommend while True over while 1 just so you don't get into the habit of treating 0/1 as booleans.
• One of the existing docstring is okay, the other isn't ("xyz").
• There's no need to put a semicolon at the end of a line.

• aefxx: Thank you, I appreciate the time you took. I'll go over the code and revise according to your suggestions.

Answer 2

• sys.argv is already a list. I don't see a purpose of list(sys.argv).

• Creating sockets in the mainline looks strange. It is more natural to create them in corresponding __init__ calls. Same applies to serv.listen().

• from socket import * is really bad practice, especially when socket is used as a parameter:

  self.socket = socket
  

  makes me wonder whether the parameter is an instance or a callable.

• recvfrom may raise a socket.error exception. Some of them are recoverable, so you may want to catch then as well.

• aefxx: Good points, I'll look into it, thanks! Regarding the list(sys.argv): I tinker with the list (e.g. remove the -h flag) and didn't know whether it's ok to mess with the argument vector?!?

Answer 3

Instead of while 1 it's more idiomatic to use while True. Similarly, 4 space indentation is the recommended Python style. It makes things a lot easier to read, especially as the indentation affects control flow. You should read the style guide for lots of details on how to make code readable and Pythonic.

Does Server really need to be a class? It has only one function besides __init__ and you only use it once to immediately call the one function and that's all it does. Your docstring certainly makes it sound like a function:

"""Reports back the remote host's IP address"""

Python's perfectly happy to just have functions and not deal with classes when you don't really need to use classes. Client similarly doesn't seem like it needs to be a class. I'd just make both functions.

Speaking of docstrings, """xyz""" doesn't tell a user anything. It would be better to not have a docstring than a redundant one.

• aefxx: Thanks for the hints on Python's style guide. You are right in that the server could very well be a function, yet I might add some more logic to it (think of verbose debug messages, handling interrupts and the like) and would rather have it all under a single domain.
• SuperBiasedMan: @aefxx That makes sense then. Sometimes it just seems like people use classes out of habit rather than good reason like that.