Re: dmidecode --json

[Jiri Hnidek]

Hi All,
I'm sorry for the late response, but I am busy at work.

Thanks for all your great comments.

Of course, I prefer the variant using json-c, because it is IMHO a more
reliable
approach.

More comments follows:

On Mon, Nov 25, 2024 at 6:41 PM Jean Delvare <jdelvare@suse.de> wrote:

> 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 didn't like the first PR too much. I personally considered this
implementation brittle and it was the reason why I closed it.
The first PR was really only proof of concept. It had many sharp
edges.



> 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.
>

I will look at this.


> * 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 personally like standing on the shoulders of giants and json-c is
small library that is widely used. As you can see here:
https://github.com/json-c/json-c
It has a lot of tests (unit tests, fuzz tests, benchmark tests).
It follows the Unix philosophy: Write programs that do one thing and
do it well.


>
> 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.
>

I see. It has its benefits to not rely on any 3rd party library. On the
other hand
It is sometimes wise to stand on the shoulders of giants. If you would
prefer
not to rely on json-c or any other library, then it is OK. It is important
to have machine readable output. Using or not using json-c is implementation
detail.

I had several reasons for closing my first PR and introduce second PR:

1) Escaping special characters was really naive:
https://github.com/jirihnidek/dmidecode/pull/1/files#diff-55c1b066d52f34a32fdbcf3c9df30cc8dd43f613d4f29132eedb5b066c3c23a4R41
2) It would be necessary to escape more special characters like json-c does:
https://github.com/json-c/json-c/blob/master/json_object.c#L186
3) When I realized that proper implementation of output to JSON
would require reimplement some parts of json-c, then I decided to
use json-c.
4) When you use json-c, then you put all data to a virtual tree and you
output everything at once. There is a lower risk that you create invalid
JSON document. In the worst case there is something that does not
belong to the output before or after a JSON document. It is easier to
figure out what is wrong.
5) If the first approach was used, then there would be risk that dmidecode
outputs something wrong inside the JSON document and it would be
harder to figure out what is wrong, and why output is not valid JSON
document.


>
> 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 do not have any personal experience with BSD nor Solaris, but it
seems that json-c is available on BSD freshport:
https://www.freshports.org/devel/json-c
It seems that json-c is also available for Solaris:
http://pkg.oracle.com/solaris/release/info/0/library/json-c@0.12,5.11-11.4.0.0.1.14.0:20180814T164750Z
There is also some fork of json-c for Solaris:
https://www.opencsw.org/packages/CSWlibfastjson4/
In theory json-c is possible to compile using many C compilers on any
UNIX-like operating system.


>
> 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.
>

Yeah, that makes sense.

Thanks again

Jiri Hnidek
Red Hat



>
> Thanks,
> --
> Jean Delvare
> SUSE L3 Support
>
>