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

Change IpmiBaseLibNull to BASE library type #160

Merged
merged 3 commits into from
Sep 20, 2023

Conversation

cfernald
Copy link
Contributor

Description

The IpmiBaseLibNull implementation currently restricts its library types and consumes libraries that it does not use. This change cleans up and generalizes the NULL imlementation.

  • Impacts functionality?
  • Impacts security?
  • Breaking change?
  • Includes tests?
  • Includes documentation?

How This Was Tested

Local build

Integration Instructions

N/A

Copy link
Member

@makubacki makubacki left a comment

Choose a reason for hiding this comment

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

Main comment that I think should be resolved is adding the EFIAPI modifier since this is a public API it and should be a part of the cleanup.

@apop5
Copy link
Contributor

apop5 commented Sep 19, 2023

https://github.com/tianocore/edk2/commits/master/MdeModulePkg/Library/BaseIpmiLibNull

MdeModulePkg includes a BaseIpmiLibNull.

Our IpmiBaseLib is out of sync with the edk2 version. Would we benefit from removing our Base version and consuming the edk2 (and moving the GetBmcStatus into another Library instance contained in mu_feature_ipmi?

At some point we will want to reconcile the two implementations, right? It may not matter for this PR, but it could help us reduce the debt we are accruing.

@cfernald
Copy link
Contributor Author

https://github.com/tianocore/edk2/commits/master/MdeModulePkg/Library/BaseIpmiLibNull

MdeModulePkg includes a BaseIpmiLibNull.

Our IpmiBaseLib is out of sync with the edk2 version. Would we benefit from removing our Base version and consuming the edk2 (and moving the GetBmcStatus into another Library instance contained in mu_feature_ipmi?

At some point we will want to reconcile the two implementations, right? It may not matter for this PR, but it could help us reduce the debt we are accruing.

I think this is fair. I'll co ahead and complete this PR and plan to make this swap. Generally, I dont think anyone is calling GetBmcStatus through the base lib and it's a rare enough operation I'm fine just not having it in the abstractions.

@cfernald cfernald enabled auto-merge (squash) September 19, 2023 23:33
@cfernald cfernald merged commit 0f3834b into microsoft:main Sep 20, 2023
13 checks passed
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.

4 participants