Re: dmidecode --json

[Jiri Hnidek]

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 <jhnidek@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 <jhnidek@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 <jhnidek@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 <jdelvare@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
>>>
>>