Edward Haas has posted comments on this change. Change subject: net: Mapping bonding option value names to numerics ......................................................................
Patch Set 25: (3 comments) https://gerrit.ovirt.org/#/c/49390/25/lib/vdsm/netinfo/bonding.py File lib/vdsm/netinfo/bonding.py: Line 209: try: : opt_num_val = bond_opts_map[mode_num][option_name][val_name] : except KeyError: : opt_num_val = None > this can be also written as: I understand, the idea was to protect all dics because it is a module api. On the other hand, it should not happen as far as I know. Unit Tests cover unsupported options and the input of options is coming from the saved configuration. Seems safer to handle all possible errors in one shot, but I'm not sure which is better, let me know if you still think I should change it. https://gerrit.ovirt.org/#/c/49390/25/lib/vdsm/tool/dump_bonding_opts.py File lib/vdsm/tool/dump_bonding_opts.py: Line 82: with _bond_device(bond_name): > you could just move the context manager to wrap only the statement that is _scan_modes uses _change_mode to set the bond mode. _bond_device creates a bond. Therefore, you cannot call _scan_mode outside the context (it expects the bond to exist). Line 119: with open(BONDING_MASTERS, 'w') as bonds: : bonds.write('-' + bond_name) : bonds.flush() : bonds.write('+' + bond_name) > but this can be replaced with: See previous response. -- To view, visit https://gerrit.ovirt.org/49390 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1f4c16b1523822a2d53d4525841ff8741af6296c Gerrit-PatchSet: 25 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Edward Haas <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Edward Haas <[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: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
