Federico Simoncelli has posted comments on this change.

Change subject: qemuimg: Handle new output format
......................................................................


Patch Set 6:

(2 comments)

http://gerrit.ovirt.org/#/c/27552/6/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 86:             'virtualsize': int(__iregexSearch("virtualsize", out[2])),
Line 87:         }
Line 88: 
Line 89:         # Scan for optional fields in the output
Line 90:         row = 4
> Not a fan of magic numbers, but that was also in the old code - you actuall
We probably need a constant here. E.g.:

 row = INFO_OPTFIELDS_STARTIDX
Line 91:         for field, filterFn in (('clustersize', int), ('backingfile', 
str)):
Line 92:             try:
Line 93:                 info[field] = filterFn(__iregexSearch(field, out[row]))
Line 94:                 row = row + 1


Line 94:                 row = row + 1
Line 95:             except (_RegexSearchError, IndexError):
Line 96:                 pass
Line 97:     except _RegexSearchError:
Line 98:         raise QImgError(rc, out, err, "unable to parse qemu-img info 
output")
> what about narrowing down this try/except clause by moving the except to li
this seems easy enough to be fixed now, we just need to move the except block 
to line 88. Right?
Line 99: 
Line 100:     return info
Line 101: 
Line 102: 


-- 
To view, visit http://gerrit.ovirt.org/27552
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic229198ab7c2bb9743bdf8629416131186115431
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <[email protected]>
Gerrit-Reviewer: Adam Litke <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Francesco Romani <[email protected]>
Gerrit-Reviewer: Nir Soffer <[email protected]>
Gerrit-Reviewer: [email protected]
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Reply via email to