Nir Soffer has posted comments on this change. Change subject: Support for mapping image ids to physical devices ......................................................................
Patch Set 4: (2 comments) http://gerrit.ovirt.org/#/c/31465/4/ovirt-guest-agent/GuestAgentLinux2.py File ovirt-guest-agent/GuestAgentLinux2.py: 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}) > order, ok, make sense. Not sure why I had it reversed. Being forward compatible is important. So we can consider: {"name": {"serial": "value"}, ...} Or: {"serial": {"name": "value"}, ...} Is this format - list of dicts use elsewhere in the guest agent? Line 283: except Exception: Line 284: logging.exception("Failed to retrieve disk mapping") Line 285: return result Line 286: 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 > The tool is there to be able handling different Linux versions. This is not This fact probably need documentation here, where you run the tool. e.g. "The logic for getting the mapping vary across distributions, so we delegate to diskmapper tool". But this does not explain why we don't embed the code for disk mapping in the tool. You can detect the current distribution on startup, and then run the correct code: if supports_libudev: import udev diskmapper = udev.diskmaper elif rhel6: import sysfs diskmapper = sysfs.diskmapper ... If detecting the distribution is not easy, and you want to make this during packaging, we can do this by having multiple diskmapper.py modules with identical interface, and packing the right module for the current package. A separate tool is not need for portability. Having a separate tool makes sense when you are reusing existing tool, or you want to keep the agent safe from crashes in the relevant code, or if the tool should also run from the command line. So is this the right design for this case? Line 286: Line 287: def getMemoryStats(self): Line 288: try: Line 289: self._get_meminfo() -- 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
