Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 56 additions & 12 deletions nvme-print-json.c
Original file line number Diff line number Diff line change
Expand Up @@ -2121,25 +2121,47 @@ static void json_boot_part_log(void *bp_log, const char *devname,

/* Printable Eye string is allocated and returned, caller must free */
static char *json_eom_printable_eye(struct nvme_eom_lane_desc *lane,
struct json_object *r)
struct json_object *r)
Copy link
Collaborator

Choose a reason for hiding this comment

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

unrelated whitespace change, please drop this one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've undone the unrelated whitespace change as suggested.

{
char *eye = (char *)lane->eye_desc;
char *printable = malloc(lane->nrows * lane->ncols + lane->ncols);
uint16_t nrows = lane->nrows;
uint16_t ncols = lane->ncols;
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be swapped to the host endianness (i.e. le16_to_cpu(lane->nrows)), not that it's a new issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ve updated the code to convert the values to host endianness using le16_to_cpu() as suggested.


if (nrows == 0 || ncols == 0)
return NULL;

// Allocate buffer for full printable string (with newlines)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use /* */ commet style, e.g.

       /*
        *  Allocate buffer for full printable string (with newlines)
        *  +1 for null terminator
        */
       printable = malloc(nrows * ncols + nrows + 1);
       printable_start = printable;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified as suggested.

char *printable = malloc(nrows * ncols + nrows + 1); // +1 for null terminator
Copy link
Collaborator

Choose a reason for hiding this comment

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

Variable declaration are at the beginning of the block. There only a few exceptions allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified as suggested.

char *printable_start = printable;
int i, j;

if (!printable)
goto exit;
return NULL;

struct json_object *eye_array = json_create_array();
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we are checking for null pointers, then this one should also be checked I suppose.

And don't we need to free it eventually? In this case use the cleanup helpers so that you can exit the funtion at anytime. Can't remember what json-c does, if it takes the owner ship.

Copy link
Contributor Author

@sivaprasad6541 sivaprasad6541 May 29, 2025

Choose a reason for hiding this comment

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

Thanks for the suggestion! I initially tried using cleanup helpers, but encountered a segfault in the caller while printing the root JSON object. Since the caller is responsible for freeing the memory, I've kept the manual cleanup logic in place to avoid ownership issues.
Added a null check for eye_array now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

again declaration go to the beginning of the block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this to beginning of the block.


for (int i = 0; i < nrows; i++) {
char *row = malloc(ncols + 1);

if (!row)
continue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make to continue if we run out of memory? Maybe break or return NULL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch. I've modified it to return NULL.


for (i = 0; i < lane->nrows; i++) {
for (j = 0; j < lane->ncols; j++, printable++)
sprintf(printable, "%c", eye[i * lane->ncols + j]);
sprintf(printable++, "\n");
for (int j = 0; j < ncols; j++) {
char ch = eye[i * ncols + j];
*printable++ = ch;
row[j] = ch;
}

*printable++ = '\n';
row[ncols] = '\0';

array_add_str(eye_array, row);
free(row);
}

obj_add_str(r, "printable_eye", printable_start);
*printable = '\0';

obj_add_array(r, "printable_eye_rows", eye_array);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This breaks the 'API', the name changes and existing parsers will be unhappy.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah wait, there is a way for this change. We introduced the possibility to select the output format with --output-format-version, e.g. see f8f086c ("nvme: add output-format-version option")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have renamed it as earlier.
As per #2828 (comment)
If we hear feedback from users later, we can revisit with the --output-format-version approach as you suggested.


exit:
return printable_start;
}

Expand All @@ -2155,6 +2177,8 @@ static void json_phy_rx_eom_descs(struct nvme_phy_rx_eom_log *log,

for (i = 0; i < num_descs; i++) {
struct nvme_eom_lane_desc *desc = p;
unsigned char *vsdata = NULL;
unsigned int vsdataoffset = 0;
struct json_object *jdesc = json_create_object();

obj_add_uint(jdesc, "lid", desc->mstatus);
Expand All @@ -2169,10 +2193,30 @@ static void json_phy_rx_eom_descs(struct nvme_phy_rx_eom_log *log,
obj_add_uint(jdesc, "edlen", le16_to_cpu(desc->edlen));

if (NVME_EOM_ODP_PEFP(log->odp))
allocated_eyes[i] = json_eom_printable_eye(desc, r);
allocated_eyes[i] = json_eom_printable_eye(desc, jdesc);

/* Eye Data field is vendor specific, doesn't map to JSON */
if (desc->edlen == 0)
continue;
else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need for the else branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed else branch as suggested.

/* Hex dump Vendor Specific Eye Data*/
vsdata = (unsigned char *)malloc(desc->edlen);
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need for the cast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the casting as suggested.

vsdataoffset = (desc->nrows * desc->ncols) +
Copy link
Contributor

Choose a reason for hiding this comment

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

The various __le16 fields in desc should be converted to host-endianness before being used. The uses of these fields below should also be swapped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ve updated the code to convert the values to host endianness using le16_to_cpu() as suggested.

sizeof(struct nvme_eom_lane_desc);
Copy link
Collaborator

Choose a reason for hiding this comment

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

indention seems to be off here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing that out! had to break the line due to the 100-column limit, which affected the indentation slightly.
Now, I've modified it to look better indentation.

vsdata = (unsigned char *)((unsigned char *)desc + vsdataoffset);
char *hexstr = malloc(desc->edlen * 3 + 1); // 2 hex chars + space per byte

if (!hexstr)
return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

leaking vsdata?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch.
I've removed memory allocation for vsdata as it is not required.


char *p = hexstr;

for (int offset = 0; offset < desc->edlen; offset++)
p += sprintf(p, "%02X ", vsdata[offset]);
*(p - 1) = '\0'; // remove trailing space

obj_add_str(jdesc, "vsdata_hex", hexstr);
free(hexstr);
}
array_add_obj(descs, jdesc);

p += log->dsize;
Expand Down
14 changes: 14 additions & 0 deletions nvme-print-stdout.c
Original file line number Diff line number Diff line change
Expand Up @@ -790,6 +790,8 @@ static void stdout_phy_rx_eom_descs(struct nvme_phy_rx_eom_log *log)

for (i = 0; i < log->nd; i++) {
struct nvme_eom_lane_desc *desc = p;
unsigned char *vsdata = NULL;
unsigned int vsdataoffset = 0;

printf("Measurement Status: %s\n",
desc->mstatus ? "Successful" : "Not Successful");
Expand All @@ -807,6 +809,18 @@ static void stdout_phy_rx_eom_descs(struct nvme_phy_rx_eom_log *log)
stdout_eom_printable_eye(desc);

/* Eye Data field is vendor specific */
if (desc->edlen == 0)
continue;
else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need for the else branch, reduces the indention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the else branch is suggested.

/* Hex dump Vendor Specific Eye Data */
vsdata = (unsigned char *)malloc(desc->edlen);
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need for the cast

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the cast as suggested.

vsdataoffset = (desc->nrows * desc->ncols) +
Copy link
Contributor

Choose a reason for hiding this comment

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

As on the JSON side, the various __le16 fields in desc should be converted to host-endianness before being used. The use below should also be swapped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ve updated the code to convert the values to host endianness using le16_to_cpu() as suggested.

sizeof(struct nvme_eom_lane_desc);
Copy link
Collaborator

Choose a reason for hiding this comment

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

indention seems to be off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing that out! had to break the line due to the 100-column limit, which affected the indentation slightly.
Now, I've modified it to look better indentation.

vsdata = (unsigned char *)((unsigned char *)desc + vsdataoffset);
printf("Eye Data:\n");
d(vsdata, desc->edlen, 16, 1);
printf("\n");
}

p += log->dsize;
}
Expand Down