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
