Re: dmidecode --json

[Jean Delvare]

Hi Jiri,

On Fri, 23 Jun 2023 16:45:03 +0200, Jiri Hnidek wrote:
> Hi,
> I'm working on machine readable output of dmidecode. I added the --json CLI
> option. You can see my POC here:
> 
> https://github.com/jirihnidek/dmidecode/pull/1

I know that you have posted a second iteration of your work meanwhile,
but I still wanted to review this first iteration so I better
understand the steps you went through.

I did a quick review and this is what I found:

* First of all, I must apologize. When reviewing your work, I realized
  that my effort to abstract the output of dmidecode and make it
  possible and easy to implement alternative output format, was not
  fully committed. I proposed two possibilities for the last step, but
  never settled (I waited for feedback that never came) so never
  committed either. The two possible implementations can be seen here:
  https://lists.nongnu.org/archive/html/dmidecode-devel/2020-04/msg00001.html
  https://lists.nongnu.org/archive/html/dmidecode-devel/2020-04/msg00004.html
  I think we want to make a decision and commit that before adding JSON
  output format support, as it should make integration a lot easier and
  cleaner. Feel free to review and tell me what you think. I can send
  raw patches to you if it helps.

* In dmi_decode() case 0 (BIOS information), you fixed the position of
  pr_list_end(). This is a good catch but this bug should be fixed
  separately from the addition of JSON output format (even though I
  understand that the fix was not strictly needed before JSON as
  pr_list_end is a no-op for text output). I see that this fix was not
  included in your second iteration, which I find suspect.

* You are using the bool variable type. How portable is that?
  Remember that dmidecode must work on BSD and Solaris, not just Linux.

* Documentation update was missing but I see this was fixed in the
  second iteration, good.

> Example of output can be seen here:
> 
> https://gist.github.com/jirihnidek/02c7620a5c2cf06752ccea877b35cc84

Looks good in a browser, but when running the patched dmidecode on my
own machine, I noticed that the output was all on a single line, which
made it horrible to read in a text editor. I understand that newlines
and proper indentation are not technically needed for a JSON file, but
I consider them a must-have so that the file can be handled by a human
being if needed. Thankfully this seems to be addressed in v2.

> As you can see there are no dependencies to any other library, but some
> information is missing in the output (e.g. handle, DMI type), and there are
> other issues.

One issue I noticed is that using --json and --quiet together resulted
in an invalid JSON file. Seems fixed in v2 by making them mutually
exclusive (which I think makes sense), good.

> I would like to ask if you would be interested adding this to upstream
> version of dmidecode?

Yes!

> I would like to do proper implementation using the json-c library:
> https://github.com/json-c/json-c This is a small library that is part of
> most Linux distributions. Could adding dependency on json-c be acceptable?

Not a blocker, but I see that this option results in more intrusive
changes (although the net difference isn't that big). The first
iteration (without json-c) didn't look especially ugly to me so I would
need a better reason for adding a dependency than "proper
implementation". What actual benefit is there to using json-c?

I'll be honest with you, when I prepared the code to be able to support
alternative output formats, I really hoped that we would be able to
generate a JSON file directly without using a library, thus sticking to
the dmidecode tradition of simplicity, absence of dependency and very
low memory footprint. So my preference leans towards version 1 of your
work. Thus if you want me to review and pick version 2 instead, you'll
have to convince me with good arguments.

If we end up using the json-c library, do you know how portable it is?
Ideally I would like JSON support to also work on BSD and Solaris.

I also would like json-c support to be optional, that is, one should
still be able to build dmidecode (without JSON support, obviously) if
they somehow don't have access to json-c. No need for autodetection,
just a flag in the Makefile which can be set if needed, and proper
#ifdefs in the code to discard the new code.

Thanks,
-- 
Jean Delvare
SUSE L3 Support