Code review comment for lp:~fcorrea/landscape-client/add-vminfo-to-computer-info-message

Revision history for this message
Thomas Herve (therve) wrote :

[1] You don't have any specific tests for _get_vm_info? I thought that was the point of passing root_path.

[2]
+ xen_info = [u"proc/sys/xen", u"sys/bus/xen", u"proc/xen"]

There is no need to have unicode paths.

[3]
+ elif True in map(exists, xen_info):

It's slightly more explicit to me to do "elif filter(exists, xen_info):"

[4]
+ elif "QEMU Virtual CPU" in open(
+ join_root_path(u"proc/cpuinfo")).read():
+ virt_info = "KVM"

You should close the open file explicitly.

[5]
+import os.path

You can import os directly.

Thanks!

review: Needs Fixing

« Back to merge proposal