Ondřej Svoboda has posted comments on this change. Change subject: netinfo: rework reporting of DHCPv4/6 on network devices ......................................................................
Patch Set 9: (1 comment) https://gerrit.ovirt.org/#/c/46430/9/lib/vdsm/netinfo.py File lib/vdsm/netinfo.py: Line 582: Line 583: return updated_networking Line 584: Line 585: Line 586: def _update_nested_dict(original, replacement): > I think this should be implemented a s a straight forward update of a dicti Basically, we're dealing here with 4 subdictionaries (bondings={}, bridges={}, nics={}, vlans={}) of netinfo, and we may be updating any of it, depending on the nature of networks defined: some can be based on nics, others on bondings, others on VLANs, and the rest (should be the biggest part) are bridged. There can be a lot of networks based on any combination and amount of these devices. This means that there would be a general function to update the given subdictionary, one of (bondings={}, bridges={}, nics={}, vlans={}), because all of these devices have the dhcpv4/6 and cfg subdictionaries. This function would iterate over the individual devices and replace the dhcpv4/6 strings. Something like devices[device]['dhcpv4'] = replacement_devices[device]['dhcpv4']. It wouldn't be hard to replace the one field we know about, in cfg dictionary, but the line would be even longer. Remember that a caller of this function (I think it would have to be netinfo.get) would have to invoke it four times for each device type, and we get this, and the inner iteration, for free if we use _update_nested_dict. The solution I describe is specific to the topology of netinfo and (inherently?) a bit more complex... Both the current solution and this proposed one are okay to me, but I like the current one because I think it is just clear that _propose_updates_to_reported_dhcp knows how netinfo looks like (and this way, this knowledge is in one place only), and I trust _update_nested_dict to update netinfo, however its input changes (we will remove cfg one day, we may add something else). Imagine we would like to use _update_nested_dict in a different place... If you still want to go for the specific function, I'll try to prepare it. Line 587: """ Line 588: Use a replacement dict's fields in two ways: if a value is a dictionary, Line 589: update it recursively. Otherwise replace the value (e.g. a string). Line 590: """ -- To view, visit https://gerrit.ovirt.org/46430 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaffdc836f8f64ecdc0a7e37ef50c228032f99696 Gerrit-PatchSet: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Ondřej Svoboda <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[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: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
