CP-47555 Porting usb_scan.py to python3

[acefei]

Test case passed job: 3923182
Test suite passed job: 194221 (Dev Run)
The manual test follows:

[codecov]

Codecov Report

Merging #5424 (1a34e20) into feature/py3 (411ae6a) will increase coverage by 24.6%.
The diff coverage is 100.0%.

Additional details and impacted files

@@              Coverage Diff               @@
##           feature/py3   #5424      +/-   ##
==============================================
+ Coverage         55.2%   79.9%   +24.6%     
==============================================
  Files               18       5      -13     
  Lines             2075     419    -1656     
==============================================
- Hits              1147     335     -812     
+ Misses             928      84     -844     

Files	Coverage Δ
python3/unittest/import_file.py	100.0% <100.0%> (ø)
python3/unittest/test_hfx_filename.py	100.0% <100.0%> (ø)
python3/unittest/test_usb_scan.py	99.2% <100.0%> (ø)

... and 13 files with indirect coverage changes

Flag	Coverage Δ
python2.7	59.1% <ø> (+5.5%)	⬆️
python3.11	99.5% <100.0%> (-0.5%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

[acefei]

Rebase the feature branch and rebuild the package.
Test job: 3923331
Ring3 BVT + BST: 194240

[gangj]

I think it would be better to resolve this pylint warning.

[bernhardkaindl]

Unresolved this discussion because it was set to resolved without a comment.

• This makes it hidden for review, and it causes that one cannot follow why this discussion was closed.

• and, the type analysis warning was not resolved (fixed), but disabled (without an explanation)

• This is an assertion function, and this assertion is in an if msg:. Such conditionals in assertion functions are not safe (e.g. passing msg just means there is no test of msg).

  • Instead, the original test cases that would want to check the content of msg should have this assertion instead.

[acefei]

I have a comments there that is an explain why disable pytype error, pytype think cm.exception.code is int, but actually its output is a str.

with the original author's thoughts, if testing usb without config, there is no msg need to be checked (285). in the rest of testing, it'll verify the common testcase with usb-policy.conf, that would check the content of msg should have this assertion.
I think it worked well, we don't need to change here, so just disable the pytype rule here.

  284     def test_usb_config_missing(self):
  285         self.verify_usb_exit([], [], [], "not_exist.conf")
  286
  287     def verify_usb_config_error_common(self, content, msg):
  288         path = os.path.join(self.work_dir, "usb-policy.conf")
  289         with open(path, "w") as f:
  290             f.write(content)
  291         self.verify_usb_exit([], [], [], path, msg)

[acefei]

Rebased and Tested 3933165 and 194764 (Dev Run)

[gangj]

This script "usb_scan.py" is used in xapi just by executing it without arguments: https://github.com/xapi-project/xen-api/blob/master/ocaml/xapi/xapi_pusb_helpers.ml#L98, and @acefei have manually verified that the output from this updated py3 version script is the same as the output from the original py2 version, so the change here should be ok.

[gangj]

3933165 passed (build hash: 2be1f82).
And please squash the commits before merging.

[psafont]

There are some failre code paths that have been changed but are untested by unit-tests, have these been manually tested? (when usbs are overriden or removed)

[acefei]

There are some failre code paths that have been changed but are untested by unit-tests, have these been manually tested? (when usbs are overriden or removed)

Job 3933165 tested as the highlight below.

[psafont]

Looks good, only thing left is the removal of python2 tests

[lindig]

This has two ✔️ - shall we merge it?

[bernhardkaindl]

I'm sorry, I was asked if all the pylint warnings that are disabled in this PR are safe to disable, and at least one is not safe:

"unspecified-encoding" is not safe to ignore blindly for all files.

The reason is that files can contain UTF-8 characters or even non UTF-8 bytes > ord(127), that are not valid UTF-8 and this caused crashing the auto-cert-kit XAPI plugin last year:

LC_ALL=C python3.6 -c 'open("/usr/share/hwdata/pci.ids").read()'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/lib64/python3.6/encodings/ascii.py", line 26, in decode
    return codecs.ascii_decode(input, self.errors)[0]
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc2 in position 90372: ordinal not in range(128)

This was then fixed in the Python3 port of the XCP https://github.com/xenserver/python-libs:

The description of it's xenserver/python-libs#22 has all information for it.

The summary is: For XAPI plugins (and maybe other python that do not set a valid, working UTF-8 locale when calling it), it is not safe to ignore this warning.

If none of the files in this PR are XAPI plugins, then those files can decide to ignore this warning for themselves for their entire file, even.

The lessons learnt xenserver/python-libs#22 from are:

• When it is ensured that a valid UTF-8 locale is always set when that code runs, it would be ok, to disable that warning in that module
• or, When it is ensured that the files read by that module can never contain only non-ASCII
  characters, then it would be ok as well to ignore it for that module.
• Otherwise, it's not safe to ignore.

Also:

• As long as it is ensured that all files read in a module only containe valid UTF-8, just adding encoding="utf-8" to all open calls that open files as text (not an issue for binary mode open) is fine.

Whenever valid UTF-8 is not 100% certain:

• In this cases, it is advised to ensure that decoding errors are handled.
• The easiest case, which is to just skip the invalid UTF-8 sequences when decoding the file during reading it, is to just add 'errors="ignore".

This a safe way (that drops invalid non-UTF-8 sequences) when reading a file in Python 3.6 when it is not sure if a valid UTF-8 local is set is:

LC_ALL=C python3.6 -c 'open("/usr/share/hwdata/pci.ids", encoding="utf-8", errors="ignore").read()'

OTOH, when the data should be preserved as-is:

• In these cases, it is best to open the file in binary mode and then convert the code to work with bytes instead of str type for the data from the file.

But, to disable this warning globally like this PR likes to do it, is not safe.

[bernhardkaindl]

"unspecified-encoding" is not safe to ignore blindly for all files.

The reason is that files can contain UTF-8 characters or even non UTF-8 bytes > ord(127), that are not valid UTF-8 and this caused crashing the auto-cert-kit Xapi plugin last year:

LC_ALL=C python3.6 -c 'open("/usr/share/hwdata/pci.ids").read()'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/lib64/python3.6/encodings/ascii.py", line 26, in decode
    return codecs.ascii_decode(input, self.errors)[0]
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc2 in position 90372: ordinal not in range(128)

See my review comment #5424 (review) for the further follow-up on this discussion.

[bernhardkaindl]

Suggested change

@nottest

I think when renaming the prefix of the affected functions not be test_ but verify_, the @nottest decorator can be fully removed from all utility functions in this test, pylint disable is then not needed and, the decorator itself can be removed as well.

[bernhardkaindl]

Since this is only for the feature branch, I don't want to block the collaboration on the feature branch, but a few of the changes in this PR should be improved.

I noted them with the evidence from xenserver/python-libs#22 that it is not safe to ignore "unspecified-encoding" in globally like this PR does.

Generic remarks would be:
It is not good practise to tie global changes with the port of a specific project into one PR.

Nonetheless it is good to see that static analysis typing checks are now done and not all the warning as are disabled. However, in the future, more could be done to:

1. Fix the cause of the warning when it is actually even easier and safer to do
2. Consider that the maximum number of commits in a simple PR like this should e for good review. In its current state, it has 14 commits and 8 files which appears to be to many in a single PR.
   • Such PRs may be better split into preparatory PRs that lay the groundwork and ask questions on which warnings should be globally disabled, all at once.

[bernhardkaindl]

This is inside an if msg: so we don't actually know from the code if this assertIn() really runs when it should.

If the green bar Codecov I see in the review is not due to a glitch, I see that this line in the test is executed (which is why it is a good idea to have code coverage for tests).

In 57fffaa#r1497078077 @gangj wrote:

I think it would be better to resolve this pylint warning.

Maybe this is a language barrier issue, but "resolve" means to fix, and this warning is now disabled, not fixed.

It's not obvious to me that is going on as wrong-arg-types would mean that the args don't have the same type, so something should be better explained ere.
The comment says that cm.exception.code is an is int type whose format looks like, but that is likely just very confusing language is normally int would refer to an integer. Instead, it seems like this wants to say that exception code is really the string that is quoted afterwards.

But as can be seen also, this is not actually a test function but a helper function, that is called by multiple tests, so the code coverage just says that one test covers this line, not all.

That means to clear up this confusion, it would be great to split move the code of this test function to the locations where it was called, and then it will be actually shown where msg is checked correctly.

As it stands, an if msg in a test function called by other functions is not a good idea, as it does not tell which tests check the msg. It's better to move the assertIn() to those locations that passed msg to the function, and then the test will be clearer.

There are some things that should not be done in a test, like looping over a list whose contents are not known. Such test code can result in no execution of the checks in the loop. Such guidelines should be adopted and implemented here.

[acefei]

Since this is only for the feature branch, I don't want to block the collaboration on the feature branch, but a few of the changes in this PR should be improved.

I noted them with the evidence from xenserver/python-libs#22 that it is not safe to ignore "unspecified-encoding" in globally like this PR does.

Generic remarks would be: It is not good practise to tie global changes with the port of a specific project into one PR.

Nonetheless it is good to see that static analysis typing checks are now done and not all the warning as are disabled. However, in the future, more could be done to:

1. Fix the cause of the warning when it is actually even easier and safer to do

2. Consider that the maximum number of commits in a simple PR like this should e for good review. In its current state, it has 14 commits and 8 files which appears to be to many in a single PR.

   • Such PRs may be better split into preparatory PRs that lay the groundwork and ask questions on which warnings should be globally disabled, all at once.

Thanks for your comment, I will fix the request changes in the separated PR for good review.

[github-actions]

pytype_reporter extracted 38 problem reports from pytype output

.

You can check the results of the job here