Display data based on read configuration file from ssh sessions

Question

I started playing with Python last week. This is a section of code from an application which reads a configuration file and launches ssh sessions and continuously displays data back to the local user.

The whole project is on Github.

def open_connections(sessions):
 """ open_connections(sessions)
 Opens ssh connections and forks them to the background. Passwords are sent
 from the user via STDIN to each process. If correctly identified, the
 process is successfully created and added to the Session object.
 """
 for server in sessions:
 usr = raw_input("Enter username for " + server.address + ": ")
 pwd = getpass.unix_getpass("Enter passphrase for " + server.address + ": ")
 con = paramiko.SSHClient()
 con.set_missing_host_key_policy(paramiko.AutoAddPolicy())
 con.connect(server.address, 
 username=usr, 
 password=pwd)
 print "[OK]"
 server.connection = con
def run(sessions):
 """ run(sessions)
 Takes the file descriptors from the open sessions and runs the commands
 at the specified intervals. Display the STDOUT to the current tty.
 """
 while True:
 time.sleep(TIMEOUT)
 os.system("clear")
 for session in sessions:
 print session.command + " @ " + session.address
 print "------------------------------------------------------------"
 stdin, stdout, stderr = session.connection.exec_command(session.command)
 print format_stdout(stdout.readlines())

A couple of my concerns:

• Am I doing any big Python no-nos?
• Am I following proper comment/code conventions?
• Is this the best way to display continuous information to user (screen refreshes)?

This is my first contact with Python so I'm definitely doing something I shouldn't be. The rest of the code for this project is really only 1 file, and what I posted above is the bulk of it. If anyone went to the Github page and wanted to comment on something else, I'd be happy to respond.

Answer 1

Docstrings

This:

def open_connections(sessions):
 """ open_connections(sessions)
 Opens ssh connections and forks them to the background. Passwords are sent
 from the user via STDIN to each process. If correctly identified, the
 process is successfully created and added to the Session object.
 """
 for server in sessions:
 # ...

Should be:

def open_connections(sessions):
 """Opens ssh connections and forks them to the background.
 Passwords are sent from the user via STDIN to each process. If correctly
 identified, the process is successfully created and added to the Session
 object.
 """
 for server in sessions:
 # ...

To know more, take a look at PEP257 about multi line docstrings.

There's also no need to repeat the function declaration inside your docstring. If you tomorrow decide to use Sphinx for generating your code documentation, that part will be taken care of on its own. You can find other possible conventions on docstrings here.

while 1 and while True

Replace this:

while True:
 # ...

with:

while 1:
 # ...

This will do a tiny difference in Python 2 (in Python 3 they will be exactly the same). This topic was well discussed on SO here.

print formatting

This line:

print "------------------------------------------------------------"

Will be more readable this way:

print "-" * 60

Check out the Format Specification Mini-Language if you want to do something fancy.

Don't shadow the built-in

I tracked down in your repository, this function:

def format_stdout(stdout):
 """ format_stdout(stdout)
 Takes the list from the stdout of a command and format it properly with
 new lines and tabs.
 """
 nice_stdout = ""
 for tuple in stdout:
 nice_stdout += tuple
 return nice_stdout

Do not use tuple as variable because it will shadow the built in tuple(). But more important I think that you don't need this function at all.

Take a look at join(), it will be faster and better (since strings in Python are immutables).

So you should replace this line:

print format_stdout(stdout.readlines())

with something like:

print ''.join(stdout.readlines())

Write only one time what you could write several

Change this:

usr = raw_input("Enter username for " + server.address + ": ")
pwd = getpass.unix_getpass("Enter passphrase for " + server.address + ": ")

To something like:

template_msg = "Enter {} for " + server.address + ": "
usr = raw_input(template_msg.format("username"))
pwd = getpass.unix_getpass(template_msg.format("passphrase"))

This way you'll be also also calling server.address one time, and one line less to maintain :)

Clear console

It will be better to avoid calling os.system directly, the subprocess module is there exactly for that.

Anyway calling clear is not your only option: check out this SO question.

Don't check the lenght of a list

(unless you really need to know how long it is)

Again peeking from your git repo, I've found this:

if ( len(sessions) == 0 ):

1. Lose the brackets:

   if len(sessions) == 0:
   

   This will work too, Python is not C/C++ so don't put brackets everywhere :)

2. More important check the list directly:

   if session:
    # sessions is not empty
   else:
    # sessions is empty
   

The implicit boolean conversion is a very smart Python feature :)

• nopcorn: Wow, thanks for an amazing breakdown of some key features I'm still missing in Python. +1 and accepted.

Answer 2

 usr = raw_input("Enter username for " + server.address + ": ")
 pwd = getpass.unix_getpass("Enter passphrase for " + server.address + ": ")
 con = paramiko.SSHClient()

I'd avoid using abbreviations. Call them username and password and connection. You don't gain anything using abbreviations and it reduces readability.

 for session in sessions:
for server in sessions:

Are they servers or sessions?

I'd also move the contents of the loop into methods on your session objects.

• nopcorn: I'll make the comments more descriptive to avoid any ambiguity. Also, you're right. I didn't even notice I'm calling them sessions and servers. You mean instead of looping the logic, loop calling a logic method?
• Winston Ewert: @MaxMackie, comments? I didn't mention any coments. Yes, loop calling a logic method.
• nopcorn: My bad, typo. It's early :)