Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CA-392459: Avoid opening /dev/mem when calling biosdevname #108

Merged

Conversation

rosslagerwall
Copy link
Collaborator

biosdevname opens /dev/mem to read the $PIR PCI interrupt routing table. This fails with Secure Boot enabled and causes a warning in the kernel. log. Since the $PIR PCI interrupt routing table is used for routing PCI interrupts to ISA IRQs, it is not useful on any modern system so pass "-x" to biosdevname to avoid this behaviour.

rosslagerwall and others added 2 commits May 8, 2024 07:29
biosdevname opens /dev/mem to read the $PIR PCI interrupt routing table.
This fails with Secure Boot enabled and causes a warning in the kernel.
log. Since the $PIR PCI interrupt routing table is used for routing PCI
interrupts to ISA IRQs, it is not useful on any modern system so pass
"-x" to biosdevname to avoid this behaviour.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Signed-off-by: Bernhard Kaindl <bernhard.kaindl@cloud.com>
@coveralls
Copy link

coveralls commented May 8, 2024

Pull Request Test Coverage Report for Build 9001178197

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 5 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.7%) to 93.035%

Files with Coverage Reduction New Missed Lines %
tests/integration/utils.py 5 87.69%
Totals Coverage Status
Change from base Build 8552247539: 0.7%
Covered Lines: 2725
Relevant Lines: 2929

💛 - Coveralls

liulinC
liulinC previously approved these changes May 8, 2024
@liulinC
Copy link
Contributor

liulinC commented May 8, 2024

A not related comment for @bernhardkaindl
Given we are python3 ready for this repo,
should we remove the python2 compatibility support (and also the python2 scans) to make code simple.

andyhhp
andyhhp previously approved these changes May 8, 2024
Copy link
Collaborator

@bernhardkaindl bernhardkaindl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the coverage upload to coveralls.io in GitHub CI.

In order to apply this fix, I had to rebase it onto the latest master branch.

To answer @liulinC's question:

A not related comment for @bernhardkaindl
Given we are python3 ready for this repo,
should we remove the python2 compatibility support (and also the python2 scans) to make code simple.

Python2 is not ready to be removed.

  1. There are still 7 open tasks below CP-48020 (remaining Py2->Py3 work) that are not yet done.
    • Without these done, status-report is not validated to be ready for production use on Python3.
  2. UPD-990 for the Yangtze release contains CP-41238 ([XSI-1344] Bugtool should contain up-to-date RRDs)
    • The fix for CP-41238/XSI-1344 depends on other commits on master.
    • Instead of backporting these large changes (potentially introducing errors) and having to maintain that older branch for the Yangtze release, it will be less work to use master for UPD-990 for fixing XSI-1344.
    • Because we use master for XS8 as well, master is in constant production use with Python2 in:
      • XenRT
      • Internal diagnostics
      • Customer support.
    • Therefore, we know that master is already in production use since quite some time with Python2, and we can safely use master for the Yangtze hotfix UPD-990 too.
    • This means, due to the constant testing in XenRT and Customer support, we know that we can safely deploy master for Yangtze hotfixes like UPD-990.

This benefit alone is quite big. There are only 5 (yes, just five) conditions in the status-report code where there is a tiny special case for Python2/Python3. Compared to the total size of over 2390 lines of the program, this is totally negligible.

That means it does not increase or decrease the complexity by any perceivable amount.

Removing Python2 support would not make this code simple by any means, the complexity is not in these 5 if conditions but in very very very large and complex functions.

I am not aware of any other xenserver or xapi script except of the host-installer's install.py go() function (it has a CC of 60) that comes close in Cyclomatic Complexity (CC) to the main() function of xen-bugtool with a CC of 84, and Python2/3 does not change that at all:

pip install radon
# Clone python-libs, and host-installer copy perfmon from xen-api, then run:
radon cc xen-bugtool host-installer/ perfmon xcp --total-average -nd --md
Filename Name Type Start:End Line Complexity Classification
xen-bugtool main F 777:1359 84 F
xen-bugtool load_plugins F 1761:1827 27 D
xen-bugtool collect_data F 701:758 21 D
host-installer/install.py go F 89:325 60 F
host-installer/backend.py performInstallation F 293:446 31 E
host-installer/backend.py partitionTargetDisk F 525:587 21 D
host-installer/disktools.py DOSPartitionTool.writeThisPartitionTable M 839:912 23 D
host-installer/restore.py restoreFromBackup F 17:177 33 E
host-installer/product.py ExistingInstallation._readSettings M 101:412 75 F
host-installer/diskutil.py probeDisk F 467:530 21 D
host-installer/init main F 92:247 35 E
host-installer/init configureNetworking F 28:85 24 D
host-installer/tui/repo.py confirm_load_repo F 207:283 21 D
host-installer/tui/network.py get_iface_configuration F 15:134 29 D
host-installer/tui/installer/screens.py get_name_service_configuration F 795:962 28 D
perfmon main F 1307:1522 38 E
perfmon VMMonitor.get_default_variable_config M 858:917 23 D
xcp/cpiofile.py CpioFile.open M 1003:1083 22 D
xcp/bootloader.py Bootloader.readExtLinux M 110:194 32 E
xcp/bootloader.py Bootloader.readGrub M 197:301 28 D
xcp/bootloader.py Bootloader.readGrub2 M 304:463 26 D
xcp/bootloader.py Bootloader.writeGrub2 M 557:619 23 D
xcp/net/ifrename/dynamic.py DynamicRules.generate M 147:227 23 D
xcp/net/ifrename/logic.py rename_logic F 125:366 41 F
xcp/net/ifrename/logic.py rename F 368:498 35 E
xcp/net/ifrename/static.py StaticRules.load_and_parse M 103:210 25 D
xcp/net/ifrename/static.py StaticRules.generate M 212:292 23 D

See the FAQ at https://github.com/xenserver/status-report/blob/8ccbb86e1e59a09ad060c269bcf40d402fbcbe23/README.md#frequently-asked-questions for further information.

@bernhardkaindl bernhardkaindl merged commit 0b7c72c into xenserver:master May 9, 2024
6 checks passed
@rosslagerwall rosslagerwall deleted the private/rossla/CA-392459 branch May 10, 2024 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants