Ondřej Svoboda has posted comments on this change. Change subject: net: ovs: better rollback ......................................................................
Patch Set 8: Code-Review-1 (18 comments) I find modification of dictionaries while iterating them dangerous. Some hook scripts don't have proper executable permissions. With assumed RPM magic not universally present, I would like to see +x permissions set explicitly :-) https://gerrit.ovirt.org/#/c/46907/8//COMMIT_MSG Commit Message: Line 9: Untill Until Line 13: failtures failures Line 19: witch with a Line 20: OVS only an OVS-only (the hyphen helps, don't forget the next occurence as well) https://gerrit.ovirt.org/#/c/46907/8/vdsm_hooks/ovs/README File vdsm_hooks/ovs/README: Line 65: finishes finishes, Line 118: happen happens Line 136: trailing space https://gerrit.ovirt.org/#/c/46907/8/vdsm_hooks/ovs/ovs_after_get_stats.py File vdsm_hooks/ovs/ovs_after_get_stats.py: Line 1: #!/usr/bin/env python The file has a hashbang, but its mode changed to non-executable in this commit. https://gerrit.ovirt.org/#/c/46907/8/vdsm_hooks/ovs/ovs_after_network_setup.py File vdsm_hooks/ovs/ovs_after_network_setup.py: Line 1: #!/usr/bin/env python Also, it looks like it could be run standalone, but executable permissions are not set. Line 41: successfull successful https://gerrit.ovirt.org/#/c/46907/8/vdsm_hooks/ovs/ovs_after_network_setup_fail.py File vdsm_hooks/ovs/ovs_after_network_setup_fail.py: Line 1: #!/usr/bin/env python hashbang/non-executable mismatch, as with other files Line 29: , : False) dict.get() would just return None. Line 34: In At https://gerrit.ovirt.org/#/c/46907/8/vdsm_hooks/ovs/ovs_before_network_setup.py File vdsm_hooks/ovs/ovs_before_network_setup.py: Line 82: running_config.networks.pop(net) Doesn't Python complain when you modify a dictionary while iterating it? You can use dict() to create a temporary copy on line 80. Line 85: running_config.bonds.pop(bond) Likewise. Line 94: Remove Removing (to keep uniformity) Line 96: Reconfigure Reconfiguring Line 135: inRollback No need to mimic camelCase :-) -- To view, visit https://gerrit.ovirt.org/46907 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8f6b63d03bb9579e260bfad1686047a431f69543 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Petr Horáček <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Ido Barkan <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Ondřej Svoboda <[email protected]> Gerrit-Reviewer: Petr Horáček <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
