Francesco Romani has posted comments on this change. Change subject: Implementation for CPU topology reporting ......................................................................
Patch Set 5: (9 comments) http://gerrit.ovirt.org/#/c/23268/5/ovirt-guest-agent/Makefile.am File ovirt-guest-agent/Makefile.am: Line 9: GuestAgentLinux2.py \ Line 10: OVirtAgentLogic.py \ Line 11: VirtIoChannel.py \ Line 12: ovirt-guest-agent.py \ Line 13: topology.py \ tabs vs spaces? Line 14: $(NULL) Line 15: Line 16: noinst_PYTHON=\ Line 17: GuestAgentWin32.py \ http://gerrit.ovirt.org/#/c/23268/5/ovirt-guest-agent/topology.py File ovirt-guest-agent/topology.py: Line 17: # Line 18: import os.path Line 19: Line 20: Line 21: def _file(*args): nit: maybe something node/cpunode? _file is probably too generic here. Line 22: return os.path.join('/sys/devices/system/cpu', *args) Line 23: Line 24: Line 25: def _range_to_list(data): Line 37: Line 38: def _read_file(*parts): Line 39: f = open(_file(*parts), 'r') Line 40: result = f.read().replace('\n', '') Line 41: f.close() Can we use with here or we are constraint by the support of ancient python versions? Line 42: return result Line 43: Line 44: Line 45: def _read_range_file_to_list(file): Line 66: return _read_file('online') Line 67: Line 68: Line 69: def _online_list(): Line 70: return _read_range_file_to_list('online') As matter of personal taste I don't know if the 5 one-liners [_present_range, _online_list] above really make the code clearer. I'm inclined to say no, at least at first glance. Again, it is personal taste so it can stay this way. Line 71: Line 72: Line 73: def read_topology(detailed=False): Line 74: cores = set() Line 78: for id in _online_list(): Line 79: thread = _read_topology_file(id, 'thread_siblings_list') Line 80: socket = _read_topology_file(id, 'physical_package_id') Line 81: if not thread in cores: Line 82: cores.add(thread) Uhm, I don't get why we need the if check, if cores is a set the add() alone should be fine, it isn't? Line 83: if not socket in sockets: Line 84: sockets.add(socket) Line 85: details.append({ Line 86: 'id': id, Line 80: socket = _read_topology_file(id, 'physical_package_id') Line 81: if not thread in cores: Line 82: cores.add(thread) Line 83: if not socket in sockets: Line 84: sockets.add(socket) ditto Line 85: details.append({ Line 86: 'id': id, Line 87: 'thread': thread, Line 88: 'socket': socket}) Line 100: result['details'] = details Line 101: return result Line 102: Line 103: Line 104: if __name__ == '__main__': Code seems ok but I'd add a couple of unit test, even just for reference and to document a known good output. Line 105: from pprint import pprint Line 106: print 'Topology:' Line 107: pprint(read_topology()) Line 108: print 'Topology detailed:' http://gerrit.ovirt.org/#/c/23268/5/tests/message_validator.py File tests/message_validator.py: Line 112: Line 113: Line 114: def validate_network_interfaces(msg): Line 115: assert(msg['__name__'] == 'network-interfaces') Line 116: assertIn('interfaces', msg) I think this is a welcome improvement, but unrelated here? Line 117: assert(isinstance(msg['interfaces'], list)) Line 118: for obj in msg['interfaces']: Line 119: assert_string_param(obj, 'hw') Line 120: assert_string_param(obj, 'name') Line 134: assert_integral_param(disk, 'used') Line 135: Line 136: Line 137: def validate_memory_stats(msg): Line 138: assertIn('memory', msg) ditto Line 139: mem = msg['memory'] Line 140: assert_integral_param(mem, 'majflt') Line 141: assert_integral_param(mem, 'mem_free') Line 142: assert_integral_param(mem, 'mem_total') -- To view, visit http://gerrit.ovirt.org/23268 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If937fd33d2efa11425c814188df2087a85af8357 Gerrit-PatchSet: 5 Gerrit-Project: ovirt-guest-agent Gerrit-Branch: master Gerrit-Owner: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
