Add support for reading from a raw fru dump #28

matsievskiysv · 2022-11-15T07:37:11Z

This is a squashed and rebased PR #9.
It works for the most part, except for custom fields, time and ~~multi-records~~.

Source file:

{
	/* The internal use area, if specified, must list all the data as hex string,
	   the length is calculated automatically */
	"internal" : "010203040A0B0C0D",
	"chassis" : {
		"type": 10,
		"pn" : "CHAS-C00L-12",
		/* Fields may be given explicit types by setting to object with keys `type` and `data`*/
		/* Supported types are: "bcdplus", "6bitascii" and "text" */
		"serial": {
		    "type": "bcdplus",
		    "data": "45678"
		},
		"custom" : [
			{ "data" : "Auto-typed text custom field" },
			{ "type" : "binary", "data": "B14A87" },
			/* For explicit text types range is not checked, so be careful */
			{ "type" : "bcdplus", "data": "1234" },
			{ "type" : "6bitascii", "data": "1234" },
			{ "type" : "text", "data": "1234" }
		]
	},
	"board" : {
		/* The date, if not specified, will be taken automatically from
		 * the current system time. You may specify the `-u` option to
		 * `frugen` in order to leave the date 'Unspecified' */
		/* "date" : "1/10/2016 3:00:45",*/
		"mfg" : "Biggest International Corp.",
		"pname" : "Some Cool Product",
		"serial" : "123456",
		"pn" : "BRD-PN-345",
		"file" : "example1.json",
		"custom" : [
			{ "type" : "binary", "data" : "0123DEADBABE" },
			{ "type" : "auto",   "data" : "This is a text custom field" },
			{ "type" : "auto",   "data" : "This is test2" }
		]
	},
	"product" : {
		"lang": 1,
		"mfg" : "Super OEM Company",
		"pn" : "PRD-PN-1234",
		"pname" : "Label-engineered Super Product",
		"serial" : "OEM12345",
		"atag" : "Accounting Dept.",
		"ver" : "v1.1",
		"file" : "example2.json",
		"custom" : [
			{ "type" : "auto",   "data" : "Product Custom 1" },
			{ "type" : "auto",   "data" : "PRDCSTM" },
			{ "type" : "auto",   "data" : "PRDCSTM2" },
			{ "type" : "binary", "data" : "C001BEEF" }
		]
	},
	"multirecord" : [
		{ "type" : "management", "subtype" : "uuid", "uuid" : "9bd70799-ccf0-4915-a7f9-7ce7d64385cf" }
	]
}

Is converted using commands:

./build/frugen-static -j -z example.json example.bin
./build/frugen-static -r -z example.bin

Output:

Chassis
        type: 10
        pn(sixbitascii): CHAS-C00L-12
        serial(bcdplus): 45678
        custom: Auto-typed text custom field
        custom:  J
        custom: 4
        custom:  4Q
        custom: 4
Product
        lang: 25
        mfg(text): Super OEM Company
        pname(text): Label-engineered Super Product
        serial(sixbitascii): OEM12345
        pn(sixbitascii): PRD-PN-1234
        ver(text): v1.1
        atag(text): Accounting Dept.
        file(text): example2.json
        custom: Product Custom 1
        custom:  L 3
        custom:  L 3 J
        custom:
Board
        lang: 25
        time: 13/55/2067 08:07:24
        mfg(text): Biggest International Corp.
        pname(text): Some Cool Product
        serial(bcdplus): 123456
        pn(sixbitascii): BRD-PN-345
        file(text): example1.json
        custom: #ޭ
        custom: This is a text custom field
        custom: This is test2
Multirecord

matsievskiysv · 2022-11-15T11:31:49Z

Fixed custom fields:

Chassis
	type: 10
	pn(sixbitascii): CHAS-C00L-12
	serial(bcdplus): 45678
	custom(text): Auto-typed text custom field
	custom(binary): b14a87
	custom(bcdplus): 1234
	custom(sixbitascii): 1234
	custom(text): 1234
