Systems management using SSH for my school

Question

I wrote this script in order to generate a hosts.txt file with the IP addresses of the laptops in my school.

Each laptop is assigned a number, and their IP addresses are assigned to them with the template "192.168.0.x". X being the computer's number + 10. All these computers are running Arch Linux with XFCE4, custom tailored to aid in education. Each laptop is setup with SSH and the Teacher, (me), has the ability to ssh into any machine and execute any command I deem necessary.

Up until recently, I would SSH into the machines individually and install programs one-by-one. Now I use a program called PSSH (Parallel SSH) to connect to certain machines simultaneously and perform my commands. I only find one issue with this arrangement, all the IP addresses are stored in a file called, you guessed it, hosts.txt.

Now, as you can imagine, this gets quite cumbersome when dealing with 70 different addresses, and I would rather not have to type all the addresses by hand. So I made this script to generate a hosts file based off the computer numbers I choose. Selecting all the machines is now as easy as typing 'all'.

addresses = []
nums = []
run = True

def rem_dupl(seq):
    seen = set()
    seen_add = seen.add
    return [x for x in seq if not (x in seen or seen_add(x))]

while run:
    x = str(input("Computer numbers: "))

    if x == 'done':
        run = False
    elif x == 'all':
        nums = [x for x in range(1, 70)]
        run = False
    elif int(x) > 0 and int(x) < 70:
        nums.append(x)
    else:
        print "Out of range"
else:
    nums_finished = rem_dupl(nums)

    for i in nums_finished:
        addresses.append("192.168.0." + str((int(i) + 10)))

    temp = open('hosts', 'w').close()

    file = open('hosts.txt', 'w')
    for item in addresses:
        print>>file, item

   file.close

Any thoughts, constructive criticism, apparent issues, or redundancies?

Answer 1

As suggested in the comments, I would strongly suggest looking at a proper configuration management tool such as Ansible or Puppet (both free) instead of rolling your own. That will probably save you a lot of time, once you’ve done the initial ramp-up.

---

Here are some comments on the code presented:

• There aren’t any comments or documentation. It’s hard to tell if this script is behaving correctly, because it’s not written anywhere. (Although the description on the question is good, it would be better if it was embedded in the script itself.)

• Use full names in variables; don’t skimp out with abbreviations. It makes the code much easier to read, and characters are cheap. For example: remove_duplicates instead of rem_dupl.

  Speaking of that function, here’s a cleaner way to write it:

  def remove_duplicates(sequence):
      return list(set(sequence))
  

• In the while run: loop, rather than having the sentinel variable run, it would be nicer to use break statements to get out of the loop. Something like:

  while True:
      x = ...
      if x == 'done':
          break
      elif x == 'all':
          ...
          break
      else:
          ...
  

  This is one less variable to understand and track (for the human reading your code, that is!)

• In Python 2, range() already returns a list, so

  nums = [x for x in range(1, 70)]
  

  can be reduced to

  nums = range(1, 70)
  

  I suspect you probably meant

  nums = [str(x) for x in range(1, 70)]
  

  because then nums is always a list of strings, however it gets populated.

• Rather than creating addresses as an empty list and populating it one-by-one, you could use a list comprehension:

  addresses = ["192.168.0." + str((int(i) + 10)) for i in set(nums)]
  

  Note that in this case, we can just deduplicate with set() – we don’t even need to recast to a list to put it in our list comprehension.

• It’s not clear what the temp variable is being used for, as we never seem to do anything with the hosts file.

• Rather than using the f = open(path); ...; f.close() paradigm, it’s better to use the with statement like so:

  with open('hosts.txt', 'w') as f:
      f.write('\n'.join(addresses))
  

  This ensures that the file will always be closed correctly, even if something goes wrong while you’re trying to write to it.

  (For example, your file.close is missing the parentheses to actually call the close() method, so this file will never be closed.)

  Note also that it’s generally considered bad practice to use a builtin as a variable name. This can be a source of fun and weird bugs, so avoid it unless absolutely necessary.

And an ancillary thought:

• As presented, the hosts will be written in the order supplied. Would it be better to sort the output, so that you can easily skim the list and spot missing entries? Not a must, just an idea.

Answer 2

As @alexwlchan gave a nice answer, I won't restate what they put.

Instead what are the main things that your code does?

1. Get user input, changes it it list of numbers. (While loop)
2. Change the numbers to distinct ip addresses after being filtered to the range \$1 < x < 70\$.
3. Write to a file.

I'm skipping (1), for now, but (2) and (3) are perfect for Python.

---

You used list comprehensions in your question, you used them as a means to filter a list. However you can do both your unique and range filter in one comprehension.

[i for i in [...] if 0 < i < 70 and not (i in seen or seen_add(i))]

However you can also do what is know as map. Where you change one item to another item. For example you add ten to i and make it the last term in an ip address. Or more simply put:

['192.168.0.' + str(i + 10) for i in [...]]

however rather than doing this as two comprehensions you can just do it as one.

ips = (
    '192.168.0.' + str(i + 10)
    for i in [...]
    if 0 < i < 70 and not (i in seen or seen_add(i))
)

---

Alex has already gone over (3), but to re-state you want to use with. You can then consume the ips by joining them together and save them in a file.

with open('hosts.txt', 'w') as f:
    f.write('\n'.join(ips))

---

I kept (1) till last, as I don't think the way you have to enter data is too nice. If we were in an alternate world where Python didn't have range, first, people would hate the language, but we would build arrays the way you are.

my_list = [1, 2, 3, 4, 5, 6]

This would be a horrible world. Instead you could get the user to input a range. The below could be a nice notation to do this.

> 1..4
1, 2, 3
> 1..4, 5
1, 2, 3, 5
> 1..4, 6..8
1, 2, 3, 6, 7
# But we want to have steps!
> 1..7'2
1, 3, 5

This is actually quite easy to implement. If you think of 1..7'2 as range(1, 7, 2). All you would need to do is match the pattern \d..\d'\d. This is also fairly easy with regex.

import re
def get_input():
    regex = "(\d+)(?:\.\.(\d+)(?:\'(\d*))?)?"
    for match in re.findall(regex, input("> ")):
        match = [int(i) for i in match if i]
        if len(match) == 1:
            yield match[0]
        else:
            for j in range(*match):
                yield j

If you join this all together it would be used like:

$ python hosts.py
> 1..60'2
$ head -3 hosts.tex
192.168.0.11
192.168.0.13
192.168.0.15

If you however still want to use the while loop, then you would need it to return an array of numbers, as int's not strings.

---

It's also recommended to upgrade to Python3. Arch Linux has changed to Python3 being the main Python, so it makes even more sense to use Python3 rather than Python2.

Answer 3

Just an extra note related to using string concatenation or not. The entire else: block could be replaced by this:

with open("hosts.txt", "w") as output:
    output.write("\n".join("192.168.0.{}".format(i + 10) for i in set(nums)))

And if you want the output sorted, you could use sorted(set(nums)). This uses the, to me, more intuitive str.format() method to build the final IP-address, and this can be customized if you are so inclined.

The code use set to induce uniqueness, str.format for format the IP-address, simple integer arithmetic to add the offset, with statement to proper handle opening and closing of file, and join to join the final list before writing.