Assaf Muller has posted comments on this change. Change subject: netinfo: Determine bootproto also without ifcfg files ......................................................................
Patch Set 3: Code-Review-1 (5 comments) http://gerrit.ovirt.org/#/c/23098/3//COMMIT_MSG Commit Message: Line 9: DHCP version (4 or 6) is detected reliably only for dhclient, Line 10: dhcpcd allows setting the version in its configuration file. Line 11: Line 12: Parsing of configuration files would get very complex. Line 13: I'm eagerly awaiting your findings about lease files as a better approach. I have a hunch it'd be a significantly simpler solution. Line 14: I will investigate if 'lease' files are a better fit. Line 15: They certainly use less expressive syntax and we avoid Line 16: having to simulate the interplay between cmdline arguments Line 17: and configuration; and possibly we can even tell if an http://gerrit.ovirt.org/#/c/23098/3/lib/vdsm/netinfo.py File lib/vdsm/netinfo.py: Line 445: return None Line 446: else: Line 447: raise NotImplementedError Line 448: Line 449: This function is confusing / doesn't stand on its own at the module level. I'd advise moving it as an inline function in 'get()'. Line 450: def _getBootProtocols(networks): Line 451: ifaces = {} Line 452: for net, info in networks.iteritems(): Line 453: ifaces[info['iface']] = net Line 528: # This possibly includes: defaulting to all interfaces; dhcpcd's globbing; Line 529: # explicit/implicit configuration files (of different nature, indeed) Line 530: # Line 531: # A last resort: lease files created by daemons _somewhere_ Line 532: Please add an inline comment here about this next section being a fallback for each network. Maybe in an inline function instead. Line 533: for netinfo in networks.itervalues(): Line 534: bootproto = None Line 535: if 'bootproto4' not in netinfo: Line 536: bootproto = getBootProtocol(netinfo['iface']) Line 529: # explicit/implicit configuration files (of different nature, indeed) Line 530: # Line 531: # A last resort: lease files created by daemons _somewhere_ Line 532: Line 533: for netinfo in networks.itervalues(): netinfo for the attributes dictionary of a network is a very unfortunate name, as it has different connotations for VDSM developers. Line 534: bootproto = None Line 535: if 'bootproto4' not in netinfo: Line 536: bootproto = getBootProtocol(netinfo['iface']) Line 537: netinfo['bootproto4'] = bootproto Line 530: # Line 531: # A last resort: lease files created by daemons _somewhere_ Line 532: Line 533: for netinfo in networks.itervalues(): Line 534: bootproto = None With how getBootProtocol is currently implemented it only gets 'bootproto' from ifcfg / unified persistence, and doesn't care about ipv6. With that in mind you can simplify the code to: if 'bootproto4' not in netinfo: netinfo['bootproto4'] = getBootProtocol(netinfo['iface']) We'll have to track the issue of getBootProtocol and IPv6 though... Currently that function is used once, to know how to remove source routing when a network is deleted. It won't work for IPv6 as it is now. Line 535: if 'bootproto4' not in netinfo: Line 536: bootproto = getBootProtocol(netinfo['iface']) Line 537: netinfo['bootproto4'] = bootproto Line 538: if 'bootproto6' not in netinfo: -- To view, visit http://gerrit.ovirt.org/23098 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5fbc48a0adf5f40120a72ec2c4cc2fc80b7226b8 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Ondřej Svoboda <osvob...@redhat.com> Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com> Gerrit-Reviewer: Assaf Muller <amul...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Ondřej Svoboda <osvob...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
