Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've written the below python class that fetches iptables rules using basic string processing in linux. Could you plz go thru it and tell me whether I'm doing this in the correct manner. I've tested the code and its 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()
share|improve this question

2 Answers

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

share|improve this answer
Sometimes, for learning something new, we have to reinvent the wheel!! If you think about it, almost everything you can think of is already coded someplace. I intend to gain mastery over python by these exercises. – Prahlad Yeri Jul 1 at 20:05
There are better ways to improve your Pythonistas skills, and I think that parsing the output of iptables commands isn't worth taking it onto the board. It will learn you bad, unsecure habits in programming, that should be avoided. – Michał F Jul 3 at 8:30
I want to create a router-cum-firewall type of app (such as ufw), Is python-iptables module flexible enough to handle that? – Prahlad Yeri Jul 3 at 17:05

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.

share|improve this answer
many thanks for the excellent review. I'd read about the PEP standards, but didn't know it is that much followed in actual practice. – Prahlad Yeri Jul 3 at 17:03

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.