Vinzenz Feenstra has posted comments on this change. Change subject: Support for mapping image ids to physical devices ......................................................................
Patch Set 4: (11 comments) http://gerrit.ovirt.org/#/c/31465/4/ovirt-guest-agent/GuestAgentLinux2.py File ovirt-guest-agent/GuestAgentLinux2.py: 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 I will switch to subprocess, I merely kept the style of how it is done in this file. I wasn't sure that subprocess is already there in 2.4 but when I just checked, it is. Deprecation since 2.6 is not really a concern, since I have to support 2.4 syntax as well. I have to ensure that things work without changes across the different python versions. At least for now. 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 "|", th you're right, I did not think of that, will fix. 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": "seria order, ok, make sense. Not sure why I had it reversed. Probably it's ok to have the {'name': 'serial'} way, however it won't be extensible in future, and I try to keep data formatting extensible. This is for the VDSM <-> Guest Protocol, therefore rather named fields, where we can add later something else, than having now key/value pairs and it needs an entire new data structure to keep backwards compatibility. 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 true, I generally should start doing it in the guest agent ;-) 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, The tool is there to be able handling different Linux versions. This is not about efficiency but portability. I have to support so many different distributions already, that it is simpler to install the working version for this task as a separate external script. That's why you can see that there's a version for el5, el6 and a libudev based on. 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? no reason, I am not always sure if I am using some bashisms or not, so to be "safe" I use 'bash' 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? el5 /sys/block/$DEVICE/device/serial el6 /sys/block/$DEVICE/serial I prefer having the separate, but I might even drop diskmapper.el6 in favor of the currently "more portable" libudev version. Please note that with "more portable" I mean that it works on way more supported linux based systems (fedora, el6, el7... basically it works where libudev is) http://gerrit.ovirt.org/#/c/31465/4/scripts/diskmapper/diskmapper.el6 File scripts/diskmapper/diskmapper.el6: Line 1: #!/bin/sh Line 2: Line 3: for DEVICE in `ls /sys/block`; > DEVICE is a variable, not a constant, or not a value from the environment - no, reason, will make it lowercase Line 4: do Line 5: if [ -e /sys/block/$DEVICE/serial ]; Line 6: then Line 7: echo -n /dev/$DEVICE Line 1: #!/bin/sh Line 2: Line 3: for DEVICE in `ls /sys/block`; Line 4: do Line 5: if [ -e /sys/block/$DEVICE/serial ]; > I think that -f is better here, you are looking for a file, right? Yeah I am looking for a file Line 6: then Line 7: echo -n /dev/$DEVICE Line 8: echo -n '|' Line 9: cat /sys/block/$DEVICE/serial Line 6: then Line 7: echo -n /dev/$DEVICE Line 8: echo -n '|' Line 9: cat /sys/block/$DEVICE/serial Line 10: echo > Contents of sysfs items typically terminated by a newline, but it can be al ok thanks, will do it that way Line 11: fi 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 It has the advantage that it works, sysfs is very unstable as an API. It changes all the time. el5 - I can do it via sysfs el6 - I can do it via sysfs but it's different from el5 f20 - I couldn't even figure out how to do it via sysfs Let's say that this script is the preferred way for me of doing it. I am not a big fan of the fact that it uses ctypes, however it solves my problem for many distributions, which makes this the preferred way. -- 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