Product
	lang: 25
	mfg(text): Super OEM Company
	pname(text): Label-engineered Super Product
	serial(sixbitascii): OEM12345
	pn(sixbitascii): PRD-PN-1234
	ver(text): v1.1
	atag(text): Accounting Dept.
	file(text): example2.json
	custom(text): Product Custom 1
	custom(sixbitascii): PRDCSTM
	custom(sixbitascii): PRDCSTM2
	custom(binary): c001beef
Board
	lang: 25
	time: 13/29/2067 10:07:24
	mfg(text): Biggest International Corp.
	pname(text): Some Cool Product
	serial(bcdplus): 123456
	pn(sixbitascii): BRD-PN-345
	file(text): example1.json
	custom(binary): 0123deadbabe
	custom(text): This is a text custom field
	custom(text): This is test2
Multirecord

matsievskiysv · 2022-11-15T12:38:03Z

Fixed time decoding:

time: 15/11/2022 09:35:33

matsievskiysv · 2022-11-15T12:40:09Z

I think, the only thing left to do for now is JSON dump.

matsievskiysv · 2022-11-16T07:23:16Z

I think, it's ready for review.

Basic command usage

Generate FRU dump from example.json:

fugen-static -j -z example.json example.bin

Parse FRU dump to jq or example2.json

frugen-static -r -z example.bin > example2.json
frugen-static -r -z example.bin | jq

...
Profit!

More involved example: parse FRU dump, replace one of the fields and save to JSON file

frugen-static -r -z example.bin | jq '.board.file.data |= "otherexample.json"' > otherexample.json

Motivation

The motivation for this is to use ipmitool to dump the current fru contents and then modify/add fields before writing it back. (From #9)

As may be seen from the examples above, dumping parsed data to stdout is more flexible than just using FRU dump in new dump generation:

data can be examined
data can be easily modified

Further work

Internal area and multi-record parsers are not implemented yet.

matsievskiysv · 2022-11-18T10:32:37Z

It's not very convenient to use functions defined in fru_reader.h as they work with file descriptor. It would be more flexible to expose functions that operate on a buffer (and maybe a set of helper functions for file descriptor and/or stream access).

matsievskiysv · 2022-11-21T09:30:49Z

Refactored decoder functions a bit for better library API

AlexanderAmelkin

First of all, thank you for the effort! Then, there are some things I'd like to improve.

fru.c

fru.h

frugen.c

AlexanderAmelkin · 2022-11-21T13:21:57Z

frugen.c

@@ -674,6 +748,55 @@ int main(int argc, char *argv[])
 					fatal("JSON support was disabled at compile time");
 #endif
 				}
+				else if (use_binary) {
+					int fd = open(optarg, O_RDONLY);


Looks like this and the above json block need to be extracted into separate functions like load_binary() and load_json() respectively. With a separate commit, as a refactoring.

I think that these functions belong to fru library.

fair enough

frugen.c

matsievskiysv · 2022-11-22T17:07:59Z

I've pushed edits, resolving most of the notes. Later, I will squash commits and split changed into separate commits, but first, library API and CLI arguments need to be thought over.

fru.c

AlexanderAmelkin · 2022-12-05T13:14:13Z

fru.c

+		uint32_t val;
+		uint8_t arr[4];
+	} min_since_1996_big_endian;
+	min_since_1996_big_endian.val = 0;


could as well add = { 0 }; at the end of the declaration above.
I don't insist though.

you don't need to assign 0 to val after min_since_1996_big_endian = {0}, also please make it { 0 }.

frugen.c

matsievskiysv · 2023-01-09T07:29:17Z

frugen is coupled too tight to the fru library, moving parsing functions from frugen to fru requires a lot of changes. This adds to an already big patch. @AlexanderAmelkin, maybe you could create a new branch for the reader feature, so these things could be added one PR at a time?

