Re: dmidecode --json

[Jiri Hnidek]

Hi,
Thanks for all your comments. I will create another feature branch and I
will
try to implement all suggestions you had.

It seems that your patch could help:

https://lists.nongnu.org/archive/html/dmidecode-devel/2020-04/msg00004.html

I will try to use it, but it seems that I will have to apply it manually.


On Tue, Nov 26, 2024 at 9:22 PM Jean Delvare <jdelvare@suse.de> wrote:

> 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.
>
>
I will try to use the proposed global structure. It is less flexible in
some ways,
and it will add other constraints, but you are right, The final change
should
be smaller.


> [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.
>
>
It makes sense. I will try to focus on dmidecode this afternoon and then I
will let you know.


> Thanks a lot for your work and patience.
>
> --
> Jean Delvare
> SUSE L3 Support
>
>
Thanks again for your feedback

Jiri