Re: dmidecode --json

[Jiri Hnidek]

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
>