Hi All, I created this draft PR: https://github.com/jirihnidek/dmidecode/pull/3
* This new feature branch was created from synced upstream master branch * It contains a patch: https://lists.nongnu.org/archive/html/dmidecode-devel/2020-04/msg00004.html * It hopefully contains all suggestions. * The support for JSON is optional. You have to enable support of json-c using e.g.: CFLAGS="-DWITH_JSON_C" LDFLAGS="-ljson-c" make * The output contains only a basic list of headers of each smbios structure ATM * It uses a global structure approach. It works so far so good * It seems that implementing pr_attr() pr_subattr() pr_list_start() pr_list_item() pr_list_end() for JSON output will be doable using a global structure approach. Jiri On Thu, Nov 28, 2024 at 12:52 PM Jiri Hnidek <jhni...@redhat.com> wrote: > 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 <jdelv...@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 >
