Nir Soffer has posted comments on this change. Change subject: Support for mapping image ids to physical devices ......................................................................
Patch Set 4: (9 comments) http://gerrit.ovirt.org/#/c/31465/4/ovirt-guest-agent/GuestAgentLinux2.py File ovirt-guest-agent/GuestAgentLinux2.py: Line 272: Line 273: return usages Line 274: Line 275: def getDiskMapping(self): Line 276: cmdline = '/usr/share/ovirt-guest-agent/diskmapper' This is a constant, should be UPPERCASE and defined where other constants are used. Line 277: result = [] Line 278: try: Line 279: for line in os.popen(cmdline).read().split('\n'): Line 280: if line: Line 275: def getDiskMapping(self): Line 276: cmdline = '/usr/share/ovirt-guest-agent/diskmapper' Line 277: result = [] Line 278: try: Line 279: for line in os.popen(cmdline).read().split('\n'): We can simplify this by iterating over the file returned from os.popen. It is also nice to close the file when we are done. Both can be achived using: with os.popen(cmdline) as f: for line in f: ... But os.popen is deprecated since 2.6 - you should use subprocess module for this in new code. Line 280: if line: Line 281: k, v = line.split('|') Line 282: result.append({'serial': v, 'name': k}) Line 283: except Exception: Line 277: result = [] Line 278: try: Line 279: for line in os.popen(cmdline).read().split('\n'): Line 280: if line: Line 281: k, v = line.split('|') Split only once - your format is name|serial - if serial contains a "|", this split will raise, because the number of variable on the left does not match the number of items in the tuple. Even if the serial cannot contain "|", it is more correct to split exactly the number of times that you want. Also these names are confusing - this is not "key" and "value", but "name" and "serial". So this should be: name, serial = line.split('|', 1) Line 282: result.append({'serial': v, 'name': k}) Line 283: except Exception: Line 284: logging.exception("Failed to retrieve disk mapping") Line 285: return result Line 278: try: Line 279: for line in os.popen(cmdline).read().split('\n'): Line 280: if line: Line 281: k, v = line.split('|') Line 282: result.append({'serial': v, 'name': k}) This format allows duplicate names. Why not simpler format, {"name": "serial"}? Also just to be easier to read, why not keep the same order of the original output? name, serial = line.split('|', 1) results.append({'name': name, 'serial': serial}) Line 283: except Exception: Line 284: logging.exception("Failed to retrieve disk mapping") Line 285: return result Line 286: Line 279: for line in os.popen(cmdline).read().split('\n'): Line 280: if line: Line 281: k, v = line.split('|') Line 282: result.append({'serial': v, 'name': k}) Line 283: except Exception: Please check only the exceptions that os.popen can raise - probably IOError and or OSError. The current code will handle errors in function, for example, synatx error in the parsing code. This is an error that this function cannot handle, and should let the caller handle it. Line 284: logging.exception("Failed to retrieve disk mapping") Line 285: return result Line 286: Line 287: def getMemoryStats(self): Line 281: k, v = line.split('|') Line 282: result.append({'serial': v, 'name': k}) Line 283: except Exception: Line 284: logging.exception("Failed to retrieve disk mapping") Line 285: return result It is not clear why do we need the diskmapper tool - instead of running it, we can implement it here much less code: mapping = {} for path in glob.glob('/sys/block/*/serial'): name = os.path.basename(os.path.dirname(path)) with open(path) as f: serial = f.readline().rstrip() mapping[name] = serial return mapping This is more efficient and simpler. Line 286: Line 287: def getMemoryStats(self): Line 288: try: Line 289: self._get_meminfo() http://gerrit.ovirt.org/#/c/31465/4/scripts/diskmapper/diskmapper.el5 File scripts/diskmapper/diskmapper.el5: Line 1: #!/bin/bash Do you have reason to use "bash"? wouldn't "sh" be enough? Line 2: Line 3: for DEVICE in `ls /sys/block`; Line 4: do Line 5: if [ -e /sys/block/$DEVICE/device/serial ]; then Line 7: echo -n '|' Line 8: cat /sys/block/$DEVICE/device/serial Line 9: echo Line 10: fi Line 11: done Is there a difference between this file and el6 one? Can we use same file for both cases? http://gerrit.ovirt.org/#/c/31465/4/scripts/diskmapper/diskmapper.libudev File scripts/diskmapper/diskmapper.libudev: Line 88: print '%s|%s' % (devnode, serial) Line 89: entry = ludev.udev_list_entry_get_next(entry) Line 90: finally: Line 91: ludev.udev_enumerate_unref(udevenum) Line 92: ludev.udev_unref(udev) Does this have any advantage compared with the simple shell script used in el? If this does the same thing as the shell script, I think we should not add this complexity, and risk stability using ctypes. -- To view, visit http://gerrit.ovirt.org/31465 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie8961b3703703df573dbf2f0fca2143b463ae76b Gerrit-PatchSet: 4 Gerrit-Project: ovirt-guest-agent Gerrit-Branch: master Gerrit-Owner: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <mskri...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
