nvme: plot eye chart data and hex dumping VS eye data

[sivaprasad6541]

-Fixed segmentation fault issue in JSON output format for VS Eye data
-Added support to hex dump VS Eye data in both normal and JSON formats
-This enables customers to decode the raw eye chart data if needed
-Ensured compatibility with existing d() and obj_d() hex dump utilities
-Addressed all the review comments that are given as part of pull request #2828

[sivaprasad6541]

@igaw I have addressed all the review comments mentioned in PR #2828 and created this new one. Could you please review.

[sivaprasad6541]

@igaw I have addressed all the review comments mentioned in PR #2828 and created this new one. Could you please review.

@igaw Could you please confirm if I’ve missed anything in the PR? If everything looks good, kindly proceed with the merge to avoid potential conflicts. Thank you!

[igaw]

there a bunch of memory leaks in the error path as far I can tell.

[igaw]

this leaks eye_array.

[sivaprasad6541]

modified as suggested.

[igaw]

this allocation could fail

[sivaprasad6541]

Added check for allocation.

[igaw]

I suggest to use goto error handlilng pattern:

@@ -2126,13 +2126,17 @@ static char *json_eom_printable_eye(struct nvme_eom_lane_desc *lane,
        char *eye = (char *)lane->eye_desc;
        uint16_t nrows = le16_to_cpu(lane->nrows);
        uint16_t ncols = le16_to_cpu(lane->ncols);
-       char *printable = NULL;
+       struct json_object *eye_array;
        char *printable_start = NULL;
-       struct json_object *eye_array = json_create_array();
+       char *printable = NULL;

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

+       eye_array = json_create_array();
+       if (!eye_array)
+               return NULL;
+
        /*
         * Allocate buffer for full printable string (with newlines)
         * +1 for null terminator
@@ -2141,21 +2145,13 @@ static char *json_eom_printable_eye(struct nvme_eom_lane_desc *lane,
        printable_start = printable;

        if (!printable)
-               return NULL;
-
-       if (!eye_array) {
-               free(printable);
-               return NULL;
-       }
+               goto fail_free_eye_array;

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

-               if (!row) {
-                       /* Cleanup on failure */
-                       free(printable_start);
-                       return NULL;
-               }
+               if (!row)
+                       goto fail_free_printable;

                for (int j = 0; j < ncols; j++) {
                        char ch = eye[i * ncols + j];
@@ -2175,6 +2171,13 @@ static char *json_eom_printable_eye(struct nvme_eom_lane_desc *lane,
        obj_add_array(r, "printable_eye", eye_array);

        return printable_start;
+
+fail_free_printable:
+       free(printable);
+fail_free_eye_array:
+       json_free_object(eye_array);
+
+       return NULL;
 }

[sivaprasad6541]

Apologies for the delay, and thank you so much for your feedback and suggestions. I've incorporated the suggested changes. Please take a moment to review and let me know if everything looks good or if any further adjustments are needed.

[igaw]

isn't this leaking jdesc?

[sivaprasad6541]

modified as suggested.

[igaw]

please avoid adding definition inside the code block. add them at the begin of the function or the code block.

[sivaprasad6541]

Modified as suggested.

[igaw]

and this leaks jdec for sure.

[sivaprasad6541]

Added code to free the jdesc.

[igaw]

so maybe something like

        for (i = 0; i < num_descs; i++) {
                struct nvme_eom_lane_desc *desc = p;
+               _cleanup_free_ char *hexstr = NULL;
                unsigned char *vsdata = NULL;
                unsigned int vsdataoffset = 0;
-               struct json_object *jdesc = json_create_object();
-               uint16_t nrows = le16_to_cpu(desc->nrows);
-               uint16_t ncols = le16_to_cpu(desc->ncols);
-               uint16_t edlen = le16_to_cpu(desc->edlen);
+               uint16_t nrows, ncols, edlen;
+               struct json_object *jdesc;
+               char *hexdata;
+
+               jdesc = json_create_object();
+               if (!jdesc)
+                       return;
+
+               nrows = le16_to_cpu(desc->nrows);
+               ncols = le16_to_cpu(desc->ncols);
+               edlen = le16_to_cpu(desc->edlen);

                obj_add_uint(jdesc, "lid", desc->mstatus);
                obj_add_uint(jdesc, "lane", desc->lane);
@@ -2214,16 +2225,17 @@ static void json_phy_rx_eom_descs(struct nvme_phy_rx_eom_log *log,
                        continue;

                /* 2 hex chars + space per byte */
-               _cleanup_free_ char *hexstr = malloc(edlen * 3 + 1);
-
-               if (!hexstr)
+               hexstr = malloc(edlen * 3 + 1);
+               if (!hexstr) {
+                       json_free_object(jdesc);
                        return;
+               }

                /* Hex dump Vendor Specific Eye Data */
                vsdataoffset = (nrows * ncols) + sizeof(struct nvme_eom_lane_desc);
                vsdata = (unsigned char *)((unsigned char *)desc + vsdataoffset);

-               char *hexdata = hexstr;
+               hexdata = hexstr;

                for (int offset = 0; offset < edlen; offset++)

[sivaprasad6541]

Thanks for the suggestions. I've modified the code as per your suggestion. Please review the code.

[igaw]

return value is not checked, could be NULL.

[sivaprasad6541]

Added NULL check as suggested.

[igaw]

and here we leak vsdata.

BTW, valgrind is a very good tool find such leaks, give it a try.

[sivaprasad6541]

Thanks you for the feedback @igaw.
I have ran valgrind after modifying suggested changes and no leeks observed.

[igaw]

Just squashed both changes togehter and reordered some variable deceleration so it follows the inverse Christmas tree layout.

Thanks!

[sivaprasad6541]

Thank you @igaw.