Python class to abstract the iptables Linux firewall

Question

I've written the following Python class that fetches iptables rules using basic string processing in Linux. Am I doing this in the correct manner? I've tested the code and it's running fine.

I'm ultimately going to use this class in a GUI app that I'm going to write using Python + GObject.

#!/usr/bin/python
import sys, subprocess

class Fwall:
    currchain=""
    dichains={'INPUT':[],'OUTPUT':[],'FORWARD':[]}
    dipolicy=dict(INPUT=True,OUTPUT=True,FORWARD=True)
    dicolumns={}

def __init__(self):
    self.getrules()
    self.printrules()
    print "class initialized."

def getrules(self):
    s = subprocess.check_output("iptables --list --verbose",shell=True,stderr=subprocess.STDOUT)#.split('\n')
    print s
    for line in s.splitlines():
        if len(line)>0:
            self.parseline(line)

def parseline(self,line):
    line=line.strip()
    if line.startswith("Chain"): #this is the primary header line.
        self.currchain=line.split(' ')[1]
        allowed=not line.split('policy ')[1].startswith('DROP')
        self.dipolicy[self.currchain]=allowed
        #print "curr chain set to " + self.currchain
    else:
        items=line.split()
        if line.strip().startswith('pkts'): #this is the secondary header line, so fetch columns.
            if len(self.dicolumns)==0:
                for i,item in enumerate(items):
                    if len(item)>0:
                        self.dicolumns.setdefault(item,i)
                #print self.dicolumns
            else:
                return
        else:
            ditemp={}
            #print line
            self.dichains[self.currchain].append(ditemp)
            for k,v in self.dicolumns.iteritems():
                #idx= self.dicolumns[item]#,items[item]
                ditemp.setdefault(k,items[v])
                #print line
                #print k,v
                #print k,items[v]#,idx

def printrules(self):
    for k,v in self.dichains.iteritems():
        print k
        litemp=self.dichains[k]
        for item in litemp:
            print item


if __name__=="__main__":
    f=Fwall()

Answer 1

There are violations of PEP-8 throughout and there are no PEP-257 docstrings. It sounds petty, but these mean the difference between an easy read and a hard one.

The class name Fwall is terrible, if you mean Firewall call it that. If you mean that it is a container of FirewallRules then say so. Names are extremely important.

If getrules() is not part of the public class interface, convention suggests naming it _getrules which tells the reader that it is expected to be used only by the class itself. Having print statements is fine for debugging, but a well behaved library should assume that there is no stdout and return lists of strings in production code.

Long lines are hard to read, especially on StackExchange. Fortunately, splitting within parenthesis is natural and preferred, thus:

s = subprocess.check_output("iptables --list --verbose",
        shell=True, stderr=subprocess.STDOUT)

but sticking a comment marker in code will certainly cause misreadings. Where you have

s = subprocess.check_output(...)#.split('\n')

you should drop the #.split('\n'). Also, following PEP-8, put spaces after argument separating commas.

You use iteritems() unnecessarily as the standard iteration behavior of a dictionary is now assumed. Where you have for k,v in self.dicolumns.iteritems(): the line

for k, v in self.dicolumns:

is equivalent. To bang on the "names are important" drum, every dictionary has k and v, using more descriptive names would tell me what the meaning of the keys and values are. For that matter I have no idea was a dipolicy or dicolumns are, they might be related or parallel lists but I don't know. You tend to use if len(item)==0 when if item means the same thing.

I had to edit your entry because the class methods were not properly indented; this usually means that you are using a mix of tabs and spaces. Tell your editor to forget that the TAB character exists and to use only spaces, there are instructions for most editors on how to make it pretend that the TAB key behaves like you expect without putting literal \t characters in your source code.

The structure of parseline() is so deeply nested and has so many return points and un-elsed ifs that I really can't follow the logic of it. Read The Zen of Python and keep it wholly.

Finally, iptables is notoriously hairy. I strongly suggest that you use python-iptables or at least study its class structure. You might also want to look at Uncomplicated Firewall as it is designed to abstract iptables into something most humans can understand. It doesn't handle all possible uses, but it makes the typical cases comprehensible.

Answer 2

This is not maybe a place for such suggestions, but why are you reinventing the wheel? There is already the library intended to do that: python-iptables