fru.c

AlexanderAmelkin · 2023-01-11T12:56:49Z

fru.c

+		uint32_t val;
+		uint8_t arr[4];
+	} min_since_1996_big_endian;
+	min_since_1996_big_endian.val = 0;


you don't need to assign 0 to val after min_since_1996_big_endian = {0}, also please make it { 0 }.

AlexanderAmelkin · 2023-01-11T12:58:51Z

fru.c

+
+	field = (fru_field_t*)data;
+	if (!fru_decode_data(field, &product_out->mfg,
+                         sizeof(product_out->mfg.val)))


What is this second line aligned to? I don't see it.
If this is not an alignment, then please use tabs.
When mixing indentation with alignment (like here), please use tabs for indentation and spaces for alignment. That way it wont visually break in editors that use a different tab size:

if (!fru_decode_data(field, &product_out->mfg, sizeof(product_out->mfg.val)))

This applies to the below cases as well.

Sorry, what you did is not exactly what I meant by 'tabs for indentation, spaces for alignment'.
If you change tab size, this is still going to break.
This is 1 level on indent, hence it must be just 1 tab. The rest must be spaces.

There are lots of other places like this too.

fru.c

AlexanderAmelkin · 2023-01-11T13:14:33Z

frugen.c

+					}
+
+					free(buffer);
+				}
 				else {
 					fatal("The requested input file format is not supported");


Now it looks more like "Please specify the input file format"

You didn't actually resolve this, did you?

Sorry, this edit got lost during the commit split

frugen.c

matsievskiysv · 2023-01-12T12:01:32Z

I've updated the code according to your comments and split it into separate commits.
The following remains for the following PRs:

Extract JSON and raw reader logic into separate function. This requires a lot of changes because of statically allocated data in the main function. It would be easier to navigate in diff in separate PR.
Change CLI behaviour (from the table in previous messages). It makes sense to do this together with the previous task.

I propose to leave these improvements for the separate PR.

AlexanderAmelkin · 2023-01-12T13:18:26Z

frugen.c

+					}
+
+					free(buffer);
+				}
 				else {
 					fatal("The requested input file format is not supported");


You didn't actually resolve this, did you?

AlexanderAmelkin · 2023-01-12T13:20:28Z

I've updated the code according to your comments and split it into separate commits. The following remains for the following PRs:

Extract JSON and raw reader logic into separate function. This requires a lot of changes because of statically allocated data in the main function. It would be easier to navigate in diff in separate PR.

Change CLI behaviour (from the table in previous messages). It makes sense to do this together with the previous task.

I propose to leave these improvements for the separate PR.

I agree that those changes can go in separate PRs.

matsievskiysv · 2023-01-13T04:28:47Z

Since there're a lot of indentation problems, maybe it would be better to add a formatting rules for clang-format or indent.
This is a clang-format file I use for my projects and It's close to the format of the project (drop the .txt extension):
.clang-format.txt

AlexanderAmelkin · 2023-01-17T10:52:09Z

Since there're a lot of indentation problems, maybe it would be better to add a formatting rules for clang-format or indent

As far as I'm aware, clang-format doesn't support the concept of 'tabs for indent, spaces for alignment', and I wouldn't like to use any less. If you know how to teach it that, please tell.

I will look into indent's capabilities.

matsievskiysv · 2023-01-17T14:17:33Z

clang-format doesn't support the concept of 'tabs for indent, spaces for alignment'

I think it is using it: fru.c.txt

matsievskiysv force-pushed the reader branch from 2496dce to 5395299 Compare November 15, 2022 11:30

matsievskiysv force-pushed the reader branch 2 times, most recently from 6101093 to eb44561 Compare November 15, 2022 12:36

matsievskiysv force-pushed the reader branch from eb44561 to edbda15 Compare November 16, 2022 06:32

matsievskiysv marked this pull request as ready for review November 16, 2022 06:37

matsievskiysv force-pushed the reader branch from 7be4aac to ba93e3c Compare November 21, 2022 09:29

matsievskiysv force-pushed the reader branch 2 times, most recently from fc5dddf to 8a11d16 Compare November 21, 2022 13:06

AlexanderAmelkin suggested changes Nov 21, 2022

View reviewed changes

matsievskiysv force-pushed the reader branch from 5204a07 to 8a1691d Compare November 29, 2022 14:58

AlexanderAmelkin suggested changes Dec 19, 2022

View reviewed changes

matsievskiysv force-pushed the reader branch from 8a1691d to 457bffe Compare January 9, 2023 04:55

AlexanderAmelkin suggested changes Jan 11, 2023

View reviewed changes

matsievskiysv force-pushed the reader branch from 28aad59 to b4428e6 Compare January 12, 2023 11:49

AlexanderAmelkin suggested changes Jan 12, 2023

View reviewed changes

Matsievskiy S.V added 2 commits February 6, 2023 07:45

Fix wrong arguments to fatal macro

450c26b

Fix field comment line

ad885f0

Add reader

f57e093

matsievskiysv force-pushed the reader branch 3 times, most recently from cf7d75d to 9abaf70 Compare February 6, 2023 05:28

Reindent files

9abaf70

matsievskiysv requested a review from AlexanderAmelkin February 6, 2023 05:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for reading from a raw fru dump #28

Add support for reading from a raw fru dump #28

matsievskiysv commented Nov 15, 2022 •

edited

Loading

matsievskiysv commented Nov 15, 2022

matsievskiysv commented Nov 15, 2022

matsievskiysv commented Nov 15, 2022 •

edited

Loading

matsievskiysv commented Nov 16, 2022

matsievskiysv commented Nov 18, 2022

matsievskiysv commented Nov 21, 2022

AlexanderAmelkin left a comment

AlexanderAmelkin Nov 21, 2022

matsievskiysv Nov 22, 2022

AlexanderAmelkin Dec 1, 2022

matsievskiysv commented Nov 22, 2022

AlexanderAmelkin Dec 5, 2022

AlexanderAmelkin Jan 11, 2023

matsievskiysv commented Jan 9, 2023

AlexanderAmelkin Jan 11, 2023

AlexanderAmelkin Jan 11, 2023

AlexanderAmelkin Jan 12, 2023

AlexanderAmelkin Jan 11, 2023

AlexanderAmelkin Jan 12, 2023

matsievskiysv Jan 13, 2023

matsievskiysv commented Jan 12, 2023

AlexanderAmelkin Jan 12, 2023

AlexanderAmelkin commented Jan 12, 2023

matsievskiysv commented Jan 13, 2023

AlexanderAmelkin commented Jan 17, 2023

matsievskiysv commented Jan 17, 2023

Add support for reading from a raw fru dump #28

Are you sure you want to change the base?

Add support for reading from a raw fru dump #28

Conversation

matsievskiysv commented Nov 15, 2022 • edited Loading

matsievskiysv commented Nov 15, 2022

matsievskiysv commented Nov 15, 2022

matsievskiysv commented Nov 15, 2022 • edited Loading

matsievskiysv commented Nov 16, 2022

Basic command usage

Motivation

Further work

matsievskiysv commented Nov 18, 2022

matsievskiysv commented Nov 21, 2022

AlexanderAmelkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsievskiysv commented Nov 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsievskiysv commented Jan 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsievskiysv commented Jan 12, 2023

Choose a reason for hiding this comment

AlexanderAmelkin commented Jan 12, 2023

matsievskiysv commented Jan 13, 2023

AlexanderAmelkin commented Jan 17, 2023

matsievskiysv commented Jan 17, 2023

matsievskiysv commented Nov 15, 2022 •

edited

Loading

matsievskiysv commented Nov 15, 2022 •

edited

Loading