[node-patches] Change in ovirt-node[master]: add dynamic firewall creation for plugins

fabiand at fedoraproject.org fabiand at fedoraproject.org
Thu Aug 8 14:14:46 UTC 2013


Fabian Deutsch has posted comments on this change.

Change subject: add dynamic firewall creation for plugins
......................................................................


Patch Set 2: Code-Review-1

(3 comments)

....................................................
File src/ovirt/node/utils/firewall.py
Line 45:     else:
Line 46:         return False
Line 47: 
Line 48: 
Line 49: def setup_iptables(conf):
this function should take a port and protocol, not a line.
Line 50:     for rule in conf:
Line 51:         try:
Line 52:             port, proto = rule.split(",")
Line 53:         except:


Line 56:                "--dport", port, "-j", "ACCEPT"]
Line 57:         process.check_call(cmd, shell=True)
Line 58: 
Line 59: 
Line 60: def setup_firewalld(conf):
this function should take a port and protocol, not a line.
Line 61:     port_conf = ""
Line 62:     for rule in conf:
Line 63:         try:
Line 64:             port, proto = rule.split(",")


Line 87:         with open(f) as i:
Line 88:             conf = i.readlines()
Line 89:         for line in conf:
Line 90:             if not line.startswith("#"):
Line 91:                 fw_conf.append(line.strip())
I'd rather split the line here into the appropriate components: (port, proto) so they can be passed to the functions.

So:

    port, proto = line.strip().split(",")
    fw_conf.append((port, proto))
Line 92: 
Line 93:     if is_firewalld():
Line 94:         setup_firewalld(fw_conf)
Line 95:     else:


-- 
To view, visit http://gerrit.ovirt.org/17843
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia6b364cba46afe490b1ab84ba8fe2f879eed4dcf
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-node
Gerrit-Branch: master
Gerrit-Owner: Joey Boggs <jboggs at redhat.com>
Gerrit-Reviewer: Fabian Deutsch <fabiand at fedoraproject.org>
Gerrit-HasComments: Yes



More information about the node-patches mailing list