Hi Jiri, [It doesn't look like this message was ever distributed last December, even though it is in my Sent box. At least I can't find it in the list archive, and I did not receive it from the list either. Therefore I am sending it again, sorry if this is a duplicate for you.]
Sorry for the slow review. On Fri, 2024-11-29 at 23:56 +0100, Jiri Hnidek wrote: > > 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. Overall I like it really much, I think this is going in the right direction. Here are a few comments: [commit e5b0f07c7409 ("Added initial support for output DMI data to JSON")] I wouldn't introduce a typedef for struct json_dmi_output as this structure is only used once. I don't understand the test for (ofmt != NULL) in pr_finish(). I can't see how ofmt could not be set, if that was really possible then I believe things would break much earlier. I also don't understand why you explicitly initialized ofmt to NULL, this shouldn't be needed. json_dmi_out is only used in dmioutput.c so it can be declared static. There's no need to explicitly initialize all fields to NULL, a static global variable is always initialized to zero by the compiler. BTW I think I would have named that variable json_context, this more clearly describes the purpose of the structure. Note that you can name the struct the same as the variable, this is a common practice when single instance of the structure exists. json_dmi_out.array is a poor name, it tells the type but not the purpose. What about "records"? Not sure why you test json_dmi_out.root && json_dmi_out.array in pr_finish_json(), they are initialized unconditionally so there's no way they wouldn't be set. Likewise, in pr_handle_name_json() I wouldn't bother with the error path (json_dmi_out.item == NULL) as this can only happen if there's a bug in the code. A badly formatted input (DMI table) wouldn't trigger it as long as the code is right. If you really want to keep it then I think it should be written differently because most of the code is duplicated from the non-error path. I see that you are using vasprintf(). No problem with that but you need to #define _GNU_SOURCE before including <stdio.h> otherwise the function declaration may be missing and the compiler will complain. I'll review the follow-up commits after my vacation as my time permits. > > 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. Not sure about that either, but it can be added later, no worry. -- Jean Delvare SUSE L3 Support
