Hi Jiri,
On Mon, 3 Jul 2023 18:18:44 +0200, Jiri Hnidek wrote:
> I created another PR https://github.com/jirihnidek/dmidecode/pull/2, where
> "dmidecode --json" uses json-c. It is still draft. I need to do some
> refactoring and testing. Do not expect that it will work seamlessly with
> all combinations of CLI options.
I started reviewing the commits (although I must admit I find it easier
to do when patches are being posted to the mailing list). I won't
comment on the indentation issues as Erwan already mentioned that. Also
pay attention to the placement of curly braces.
[PATCH 1/25] ff8a8f5d4c92b0c20747f68f76ed3f5b7dae27b9 "Added --json CLI option.
It does nothing ATM."
Looks good to me, although it will probably look a bit different if we
first merge my last commit to properly support alternative output
formats. Instead of setting a flag (opt.flags |= FLAG_JSON), option
--json would change the output format (set_output_format(OFMT_JSON)).
[PATCH 2/25] f49f85f9adbd3b9e4411b661f9eb4494ec8242af "Modify makefile to link
dmidecode with json-c."
Looks good, but as I said before, I would like this to be controllable
by a makefile variable, so that the dependency to json-c is optional.
[PATCH 3/25] f518d883e4168fc29f6d6dac6b2e27913692466f "Extracted all printf()
from dmidecde to pr_printf()."
The only remaining direct printf() calls should only be reached when
option --string is used, and that mode should be mutually exclusive
with --json. As a matter of fact, you do make them mutually exclusive
in a latter commit ("Make more CLI options mutual exlusive (--json and
others)"). So I think this change is not needed.
[PATCH 4/25] 060dc506d7fe1eefdb8ef54596cc2c22f005bf90 "When --json is used,
then nothing should be printed to stdout."
I believe this becomes obsolete once we apply my pending patch
https://lists.nongnu.org/archive/html/dmidecode-devel/2020-04/msg00004.html
Simply changing the output "driver" to the json one should be enough to
prevent anything from being written directly to stdout.
This commit also included unrelated changes which get essentially
reverted in the following commit.
[PATCH 5/25] 9f9c6cefce68581b0fb42931baad9ee1fd0f08d1 "dmidecode --json prints
array of headers ATM"
Name "data" is pretty vague, maybe something more specific like
"dmi_data"? Or, if we stick to the terms used in the specification,
"smbios_structures"?
I see that there is a lot of json-specific code being added directly to
the dmi_table_decode function in dmidecode.c. I must say this is
disappointing, my hope was really that the code in dmi_table_decode
would stay agnostic to the output format, and all format-specific
implementation details would be handled by the output "drivers" in
dmioutput.c. This would be especially valuable to be able to make JSON
output support optional.
I understand that you need hooks which aren't there yet because the
plain text output didn't need them, and you need to carry around some
context which the plain text output also didn't need. I think we should
add these hooks to struct ofmt (for example you obviously need
something like a "flush" callback to write the in-memory JSON object to
stdout).
For the context, I can think of two ways to deal with it. Option 1
would be that all print functions take a context as a parameter and at
least some of them return a context. It's up to output drivers if they
make use of it or not. Option 2 would be to store all context
information (which SMBIOS structure I'm in, which list I'm in if
any...) as a global structure, and all print callbacks can set and use
it.
I see you went with option 1 in latter commits, which I admit is
somewhat cleaner, however as you found out, it requires modifying
hundreds of callers throughout the source code, so I must say I have a
clear preference for option 2.
[PATCH 6/25] ef441ecc0b9df38d15170c2b9597310a9cf0136d "When --json is used,
then you can see "name" in "entry" in some items."
This is a confusing commit, beginning with the description: it's not
immediately clear whether you are fixing a bug or implementing
something new. Also "some" is vague, we would need a better explanation
of which items are affected. Generally speaking, commit messages must
give all details required to understand what changes are being done and
why they were necessary.
Overall it looks like a half-cooked commit, mixing formatting changes,
changes in calling conventions and variable renaming. In my opinion, it
includes preparatory changes which should be separated, and is missing
changes which were added in subsequent commits. Commits must be unitary
and self-sufficient.
At first I was worried by the use of _GNU_SOURCE, because dmidecode is
used on BSD, Solaris, BeOS and Haiku. But apparently vasprintf is
widely available, so using this function should be OK.
I will stop my initial review here, I think it doesn't make sense to
review the rest until you provide feedback on my comments so far, and
we decide which route to go.
Thanks a lot for your work and patience.
--
Jean Delvare
SUSE L3 Support
