Hi, I made some changes in PR #3 (mostly related to code style). I added more error messages. I also discovered a bug in dmidecode.c and fixed it in a separate commit.
Open questions: 1) Should be support for output to JSON format (requiting json-c) enabled or disabled by default? 2) When support for JSON output format is disabled, should the -j and --json CLI option be present? If yes, should we print some error message, when -j or --json is used. Something like: "dmidecode build without JSON support" If no, then I think that It could be confusing to have same version of dmidecode with different CLI option set on different platforms and distributions. I'm sorry that I wasn't more active last week, but I was busy with other projects. Jiri On Fri, Nov 29, 2024 at 11:56 PM Jiri Hnidek <jhni...@redhat.com> wrote: > Hi, > I finished implementation of > pr_attr() pr_subattr() pr_list_start() pr_list_item() pr_list_end(). > I updated the documentation, bash completion script. The dmidecode should > work as expected. > The new PR: https://github.com/jirihnidek/dmidecode/pull/3 is ready for > review. > > The information about smbios version is missing as well as information > about the version of dmidecode, > because I'm not sure how to implement it. > > Jiri > > On Fri, Nov 29, 2024 at 6:54 PM Jiri Hnidek <jhni...@redhat.com> wrote: > >> 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 >>> >>
