Nir Soffer has posted comments on this change. Change subject: Support for mapping image ids to physical devices ......................................................................
Patch Set 7: (9 comments) http://gerrit.ovirt.org/#/c/31465/7/ovirt-guest-agent/GuestAgentLinux2.py File ovirt-guest-agent/GuestAgentLinux2.py: Line 37: CredServer = CredServerFake Line 38: Line 39: Line 40: def _readLinesFromProcess(cmdline): Line 41: process = None Not needed Line 42: try: Line 43: process = subprocess.Popen(cmdline, stdout=subprocess.PIPE, Line 44: stderr=subprocess.PIPE) Line 45: except OSError: Line 42: try: Line 43: process = subprocess.Popen(cmdline, stdout=subprocess.PIPE, Line 44: stderr=subprocess.PIPE) Line 45: except OSError: Line 46: logging.exception("Failed to run process %s", cmdline) Why can and should return here. This style, continuing after errors and exiting at a single point is bad idea. This has to be done in C, but it does not make sense in Python. Line 47: Line 48: read_lines = [] Line 49: if process: Line 50: out, err = process.communicate() Line 44: stderr=subprocess.PIPE) Line 45: except OSError: Line 46: logging.exception("Failed to run process %s", cmdline) Line 47: Line 48: read_lines = [] Not needed Line 49: if process: Line 50: out, err = process.communicate() Line 51: if process.returncode != 0: Line 52: logging.error("Process %s returned err code %d", cmdline, Line 45: except OSError: Line 46: logging.exception("Failed to run process %s", cmdline) Line 47: Line 48: read_lines = [] Line 49: if process: Check not needed since you should not reach this point if process creation failed. Line 50: out, err = process.communicate() Line 51: if process.returncode != 0: Line 52: logging.error("Process %s returned err code %d", cmdline, Line 53: process.returncode) Line 49: if process: Line 50: out, err = process.communicate() Line 51: if process.returncode != 0: Line 52: logging.error("Process %s returned err code %d", cmdline, Line 53: process.returncode) Why do you want to continue if the process failed? Line 54: else: Line 55: read_lines = out.splitlines() Line 56: return read_lines Line 57: Line 51: if process.returncode != 0: Line 52: logging.error("Process %s returned err code %d", cmdline, Line 53: process.returncode) Line 54: else: Line 55: read_lines = out.splitlines() Return out.splitlines() Line 56: return read_lines Line 57: Line 58: Line 59: class PkgMgr(object): Line 296: mapping = {} Line 297: for line in _readLinesFromProcess([CMD]): Line 298: try: Line 299: name, serial = line.split('|', 1) Line 300: mapping[serial] = {'name': name} This cannot raise ValueError, and should not be inside the the try block. This should be in the else: part of the try block. Line 301: except ValueError: Line 302: logging.exception("diskmapper tool reported invalid format") Line 303: # We're returning an empty dict on an error in the reported Line 304: # format and exit the loop Line 300: mapping[serial] = {'name': name} Line 301: except ValueError: Line 302: logging.exception("diskmapper tool reported invalid format") Line 303: # We're returning an empty dict on an error in the reported Line 304: # format and exit the loop This comment does not add any value, we can see this in the 2 lines bellow. Maybe you like to explain why do you want to return empty dict in this case? Line 305: mapping = {} Line 306: break Line 307: return mapping Line 308: Line 302: logging.exception("diskmapper tool reported invalid format") Line 303: # We're returning an empty dict on an error in the reported Line 304: # format and exit the loop Line 305: mapping = {} Line 306: break There is no need to set mapping and break, just return {} Line 307: return mapping Line 308: Line 309: def getMemoryStats(self): Line 310: try: -- 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: 7 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
