Nir Soffer has posted comments on this change. Change subject: Support for mapping image ids to physical devices ......................................................................
Patch Set 5: Code-Review-1 (9 comments) Some details are still wrong. http://gerrit.ovirt.org/#/c/31465/5/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: output = '' Line 278: try: Line 279: stdout = subprocess.Popen(CMDLINE, stdout=subprocess.PIPE).stdin I don't think you can read from stdin of the process. You are supposed to write to this stream. But we don't need to be confused by this since using the higher level api is easier. Line 280: output = stdout.read() Line 281: stdout.close() Line 282: except (IOError, OSError): Line 283: logging.exception("Failed to retrieve disk mapping") Line 276: CMDLINE = '/usr/share/ovirt-guest-agent/diskmapper' Line 277: output = '' Line 278: try: Line 279: stdout = subprocess.Popen(CMDLINE, stdout=subprocess.PIPE).stdin Line 280: output = stdout.read() I don't think this will always work, for example, what if you are interrupted? Line 281: stdout.close() Line 282: except (IOError, OSError): Line 283: logging.exception("Failed to retrieve disk mapping") Line 284: Line 277: output = '' Line 278: try: Line 279: stdout = subprocess.Popen(CMDLINE, stdout=subprocess.PIPE).stdin Line 280: output = stdout.read() Line 281: stdout.close() You did not wait on the process, we have now a happy zombie. But we don't need to to this because the higher level api is doing this for us. Line 282: except (IOError, OSError): Line 283: logging.exception("Failed to retrieve disk mapping") Line 284: Line 285: result = {} Line 279: stdout = subprocess.Popen(CMDLINE, stdout=subprocess.PIPE).stdin Line 280: output = stdout.read() Line 281: stdout.close() Line 282: except (IOError, OSError): Line 283: logging.exception("Failed to retrieve disk mapping") I think you like to do this instead. First create the process - this can raise OSError, so handle it. Do not use strings for specifying commands. CMD = '/usr/share/ovirt-guest-agent/diskmapper' try: process = subprocess.Popen([CMD], stdout=subprocess.PIPE, stderr=subprocess.PIPE) except OSError: logging.exception(...) return {} Now let the process wait for completion and read its stdout and error streams. This should not raise anything. out, err = process.communicate() Handle errors: if process.returncode != 0: logging.error("diskmapper failed stderr=%s returncode=%d", err, process.returncode) return {} Line 284: Line 285: result = {} Line 286: for line in output.split('\n'): Line 287: if line: Line 281: stdout.close() Line 282: except (IOError, OSError): Line 283: logging.exception("Failed to retrieve disk mapping") Line 284: Line 285: result = {} result is ok, but I would call this "mapping", since function returns the disk mapping. Line 286: for line in output.split('\n'): Line 287: if line: Line 288: k, v = line.split('|', 1) Line 289: result[v] = {'name': k} Line 282: except (IOError, OSError): Line 283: logging.exception("Failed to retrieve disk mapping") Line 284: Line 285: result = {} Line 286: for line in output.split('\n'): Use splitlines() Line 287: if line: Line 288: k, v = line.split('|', 1) Line 289: result[v] = {'name': k} Line 290: return result Line 283: logging.exception("Failed to retrieve disk mapping") Line 284: Line 285: result = {} Line 286: for line in output.split('\n'): Line 287: if line: How can we get empty lines here? you control the tool right? I don't remember that you write such lines there. If we have empty line here, this is a severe error and we should not ignore it. Line 288: k, v = line.split('|', 1) Line 289: result[v] = {'name': k} Line 290: return result Line 291: Line 284: Line 285: result = {} Line 286: for line in output.split('\n'): Line 287: if line: Line 288: k, v = line.split('|', 1) Again, k and v are not good names. We must guess now what is the first argument and what is the second. And the split can fail if the tool did not write the expected format. This should be: try: name, serial = line.split('|', 1) except ValueError: log error and bail out - the tool is broken Line 289: result[v] = {'name': k} Line 290: return result Line 291: Line 292: def getMemoryStats(self): http://gerrit.ovirt.org/#/c/31465/5/scripts/diskmapper/diskmapper.libudev File scripts/diskmapper/diskmapper.libudev: Line 105: print '%s|%s' % (devnode, serial) Line 106: entry = ludev.udev_list_entry_get_next(entry) Line 107: finally: Line 108: ludev.udev_enumerate_unref(udevenum) Line 109: ludev.udev_unref(udev) I think this is too complex and unmaintainable. It will be much simpler and reliable to write tiny program in C using libudev. -- 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: 5 Gerrit-Project: ovirt-guest-agent Gerrit-Branch: master Gerrit-Owner: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@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
