Hi Jiri, On Tue, 4 Jul 2023 11:11:13 +0200, Jiri Hnidek wrote: > On Mon, Jul 3, 2023 at 6:57 PM Erwan Velu <erwanalia...@gmail.com> wrote: > > 4/ be careful of the code indent in your PR, you're breaking the coding > > style > > Oh no. I thought that my IDE is smart enough and it is able to detect the > style of indentation of imported projects and alter settings for > related projects. Never mind. I will fix it. Will it be enough to fix > it in next commit or is it necessary to fix it in all commits?
I know it is more work but I insist on having the cleanest possible history, so issues found during review, including coding style issues, should be amended in the original commit. > Open questions: > 1) What about "handle" values? Could it be a number? Or should I convert it > to string with hexadecimal representation? I lean towards hex values to be > consistent with text format and compatible with jc. Apparently original JSON does not support hexadecimal values as a number, so indeed you would need to use strings if you want to use hexadecimal values. While using (decimal) numbers would seems more natural for JSON, sticking to hexadecimal (and thus strings) would be closer to the text output of dmidecode. As the dmidecode output already includes many hexadecimal values which we definitely don't want to convert to decimal (bitfields, addresses), doing the same for handles should make things more simple and more consistent, so I recommend that you do it that way. > 2) There is nothing printed in pr_info() and pr_comment(), when --json is > used. Is it acceptable? This is not a blocker, but we may think about the best thing to do with these strings. I seem to understand that the JSON format has no provision for comments, but maybe they can be included as data in a separate structure. Some of the comment strings are not very valuable, but things like the SMBIOS version and the version of dmidecode can be useful to interpret the data afterwards. Note that pr_comment() and pr_info() were originally tailored for the text output format, their exact semantics aren't very clear nor carved in stone. So if finer-grained functions are needed to better integrate with the JSON output, this is an option. > 3) I converted all printf() functions from dmidecode.c to pr_printf() and > it does print anything, when --json is used now. I would like to put > such information to output somehow, when --json is used, but I have > to admit that this will not be easy. Any suggestions? I must look into the code to figure out what you did exactly and why you had to do it. In theory, all open-coded printfs had already been removed from the regular dmidecode output (the remaining ones were for special modes like --string, which are mutually exclusive with JSON output), so alternative output formats were supposed to just hook into pr_*() functions. If that wasn't the case then this is a bug which should be fixed beforehand. -- Jean Delvare SUSE L3 Support
