Request for comments - concurrent ssh client

[MRAB]

mk wrote:
> Hello everyone,
> 
> Since I'm not happy with shmux or pssh, I wrote my own "concurrent ssh" 
> program for parallel execution of SSH commands on multiple hosts. Before 
> I release program to the wild, I would like to hear (constructive) 
> comments on what may be wrong with the program and/or how to fix it. 
> (note: the program requires paramiko ssh client module)
> 
[snip]
> if opts.cmd == None and opts.script == None:
>     print "You have to specify one of the following: command to run, 
> using -c command or --cmd command, or script to run, using -s scriptfile 
> or --script scriptfile."
>     print
>     failit = True
> 
The normal way to test for None is to use "is None" or "is not None".
You do actually do that in some places!

[snip]
> hosts = [ s.strip() for s in hosts if s != '' and s != None and s != '\n' ]
> hosts = [ s.split() for s in hosts ]
> 
[snip]
Both '' and None are treated as false by 'if' and 'while'; non-empty
strings are treated as true. Also, "s.split()" splits on whitespace and
disregards leading and trailing whitespace. The preceding lines can be
simplified to:

     hosts = [ s.split() for s in hosts if s and s != '\n' ]

> if hosts == []:
[snip]

Empty lists are also treated as false and non-empty lists as true. The
Pythonic way to write the preceding line is:

   if not hosts:

[snip]
>             scriptcomp = script.strip().split()

As mentioned earlier, ".strip().split()" can be simplified to just
".split()":

               scriptcomp = script.split()

[snip]
>             try:
>                 self.conobj.connect(self.ip, username=self.username, 
> password=self.passw, port=self.port, timeout=opts.timeout, 
> allow_agent=False, look_for_keys = False)
>                 loginsuccess = True
>             except:
>                 pass
[snip]
Avoid bare "except" wherever possible. In this case you're ignoring
_every_ exception that might occur. VERY BAD IDEA!

>     def execcmds(self):
>         so = se = ''
>         try:
>             si, so, se = self.conobj.exec_command(self.cmd)
>             sol = so.readlines()
>             sel = se.readlines()
>             so = ''.join([ s.replace('\r\n','\n') for s in sol ])
>             se = ''.join([ s.replace('\r\n','\n') for s in sel ])
[snip]

Recent versions of Python will accept generator expressions here, so
instead of iterating through a list and creating a new list, which is
then passed to '.join', you can let '.join' do the iterating. This can
save time and memory:

              so = ''.join( s.replace('\r\n','\n') for s in sol )
              se = ''.join( s.replace('\r\n','\n') for s in sel )

>         if sol != [] or sel != []:

As mentioned earlier, non-empty lists are treated as true:

          if sol or sel:

  >     def chmodscript(self):
>         # just in case, as sometimes and on some operating systems the 
> execution flags on the script are not always set
>         self.execcmdonscript('chmod 0755 %s' % self.rspath)
> 
[snip]

You could also use:

          os.chmod(self.rspath, 0755)

>     def delscript(self):
>         self.execcmdonscript('rm -f %s' % self.rspath)
> 
[snip]

You could also use:

          os.remove(self.rspath)

>     for ip, th in queue:
>         if th.finished:
>             th.sshclose()
>             th.join()
>             thfinished.append((ip,th))
>             queue.remove((ip,th))
> 
Never modify a list (or any container, in fact) while iterating through
it. Some containers handle iteration by using an index, but if you
remove an item from the container then the index might no longer be
correct. Instead, build a new list of the items you want to keep:

      new_queue = []
      for ip, th in queue:
          if th.finished:
              th.sshclose()
              th.join()
              thfinished.append((ip,th))
          else:
              new_queue.append((ip,th))
      queue = new_queue

If there are other references to the queue itself then it might be
better to replace the contents of the existing queue with those of the
new one instead by changing:

     queue = new_queue

to:

     queue[:] = new_queue