Marcin Mirecki has posted comments on this change. Change subject: network: add ifcfg write hook ......................................................................
Patch Set 1: (3 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? This is before we write the file, so there is no file yet. Instead we pass the name of the file and its contents. If the user modifies that, the returned values are used to write the file. 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 The asumption is that if the user uses this at his own risk. If one uses the hook, one has to take care of all the possible consequences. 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? Right! We discussed if it should, or should not be for so long that I actually missed this. 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 -- 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
