Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Feb 17, 2025

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.

{
struct json_object *r = json_create_object();
__u16 i;
__u32 j;
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.

{
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