Skip to content

nvme-print-binary: fix to output reachability associations log length#2708

Merged
igaw merged 4 commits intolinux-nvme:masterfrom
ikegami-t:ra-log-len
Feb 17, 2025
Merged

nvme-print-binary: fix to output reachability associations log length#2708
igaw merged 4 commits intolinux-nvme:masterfrom
ikegami-t:ra-log-len

Conversation

@ikegami-t
Copy link
Contributor

Previously only the log header output since the log length is changeable. This changes to output the changeable log length correctly.

char json_str[STR_LEN];
struct json_object *rad;

obj_add_debug(uint64, r, "len", len);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if we should add debug output to the json output. For debugging purposes we have the normal output.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed to delete the debug output but added to use UNUSED macro instead since the len parameter not used.

static void binary_reachability_groups_log(struct nvme_reachability_groups_log *log, __u64 len)
{
d_raw((unsigned char *)log, sizeof(*log));
print_debug("len: %"PRIu64"\n", (uint64_t)len);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as with the json output. I don't know if we want to add to every output format debug information. I am fine with having the normal output format annotated for debugging purposes, but adding trippling the debug output seems add unnecessary maintenance burden.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes agreed then delete the output.

@ikegami-t ikegami-t force-pushed the ra-log-len branch 3 times, most recently from a2af098 to 21218ee Compare February 17, 2025 13:29
@igaw
Copy link
Collaborator

igaw commented Feb 17, 2025

I've just updated the cross build containers, the newer tool chains finds a bunch of new bugs. Don't worry about them, I'll fix them up.

@ikegami-t
Copy link
Contributor Author

I see and noted. Thank you.

@igaw
Copy link
Collaborator

igaw commented Feb 17, 2025

The build works again. Could you rebase to the current HEAD? Thanks!

Previously only the log header output since the log length is changeable.
The changes to output the changeable log length correctly.

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
Previously only the log header output since the log length is changeable.
The changes to output the changeable log length correctly.

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
…ength

Previously only the log header output since the log length is changeable.
The changes to output the changeable log length correctly.

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
Previously incorrectly the log length is calculated by the LE log data.
So fix to convert the value from LE to host endian for the calculation.

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
@ikegami-t
Copy link
Contributor Author

Sure done then all checks passed. Thank you.

@igaw igaw merged commit c785a0a into linux-nvme:master Feb 17, 2025
17 checks passed
@igaw
Copy link
Collaborator

igaw commented Feb 17, 2025

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants