Fix the FileNotFoundError of get_model_features() #2052
Conversation
|
The test about virsh.dumpxml is failed because of a calling to get_model_features() I can get the test about virsh.dumpxml passed by rewriting get_model_features() [root@RHEL8 tp-libvirt]# avocado run --vt-type libvirt virsh.dumpxml |
virttest/utils_misc.py
Outdated
| output = open(conf_dir + file, "r").read() | ||
| model = ET.fromstring(output) | ||
| for feature in model.findall("model/feature"): | ||
| features.append(feature.get('name')) |
There was a problem hiding this comment.
The code dealing with xml for both scenarios is the same logic, please simplify the code.
There was a problem hiding this comment.
I agree with @yafu-1, please re-factor to avoid redundancy.
virttest/utils_misc.py
Outdated
| #Find model in /usr/share/libvirt/cpu_map | ||
| if os.path.isdir(conf_dir) and os.path.exists(conf_dir): | ||
| filelist = os.listdir(conf_dir) | ||
| for file in filelist: |
There was a problem hiding this comment.
file is a keyword in python.
virttest/utils_misc.py
Outdated
| output = open(conf_dir + file, "r").read() | ||
| model = ET.fromstring(output) | ||
| for feature in model.findall("model/feature"): | ||
| features.append(feature.get('name')) |
There was a problem hiding this comment.
I agree with @yafu-1, please re-factor to avoid redundancy.
virttest/utils_misc.py
Outdated
| if nested_model is not None: | ||
| nested_model_name = nested_model.get('name') | ||
| if os.path.isfile(conf) and os.path.exists(conf): | ||
| output = open(conf, 'r').read() |
There was a problem hiding this comment.
better to use with statements.
There was a problem hiding this comment.
thanks for helping me review the code.
I will fix the code soon.
virttest/utils_misc.py
Outdated
| for feature in model_n.findall('feature'): | ||
| features.append(feature.get('name')) | ||
| break | ||
| if os.path.isfile(conf) and os.path.exists(conf): |
There was a problem hiding this comment.
No need for the "and" part for judgement
virttest/utils_misc.py
Outdated
| features.append(feature.get('name')) | ||
| break | ||
| # Find model in dir /usr/share/libvirt/cpu_map | ||
| if os.path.isdir(conf_dir) and os.path.exists(conf_dir): |
| @@ -1,5 +1,5 @@ | |||
| [provider] | |||
| uri: https://github.com/autotest/tp-libvirt.git | |||
| uri: file:///root/code/tp-libvirt | |||
There was a problem hiding this comment.
please remove this change.
virttest/utils_misc.py
Outdated
| filelist = os.listdir(conf_dir) | ||
| for file_name in filelist: | ||
| if model_name in file_name: | ||
| with open(conf_dir + file, "r").read() as output: |
There was a problem hiding this comment.
- It would break here as
file_nameto be used ? - please use
os.path.join()
| continue | ||
| else: | ||
| break | ||
| # Find model in dir /usr/share/libvirt/cpu_map |
| @@ -4024,26 +4025,36 @@ def get_model_features(model_name): | |||
| """ | |||
| features = [] | |||
| conf = "/usr/share/libvirt/cpu_map.xml" | |||
| conf_dir = "/usr/share/libvirt/cpu_map/" | |||
There was a problem hiding this comment.
conf = "/usr/share/libvirt/cpu_map.xml"
# check the libvirt version to branch out instead of path
if libvirt_version.version_compare(5, 0, 0):
conf = "/usr/share/libvirt/cpu_map/"
# we should raise the error if the path doesn't available
if not os.path.exists(conf):
test.error()
with open(conf, 'r') as output:
:::
else:
:::There was a problem hiding this comment.
@balamuruhans
Thank you very much for helping review the code and pointing out my code errors !
There was a problem hiding this comment.
The utils_misc.py file path is "~/avocado-vt/virttest/utils_misc.py"
The libvirt_version.py file path is "tp-libvirt/provider/ libvirt_version.py "
I can not import libvirt_version.version_compare() directly in this utils_misc.py.
Should I write a function to get libvirt version, or what can I do so that I can import the libvirt_version.version_compare() ?
There was a problem hiding this comment.
I would suggested to move the code from tp-libvirt/provider/libvirt_version.py to avocado-vt/virttest/ and use it.
There was a problem hiding this comment.
@yanghangliu @dzhengfy @balamuruhans looks like moving libvirt_version.py needs a change in many tests, for now we can copy libvirt_version.py to virttest and use in this testcase and slowly change import in other testcases through different PR and once all movement happened, we can safely remove tp-libvirt/provider/libvirt_version.py
There was a problem hiding this comment.
yes, once moving to virttest, we will have to send separate PR to tp-libirt to adopt it by changing all the testcases using it from virttest and remove the file in provider
virttest/utils_misc.py
Outdated
| features.append(feature.get('name')) | ||
| break | ||
| if not libvirt_version.version_compare(5, 0, 0): | ||
| if os.path.exists(conf): |
There was a problem hiding this comment.
For compare version, no need to judge the file exist or not. If not exist, it should be error. Also the following part.
|
This pull request introduces 163 alerts when merging 132ce0f into 40c351f - view on LGTM.com new alerts:
|
|
@yanghangliu, we have to fix the cyclic import issue, I have tried to clean utils_misc.py in #2103 to break the cyclic imports. Feel free to review it. Thanks! |
There is no /usr/share/libvirt/cpu_map.xml file existing on RHEL8， but instead of this file with /usr/share/libvirt/cpu_map directory. If a test which calls the get_model_features() is runned on RHEL8, an FileNotFoundError will be thrown resulting in making this test failed. Signed-off-by: Yanghang Liu <[email protected]>
|
The import cycle resolved by remove the virsh import. Thanks |
There is no /usr/share/libvirt/cpu_map.xml file existing on RHEL8，
but instead of this file with /usr/share/libvirt/cpu_map directory.
If a test which calls the get_model_features() is runned on RHEL8,
an FileNotFoundError will be thrown resulting in making this test
failed.
Signed-off-by: Yanghang Liu [email protected]