Ido Barkan has posted comments on this change. Change subject: network: add ifcfg write hook ......................................................................
Patch Set 1: Code-Review-1 (4 comments) https://gerrit.ovirt.org/#/c/46372/1/vdsm/network/configurators/ifcfg.py File vdsm/network/configurators/ifcfg.py: Line 546: logging.debug('ignoring variable %s', k) Line 547: Line 548: ifcfg_file = netinfo.NET_CONF_PREF + name Line 549: hook_dict = _build_ifcfg_write_hook_dict(name, Line 550: netinfo.NET_CONF_PREF + name, why don't you pass just ifcfg_file here? Line 551: cfg) Line 552: hook_return = hooks.before_ifcfg_write(hook_dict) Line 553: ifcfg_file = hook_return['ifcfg_file'] Line 554: cfg = hook_return['config'] Line 549: hook_dict = _build_ifcfg_write_hook_dict(name, Line 550: netinfo.NET_CONF_PREF + name, Line 551: cfg) Line 552: hook_return = hooks.before_ifcfg_write(hook_dict) Line 553: ifcfg_file = hook_return['ifcfg_file'] I think we should ignore the returned ifcfg_file path. We can safely assume that for all puposes the hook needs to know about it, but will never need to change it. Changing the path can break all sorts of assumption Vdsm already has on ifcfg file paths, especially around persistence. Line 554: cfg = hook_return['config'] Line 555: self.writeConfFile(ifcfg_file, cfg) Line 556: Line 557: def addBridge(self, bridge, **opts): Line 552: hook_return = hooks.before_ifcfg_write(hook_dict) Line 553: ifcfg_file = hook_return['ifcfg_file'] Line 554: cfg = hook_return['config'] Line 555: self.writeConfFile(ifcfg_file, cfg) Line 556: shouldn't there be a call to hooks.after_ifcfg_write here? Line 557: def addBridge(self, bridge, **opts): Line 558: """ Create ifcfg-* file with proper fields for bridge """ Line 559: conf = 'TYPE=Bridge\nDELAY=0\n' Line 560: opts['hotplug'] = 'no' # So that udev doesn't trigger an ifup Line 946: Line 947: return ifcfgs Line 948: Line 949: Line 950: def _build_ifcfg_write_hook_dict(name, ifcfg_file, conf): I would just inline this function, as it is a short dict literal. calling it 'hook_dict' already serves as built in documentation. Line 951: hook_dict = {'name': name, Line 952: 'ifcfg_file': ifcfg_file, Line 953: 'config': conf} -- To view, visit https://gerrit.ovirt.org/46372 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2206ccf97b210bd58f03777d4ccd016785b02939 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: ovirt-3.6 Gerrit-Owner: Marcin Mirecki <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Ido Barkan <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Marcin Mirecki <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
