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.
Continuation of my review...
[Make CLI option --json mutual exlusive with some other]
mutual -> mutually
Looks good otherwise.
[Added support for attributes, when JSON output is used]
I'm skeptical about using the exact same function for attributes and
sub-attributes. It is true that indentation doesn't matter for JSON
output, however in text output this identation is not only visual, it
has a meaning. It implies that the (indented) sub-attributes relate to
the last attribute printed. This relation should be visible in the JSON
structure.
With the current implementation, we may have DMI tables where you will
try to add an already existing hash key. Consider the following type 42
record:
Handle 0x0072, DMI type 42, 145 bytes
Management Controller Host Interface
Host Interface Type: Network
Device Type: USB
idVendor: 0x03f0
idProduct: 0x2927
Protocol ID: 04 (Redfish over IP)
Service UUID: a1e45e76-4b48-5d97-9926-b44bd79f9432
Host IP Assignment Type: DHCP
Host IP Address Format: IPv4
Redfish Service IP Discovery Type: DHCP
Redfish Service IP Address Format: IPv4
Redfish Service Hostname: redfish.thecircle.net
If we ever support another protocol ID, that protocol will come with
its own "Service UUID" sub-attribute, which you won't be able to add
because it already exists.
So at the very least you would need to prefix the sub-attribute keys
with their parent attribute, to make them unique again.
But IMHO it would be even cleaner if the parent-children relation would
be present in the JSON document structure. After all, JSON is designed
to represent complex data structures.
Note that I am open to changes to the current implementation if that
would make implementing JSON output cleanly easier. For example, if it
would help if printing the parent attribute was implemented as a
dedicated function instead of the generic pr_attr() (much like we have
pr_list_start), we can do this.
I also see that HPE-specific type 199 is going to cause trouble at the
pr_attr level already, as it prints multiple entries with the same name
(CPU ID). We'll have to change that, else the JSON output will be
invalid.
About the code itself:
+static void attr_json(const char *name, const char *format, va_list args) {
Opening curly brace should go to the following line.
+ char *str = NULL;
+ int ret;
Please leave a blank line between variable declarations and the actual
code.
+ ret = vasprintf(&str, format, args);
+ if (ret != -1)
+ {
+ /* Convert name to lowercase and replace " " with "_" */
I'm curious, is there anything in the JSON spec which prevents you from
using the name string as a hash key as is?
+ char *key = strdup(name);
+ if (key != NULL)
+ {
+ int i;
+ for (i = 0; key[i]; i++)
+ {
+ if (key[i] == ' ')
+ {
+ key[i] = '_';
+ }
+ else
+ {
+ key[i] = tolower(key[i]);
+ }
What about other special characters? I can't promise there won't be
any. There's at least one name field which includes a "-" already (in
DMI type 1) and one which includes a "." (in HPE-specific type 219). A
few include a digit too (HPE-specific type 236 for example) and one
includes a "/" (in HPE-specific type 240).
+ }
+ ret = json_object_object_add(json_dmi_out.values, key,
json_object_new_string(str));
+ if (ret < 0)
+ fprintf(stderr, "Unable to add JSON object:
'%s' with key: '%s'\n", str, key);
+ free(key);
It's really sad if json-c can't use the provided key string directly
and always makes a copy...
[Added support for lists, when JSON output is used]
static void pr_list_start_json(const char *name, const char *format, va_list
args)
{
- (void)name;
- (void)format;
- (void)args;
+ char *key = NULL;
Missing blank line here.
+ if (format != NULL)
+ {
+ char *str=NULL;
Missing spaces around "=".
+ int ret;
Missing blank line here.
+ ret = vasprintf(&str, format, args);
+ if (ret != -1)
+ {
+ /* try to create key from name and format string */
The name itself is guaranteed to be unique, so you can ignore the
formatted value string for the hash key. This will save some code and
memory, and the key will make more sense too.
Typically the format will be a count of the list items. If you are
going to print it then it should be as a value, not as a key. Or we may
decide that JSON users can easily count the list items themselves.
I see we have one oddity in the code at the moment, in
dmi_base_board_features(), if there are not features to list, we print
"None" as the formatted value, and pr_list_item() is never called.
We'll have to ensure the JSON output can cope with that, or adjust it
if needed.
+ size_t len_name = strlen(name);
+ size_t len_str = strlen(str);
Missing blank line here.
+ key = malloc(len_name + len_str + 1);
+ if (key != NULL)
+ {
+ key[0] = '\0';
Not needed, strcpy is guaranteed to NULL-terminate the buffer it writes
to.
+ strcpy(key, name);
+ strcat(key, str);
+ }
+ else
+ {
+ fprintf(stderr, "Unable to allocate memory
(size: %ld) for JSON key\n", len_name + len_str + 1);
+ }
+ free(str);
+ }
+ else
+ {
+ fprintf(stderr, "Unable to create JSON key from name:
'%s' and format: '%s'\n", name, format);
+ }
+ }
+ else
+ {
+ key = strdup(name);
+ }
+ if (key != NULL)
+ {
+ json_dmi_out.list = json_object_new_array();
+ int i;
Missing blank line here.
+ /* Convert key to lowercase and replace " " with "_" */
+ for (i = 0; key[i]; i++)
+ {
+ if (key[i] == ' ')
+ {
+ key[i] = '_';
+ }
+ else
+ {
+ key[i] = tolower(key[i]);
+ }
+ }
You've been doing the same in attr_json() already, so this clearly
deserves to go to a separate helper function.
+ json_object_object_add(json_dmi_out.values, key,
json_dmi_out.list);
+ free(key);
+ }
}
static void pr_list_item_json(const char *format, va_list args)
{
- (void)format;
- (void)args;
+ char *str = NULL;
+ int ret;
Blank line here.
+ if (json_dmi_out.list)
+ {
+ ret = vasprintf(&str, format, args);
+ if (ret != -1)
+ {
+ json_object_array_add(json_dmi_out.list,
json_object_new_string(str));
+ free(str);
+ }
+ }
+ else
+ {
+ ret = vasprintf(&str, format, args);
+ if (ret != -1)
+ {
+ fprintf(stderr, "Unable to add JSON item '%s' to the
non existing list\n", str);
+ free(str);
+ }
I personally would not bother checking json_dmi_out.list, as a crash
with a backtrace is the best way to investigate the bug, and this
avoids useless code at runtime. But if insist on checking it, then the
code above can be rewritten in a way which avoids duplicating code:
call vasprintf() first, and use json_dmi_out.list to decide whether you
should call json_object_array_add() or print an error message.
+ }
}
I'll be away next week, I'll continue with the review work when I
return.
Thanks,
--
Jean Delvare
SUSE L3 Support
