Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[WIP] Add support for reading from a raw fru dump #9
base: master
Are you sure you want to change the base?
[WIP] Add support for reading from a raw fru dump #9
Changes from 9 commits
078ecc5
e987280
66c1062
1ad43d1
4d2aa75
f689945
2a93c93
ebf5bf3
8e38de1
632f5f3
d30800d
efa60c0
06c2eb1
6bae19c
2dd511d
f2b7f22
57a0f33
dea002c
4c8b4e4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
There is already a very similar piece of code in
fru.c
, please think about some function that would allow to get rid of repetitions.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.
Looks like you're losing the "internal use" and "multi-record" areas here. The output file won't have them even if the original raw file did. That may defeat the purpose of your change as I understand it.
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.
we don't need those right now. If it's alright, I'd like to omit support for multirec and internal in the output for now. The internal area is pretty straightforward, but the multirec area seems to require some parsing, which I'd prefer to not have to implement for this PR.
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.
That's ok because libfru initially didn't support creating those areas, but please specify in the help that users may lose their pre-existing areas when using
--raw
, and also please update the README.md with the information about the new feature and its limitations.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.
Also, that code block is large enough for a separate function.