On 2021/6/4 23:04, Pattan, Reshma wrote:

quoted

-----Original Message-----
From: Min Hu (Connor) <redacted>

<snip> 

quoted

+		ret = rte_eth_dev_get_reg_info(i, &reg_info);
+		if (ret) {
+			printf("Error getting device reg info: %d\n", ret);
+			continue;
+		}
+
+		buf_size = reg_info.length * reg_info.width;

If it is to get the regs length, you can directly call  "rte_ethtool_get_regs_len(uint16_t port_id)" API , instead of  again writing the above logic.
And use the returned length in below malloc.

This logic is indeed identical to the logic of the "rte_ethtool_get_regs_len" API of Ethtool,
but the method of using the "rte_eth_dev_get_reg_info" API is the case. All users will have
similar code logic when using this API.

And use "rte_ethtool_get_regs_len" API here only reduces "buf_size = reg_info.length * reg_info.width;"
this logic. But at the same time, it introduces the dependence of example/ethtool in procinfo. The code in
example is not compiled by default, which seems not appropriate to import it in app/procinfo.

So, I think it is fine to keep this.

quoted

+		fp_regs = fopen(file_name, "wb");
+		if (fp_regs == NULL) {
+			printf("Error during opening '%s' for writing\n",
+					file_name);

Better to print error string from fopen() errno on failure , to indicate the exact error.

Agree， I will fix it in the next version.

quoted

+		} else {
+			if ((int)fwrite(buf_data, 1, buf_size, fp_regs) !=

Better have "((int)fwrite(buf_data, 1, buf_size, fp_regs)" In separate line and use the returned value inside if check.

Agree， I will fix it in the next version.

quoted

+					buf_size)
+				printf("Error during writing %s\n",
+						file_prefix);

Better to print error string from fwrite errno on failure , to indicate the exact error.

quoted

+			else
+				printf("dump device (%s) regs successfully, "

Reframe the sente to "Device regs dumped successfully"

Agree， I will fix it in the next version.

.