Dan Kenigsberg has posted comments on this change. Change subject: net: SetupNetworks small cleanups. ......................................................................
Patch Set 1: Code-Review-1 (7 comments) https://gerrit.ovirt.org/#/c/42022/1//COMMIT_MSG Commit Message: Line 6: Line 7: net: SetupNetworks small cleanups. Line 8: Line 9: Extracted the logic that alters network parameters for bonding and nics Line 10: to functions, did some internal renaming and few small cleanups. I don't believe I understand/agree with the motivation of this patch. Line 11: Line 12: Change-Id: Ib054e144dccbff40b40b5967dca0bf840bce8ab9 https://gerrit.ovirt.org/#/c/42022/1/lib/vdsm/netconfpersistence.py File lib/vdsm/netconfpersistence.py: Line 412: return [nic + vlan] Line 413: else: # isolated bridged network Line 414: return [] Line 415: Line 416: ? Line 417: def unicodize(d): https://gerrit.ovirt.org/#/c/42022/1/vdsm/network/api.py File vdsm/network/api.py: Line 681: ne.ERR_BAD_PARAMS, "No bonding option given, nor existing " Line 682: "bond %s found." % bondName) Line 683: Line 684: Line 685: def update_underlying_device_params(attrs, bondings, _netinfo): For me, this function makes bad things worse. The point is that its output is *not* network attributes, and not "underlying device params" (whatever that may be). The content of the returned dictionary is simply the arguments of the _addNetwork function. (or _editNetwork function). So a rename of this function (and net_attr) is a must. Also, I do not see how we can possibly want to make it public. Line 686: """update a specific network attributes with relevant bonding/ nic Line 687: info.""" Line 688: net_attr = dict(attrs) Line 689: if 'bonding' in net_attr: Line 694: [net_attr.pop('nic')] if 'nic' in net_attr else []) Line 695: return net_attr Line 696: Line 697: Line 698: def build_bond_options(bond_name, bondings, _netinfo): I believe this should remain private Line 699: """If VDSM is alsed to cinfigure a network over a bonding there are 2 Line 700: options: Line 701: 1. VDSM needs to create this bond. This is indicated by describing the bond Line 702: and it's nics in the 'bonding' dictionary. Line 699: alsed to cinfigure asked to configure Line 703: bu by Line 707: Etherway Eitherway -- To view, visit https://gerrit.ovirt.org/42022 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib054e144dccbff40b40b5967dca0bf840bce8ab9 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Ido Barkan <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
