Fabian Deutsch has posted comments on this change. Change subject: Validate passed NTP servers and reject bad ones ......................................................................
Patch Set 3: Code-Review-1 (2 comments) http://gerrit.ovirt.org/#/c/17888/3/scripts/ovirt-init-functions.sh.in File scripts/ovirt-init-functions.sh.in: Line 643: dns=*) Line 644: dns=${i#dns=} Line 645: ;; Line 646: ntp=*) Line 647: ntp=$(printf $i|python -c "`cat << EOP Could we push this functionality into some method, e.g.: ovirt.node.config.defaults.Nameservers.filter_valid_servers(list) returns teh list of valid servers. The member can be static. Line 648: import sys Line 649: from ovirt.node import valid Line 650: validservers = [] Line 651: for line in sys.stdin: http://gerrit.ovirt.org/#/c/17888/3/src/ovirt/node/config/defaults.py File src/ovirt/node/config/defaults.py: Line 850: validator = lambda v: valid.FQDNOrIPAddress() Line 851: map(validator, servers) Line 852: Line 853: validservers = [x for x in servers if Line 854: valid.FQDNOrIPAddress().validate(x)] I'd rather like to keep out the filtering out of this config module. My suggestion would be that we _assume_ that only valid servers are passed and therefor validate all servres against the FQDNOrIPAddress() validator and raise an exception if necessary. I would not silently filter out incrrect servers, that is hard to debug. At one point we wonder why some servers don't end up in the file. So my suggestion: Validate yes, filter no. Filter in the higher level function ... Line 855: Line 856: # Mangling to match the conf file Line 857: return {"OVIRT_NTP": ",".join(validservers) or None Line 858: } -- To view, visit http://gerrit.ovirt.org/17888 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifb135977b4359f5fe0161215cec2cdd1b22244b3 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-node Gerrit-Branch: master Gerrit-Owner: Ryan Barry <[email protected]> Gerrit-Reviewer: Fabian Deutsch <[email protected]> Gerrit-Reviewer: Joey Boggs <[email protected]> Gerrit-Reviewer: Ryan Barry <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ node-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/node-patches
