-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
eliminate dmidecode in component.py because it requires root privileges #17509
base: master
Are you sure you want to change the base?
Conversation
@jeff-yin flagging this one too for review by someone at Dell. |
@justindthomas please assign @arunlk-dell and @vpsubramaniam as reviewers too, thanks! |
@@ -16,7 +16,7 @@ | |||
except ImportError as e: | |||
raise ImportError(str(e) + "- required module not found") | |||
def get_bios_version(): | |||
return subprocess.check_output(['dmidecode', '-s', 'system-version']).strip().decode() | |||
return subprocess.check_output(['cat', '/sys/class/dmi/id/bios_version']).strip().decode() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vpsubramaniam can you check this change and see whether it is Kosher to get the bios version this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, here's what those commands look like on my N3248TE-ON.
Last login: Thu Dec 14 23:39:12 2023 from 10.20.37.104
jdt@sonic:~$ sudo dmidecode -s system-version
3.45.0.9-4
jdt@sonic:~$ cat /sys/class/dmi/id/bios_version
3.45.0.9-4
jdt@sonic:~$ show ver
SONiC Software Version: SONiC.jdt.0-865e34c64
SONiC OS Version: 12
Distribution: Debian 12.4
Kernel: 6.1.0-11-2-amd64
Build commit: 865e34c64
Build date: Mon Dec 11 10:57:26 UTC 2023
Built by: justin@sonic-build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vpsubramaniam can you check this change and see whether it is Kosher to get the bios version this way?
Yes Jeff, for getting the BIOS version we can use this file as well.
Is there a meeting I need to attend or something to get things like this merged? This seems like a pretty trivial change, but has a significantly positive usability impact (i.e., operations that shouldn't require privileges no longer error out). I have 4 PRs that have been sitting for months with some reviews, but apparently not enough. How do I move these forward? |
@zhangyanzhao @lguohan -- these changes have been approved by Dell staff. Can someone merge? |
@yxieca can you please help to check this PR to see if we can merge it? Thanks. |
/Semgrep |
Why I did it
As described in #16878, every
show
command on the N3248TE-ON results in an error aboutdmidecode
not being found. This is because that command is called in theget_bios_version
function ofcomponent.py
, which is used in a lot of places (many which should not require privileged access).The error message prior to this change:
Work item tracking
How I did it
On this platform, at least, the BIOS version can be obtained from
/sys/class/dmi/id/bios_version
without requiringroot
privileges. I adjustedcomponent.py
to use that instead ofdmidecode
.How to verify it
With that change in place, you can see that commands no longer complain about
dmidecode
:Tested branch (Please provide the tested image version)
master
Description for the changelog
Removed call to
dmidecode
for every component command on the N3248TE-ON platform.