[PATCH 1/1] app/mldev: add internal function for file read

[PATCH 1/1] app/mldev: add internal function for file read

[Srikanth Yalavarthi]

Added internal function to read model, input and reference
files with required error checks. This change fixes the
unchecked return value and improper use of negative value
issues reported by coverity scan for file read operations.

Coverity issue: 383742, 383743
Fixes: f6661e6d9a3a ("app/mldev: validate model operations")
Fixes: da6793390596 ("app/mldev: support inference validation")

Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
---
 app/test-mldev/test_common.c           | 59 ++++++++++++++++++++++++++
 app/test-mldev/test_common.h           |  2 +
 app/test-mldev/test_inference_common.c | 54 +++++++++--------------
 app/test-mldev/test_model_common.c     | 33 +++-----------
 4 files changed, 87 insertions(+), 61 deletions(-)

diff --git a/app/test-mldev/test_common.c b/app/test-mldev/test_common.c
index 016b31c6ba..3f450d6a4d 100644
--- a/app/test-mldev/test_common.c
+++ b/app/test-mldev/test_common.c
@@ -5,12 +5,71 @@
 #include <errno.h>
 
 #include <rte_common.h>
+#include <rte_malloc.h>
 #include <rte_memory.h>
 #include <rte_mldev.h>
 
 #include "ml_common.h"
 #include "test_common.h"
 
+int
+ml_read_file(char *file, size_t *size, char **buffer)
+{
+	char *file_buffer = NULL;
+	long file_size = 0;
+	int ret = 0;
+	FILE *fp;
+
+	fp = fopen(file, "r");
+	if (fp == NULL) {
+		ml_err("Failed to open file: %s\n", file);
+		return -EIO;
+	}
+
+	if (fseek(fp, 0, SEEK_END) == 0) {
+		file_size = ftell(fp);
+		if (file_size == -1) {
+			ret = -EIO;
+			goto error;
+		}
+
+		file_buffer = rte_malloc(NULL, file_size, RTE_CACHE_LINE_SIZE);
+		if (file_buffer == NULL) {
+			ml_err("Failed to allocate memory: %s\n", file);
+			ret = -ENOMEM;
+			goto error;
+		}
+
+		if (fseek(fp, 0, SEEK_SET) != 0) {
+			ret = -EIO;
+			goto error;
+		}
+
+		if (fread(file_buffer, sizeof(char), file_size, fp) != (unsigned long)file_size) {
+			ml_err("Failed to read file : %s\n", file);
+			ret = -EIO;
+			goto error;
+		}
+		fclose(fp);
+	} else {
+		ret = -EIO;
+		goto error;
+	}
+
+	*buffer = file_buffer;
+	*size = file_size;
+
+	return 0;
+
+error:
+	rte_free(file_buffer);
+
+	if (fp != NULL)
+		fclose(fp);
+
+	return ret;
+}
+
 bool
 ml_test_cap_check(struct ml_options *opt)
 {
diff --git a/app/test-mldev/test_common.h b/app/test-mldev/test_common.h
index a7b2ea652a..7e3634b0c6 100644
--- a/app/test-mldev/test_common.h
+++ b/app/test-mldev/test_common.h
@@ -24,4 +24,6 @@ int ml_test_device_close(struct ml_test *test, struct ml_options *opt);
 int ml_test_device_start(struct ml_test *test, struct ml_options *opt);
 int ml_test_device_stop(struct ml_test *test, struct ml_options *opt);
 
+int ml_read_file(char *file, size_t *size, char **buffer);
+
 #endif /* TEST_COMMON_H */
diff --git a/app/test-mldev/test_inference_common.c b/app/test-mldev/test_inference_common.c
index af831fc1bf..c8cd80d69f 100644
--- a/app/test-mldev/test_inference_common.c
+++ b/app/test-mldev/test_inference_common.c
@@ -604,10 +604,10 @@ ml_inference_iomem_setup(struct ml_test *test, struct ml_options *opt, uint16_t
 	char mp_name[RTE_MEMPOOL_NAMESIZE];
 	const struct rte_memzone *mz;
 	uint64_t nb_buffers;
+	char *buffer = NULL;
 	uint32_t buff_size;
 	uint32_t mz_size;
-	uint32_t fsize;
-	FILE *fp;
+	size_t fsize;
 	int ret;
 
 	/* get input buffer size */
@@ -647,51 +647,35 @@ ml_inference_iomem_setup(struct ml_test *test, struct ml_options *opt, uint16_t
 		t->model[fid].reference = NULL;
 
 	/* load input file */
-	fp = fopen(opt->filelist[fid].input, "r");
-	if (fp == NULL) {
-		ml_err("Failed to open input file : %s\n", opt->filelist[fid].input);
-		ret = -errno;
+	ret = ml_read_file(opt->filelist[fid].input, &fsize, &buffer);
+	if (ret != 0)
 		goto error;
-	}
 
-	fseek(fp, 0, SEEK_END);
-	fsize = ftell(fp);
-	fseek(fp, 0, SEEK_SET);
-	if (fsize != t->model[fid].inp_dsize) {
-		ml_err("Invalid input file, size = %u (expected size = %" PRIu64 ")\n", fsize,
+	if (fsize == t->model[fid].inp_dsize) {
+		rte_memcpy(t->model[fid].input, buffer, fsize);
+		rte_free(buffer);
+	} else {
+		ml_err("Invalid input file, size = %zu (expected size = %" PRIu64 ")\n", fsize,
 		       t->model[fid].inp_dsize);
 		ret = -EINVAL;
-		fclose(fp);
-		goto error;
-	}
-
-	if (fread(t->model[fid].input, 1, t->model[fid].inp_dsize, fp) != t->model[fid].inp_dsize) {
-		ml_err("Failed to read input file : %s\n", opt->filelist[fid].input);
-		ret = -errno;
-		fclose(fp);
 		goto error;
 	}
-	fclose(fp);
 
 	/* load reference file */
 	if (t->model[fid].reference != NULL) {
-		fp = fopen(opt->filelist[fid].reference, "r");
-		if (fp == NULL) {
-			ml_err("Failed to open reference file : %s\n",
-			       opt->filelist[fid].reference);
-			ret = -errno;
+		ret = ml_read_file(opt->filelist[fid].reference, &fsize, &buffer);
+		if (ret != 0)
 			goto error;
-		}
 
-		if (fread(t->model[fid].reference, 1, t->model[fid].out_dsize, fp) !=
-		    t->model[fid].out_dsize) {
-			ml_err("Failed to read reference file : %s\n",
-			       opt->filelist[fid].reference);
-			ret = -errno;
-			fclose(fp);
+		if (fsize == t->model[fid].out_dsize) {
+			rte_memcpy(t->model[fid].reference, buffer, fsize);
+			rte_free(buffer);
+		} else {
+			ml_err("Invalid reference file, size = %zu (expected size = %" PRIu64 ")\n",
+			       fsize, t->model[fid].out_dsize);
+			ret = -EINVAL;
 			goto error;
 		}
-		fclose(fp);
 	}
 
 	/* create mempool for quantized input and output buffers. ml_request_initialize is
@@ -723,6 +707,8 @@ ml_inference_iomem_setup(struct ml_test *test, struct ml_options *opt, uint16_t
 		t->model[fid].io_pool = NULL;
 	}
 
+	rte_free(buffer);
+
 	return ret;
 }
 
diff --git a/app/test-mldev/test_model_common.c b/app/test-mldev/test_model_common.c
index c28e452f29..7a8c284d13 100644
--- a/app/test-mldev/test_model_common.c
+++ b/app/test-mldev/test_model_common.c
@@ -14,11 +14,11 @@
 int
 ml_model_load(struct ml_test *test, struct ml_options *opt, struct ml_model *model, uint16_t fid)
 {
-	struct test_common *t = ml_test_priv(test);
 	struct rte_ml_model_params model_params;
-	FILE *fp;
 	int ret;
 
+	RTE_SET_USED(test);
+
 	if (model->state == MODEL_LOADED)
 		return 0;
 
@@ -26,31 +26,10 @@ ml_model_load(struct ml_test *test, struct ml_options *opt, struct ml_model *mod
 		return -EINVAL;
 
 	/* read model binary */
-	fp = fopen(opt->filelist[fid].model, "r");
-	if (fp == NULL) {
-		ml_err("Failed to open model file : %s\n", opt->filelist[fid].model);
-		return -1;
-	}
-
-	fseek(fp, 0, SEEK_END);
-	model_params.size = ftell(fp);
-	fseek(fp, 0, SEEK_SET);
-
-	model_params.addr = rte_malloc_socket("ml_model", model_params.size,
-					      t->dev_info.min_align_size, opt->socket_id);
-	if (model_params.addr == NULL) {
-		ml_err("Failed to allocate memory for model: %s\n", opt->filelist[fid].model);
-		fclose(fp);
-		return -ENOMEM;
-	}
-
-	if (fread(model_params.addr, 1, model_params.size, fp) != model_params.size) {
-		ml_err("Failed to read model file : %s\n", opt->filelist[fid].model);
-		rte_free(model_params.addr);
-		fclose(fp);
-		return -1;
-	}
-	fclose(fp);
+	ret = ml_read_file(opt->filelist[fid].model, &model_params.size,
+			   (char **)&model_params.addr);
+	if (ret != 0)
+		return ret;
 
 	/* load model to device */
 	ret = rte_ml_model_load(opt->dev_id, &model_params, &model->id);
-- 
2.17.1

Re: [PATCH 1/1] app/mldev: add internal function for file read

[Stephen Hemminger]

On Thu, 23 Mar 2023 08:28:01 -0700
Srikanth Yalavarthi <syalavarthi@marvell.com> wrote:

> +	if (fseek(fp, 0, SEEK_END) == 0) {
> +		file_size = ftell(fp);
> +		if (file_size == -1) {
> +			ret = -EIO;
> +			goto error;
> +		}
> +
> +		file_buffer = rte_malloc(NULL, file_size, RTE_CACHE_LINE_SIZE);
> +		if (file_buffer == NULL) {
> +			ml_err("Failed to allocate memory: %s\n", file);
> +			ret = -ENOMEM;
> +			goto error;
> +		}
> +
> +		if (fseek(fp, 0, SEEK_SET) != 0) {
> +			ret = -EIO;
> +			goto error;
> +		}
> +
> +		if (fread(file_buffer, sizeof(char), file_size, fp) != (unsigned long)file_size) {
> +			ml_err("Failed to read file : %s\n", file);
> +			ret = -EIO;
> +			goto error;
> +		}
> +		fclose(fp);
> +	} else {
> +		ret = -EIO;
> +		goto error;
> +	}
> +
> +	*buffer = file_buffer;
> +	*size = file_size;
> +
> +	return 0;

Granted this only test code, but is the slowest way to do this.
Stdio is buffered (in 4K chunks). And using rte_malloc comes from hugepages.

Three levels of improvement are possible:
  1. don't use rte_malloc() use malloc() instead.
  2. use direct system call for I/O
  3. use mmap() to directly map in the file instead read

RE: [EXT] Re: [PATCH 1/1] app/mldev: add internal function for file read

[Srikanth Yalavarthi]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: 28 March 2023 21:22
> To: Srikanth Yalavarthi <syalavarthi@marvell.com>
> Cc: Anup Prabhu <aprabhu@marvell.com>; dev@dpdk.org; Shivah Shankar
> Shankar Narayan Rao <sshankarnara@marvell.com>; Prince Takkar
> <ptakkar@marvell.com>; Srikanth Yalavarthi <syalavarthi@marvell.com>
> Subject: [EXT] Re: [PATCH 1/1] app/mldev: add internal function for file read
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Thu, 23 Mar 2023 08:28:01 -0700
> Srikanth Yalavarthi <syalavarthi@marvell.com> wrote:
> 
> > +	if (fseek(fp, 0, SEEK_END) == 0) {
> > +		file_size = ftell(fp);
> > +		if (file_size == -1) {
> > +			ret = -EIO;
> > +			goto error;
> > +		}
> > +
> > +		file_buffer = rte_malloc(NULL, file_size,
> RTE_CACHE_LINE_SIZE);
> > +		if (file_buffer == NULL) {
> > +			ml_err("Failed to allocate memory: %s\n", file);
> > +			ret = -ENOMEM;
> > +			goto error;
> > +		}
> > +
> > +		if (fseek(fp, 0, SEEK_SET) != 0) {
> > +			ret = -EIO;
> > +			goto error;
> > +		}
> > +
> > +		if (fread(file_buffer, sizeof(char), file_size, fp) != (unsigned
> long)file_size) {
> > +			ml_err("Failed to read file : %s\n", file);
> > +			ret = -EIO;
> > +			goto error;
> > +		}
> > +		fclose(fp);
> > +	} else {
> > +		ret = -EIO;
> > +		goto error;
> > +	}
> > +
> > +	*buffer = file_buffer;
> > +	*size = file_size;
> > +
> > +	return 0;
> 
> Granted this only test code, but is the slowest way to do this.
> Stdio is buffered (in 4K chunks). And using rte_malloc comes from
> hugepages.
> 
> Three levels of improvement are possible:
>   1. don't use rte_malloc() use malloc() instead.
Agree on this. Will update in next version.

>   2. use direct system call for I/O
>   3. use mmap() to directly map in the file instead read
Agree on the improvements.
But, considering that this is a test code and these operations are done in slow-path, I would prefer to have the implementation based on C library calls rather than using system calls.

Also, using system calls may not make this code portable? Though we are not supporting this app on platforms other than Linux, as of now.
Pls let me know what you think.

[PATCH v2] app/mldev: add internal function for file read

[Srikanth Yalavarthi]

Added internal function to read model, input and reference
files with required error checks. This change fixes the
unchecked return value and improper use of negative value
issues reported by coverity scan for file read operations.

Coverity issue: 383742, 383743
Fixes: f6661e6d9a3a ("app/mldev: validate model operations")
Fixes: da6793390596 ("app/mldev: support inference validation")

Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
---
 app/test-mldev/test_common.c           | 59 ++++++++++++++++++++++++++
 app/test-mldev/test_common.h           |  2 +
 app/test-mldev/test_inference_common.c | 54 +++++++++--------------
 app/test-mldev/test_model_common.c     | 33 +++-----------
 4 files changed, 87 insertions(+), 61 deletions(-)

diff --git a/app/test-mldev/test_common.c b/app/test-mldev/test_common.c
index 016b31c6ba..be67ea487c 100644
--- a/app/test-mldev/test_common.c
+++ b/app/test-mldev/test_common.c
@@ -5,12 +5,71 @@
 #include <errno.h>
 
 #include <rte_common.h>
+#include <rte_malloc.h>
 #include <rte_memory.h>
 #include <rte_mldev.h>
 
 #include "ml_common.h"
 #include "test_common.h"
 
+int
+ml_read_file(char *file, size_t *size, char **buffer)
+{
+	char *file_buffer = NULL;
+	long file_size = 0;
+	int ret = 0;
+	FILE *fp;
+
+	fp = fopen(file, "r");
+	if (fp == NULL) {
+		ml_err("Failed to open file: %s\n", file);
+		return -EIO;
+	}
+
+	if (fseek(fp, 0, SEEK_END) == 0) {
+		file_size = ftell(fp);
+		if (file_size == -1) {
+			ret = -EIO;
+			goto error;
+		}
+
+		file_buffer = malloc(file_size);
+		if (file_buffer == NULL) {
+			ml_err("Failed to allocate memory: %s\n", file);
+			ret = -ENOMEM;
+			goto error;
+		}
+
+		if (fseek(fp, 0, SEEK_SET) != 0) {
+			ret = -EIO;
+			goto error;
+		}
+
+		if (fread(file_buffer, sizeof(char), file_size, fp) != (unsigned long)file_size) {
+			ml_err("Failed to read file : %s\n", file);
+			ret = -EIO;
+			goto error;
+		}
+		fclose(fp);
+	} else {
+		ret = -EIO;
+		goto error;
+	}
+
+	*buffer = file_buffer;
+	*size = file_size;
+
+	return 0;
+
+error:
+	rte_free(file_buffer);
+
+	if (fp != NULL)
+		fclose(fp);
+
+	return ret;
+}
+
 bool
 ml_test_cap_check(struct ml_options *opt)
 {
diff --git a/app/test-mldev/test_common.h b/app/test-mldev/test_common.h
index a7b2ea652a..7e3634b0c6 100644
--- a/app/test-mldev/test_common.h
+++ b/app/test-mldev/test_common.h
@@ -24,4 +24,6 @@ int ml_test_device_close(struct ml_test *test, struct ml_options *opt);
 int ml_test_device_start(struct ml_test *test, struct ml_options *opt);
 int ml_test_device_stop(struct ml_test *test, struct ml_options *opt);
 
+int ml_read_file(char *file, size_t *size, char **buffer);
+
 #endif /* TEST_COMMON_H */
diff --git a/app/test-mldev/test_inference_common.c b/app/test-mldev/test_inference_common.c
index 29c18bbc85..96213413a2 100644
--- a/app/test-mldev/test_inference_common.c
+++ b/app/test-mldev/test_inference_common.c
@@ -610,10 +610,10 @@ ml_inference_iomem_setup(struct ml_test *test, struct ml_options *opt, uint16_t
 	char mp_name[RTE_MEMPOOL_NAMESIZE];
 	const struct rte_memzone *mz;
 	uint64_t nb_buffers;
+	char *buffer = NULL;
 	uint32_t buff_size;
 	uint32_t mz_size;
-	uint32_t fsize;
-	FILE *fp;
+	size_t fsize;
 	int ret;
 
 	/* get input buffer size */
@@ -653,51 +653,35 @@ ml_inference_iomem_setup(struct ml_test *test, struct ml_options *opt, uint16_t
 		t->model[fid].reference = NULL;
 
 	/* load input file */
-	fp = fopen(opt->filelist[fid].input, "r");
-	if (fp == NULL) {
-		ml_err("Failed to open input file : %s\n", opt->filelist[fid].input);
-		ret = -errno;
+	ret = ml_read_file(opt->filelist[fid].input, &fsize, &buffer);
+	if (ret != 0)
 		goto error;
-	}
 
-	fseek(fp, 0, SEEK_END);
-	fsize = ftell(fp);
-	fseek(fp, 0, SEEK_SET);
-	if (fsize != t->model[fid].inp_dsize) {
-		ml_err("Invalid input file, size = %u (expected size = %" PRIu64 ")\n", fsize,
+	if (fsize == t->model[fid].inp_dsize) {
+		rte_memcpy(t->model[fid].input, buffer, fsize);
+		rte_free(buffer);
+	} else {
+		ml_err("Invalid input file, size = %zu (expected size = %" PRIu64 ")\n", fsize,
 		       t->model[fid].inp_dsize);
 		ret = -EINVAL;
-		fclose(fp);
-		goto error;
-	}
-
-	if (fread(t->model[fid].input, 1, t->model[fid].inp_dsize, fp) != t->model[fid].inp_dsize) {
-		ml_err("Failed to read input file : %s\n", opt->filelist[fid].input);
-		ret = -errno;
-		fclose(fp);
 		goto error;
 	}
-	fclose(fp);
 
 	/* load reference file */
 	if (t->model[fid].reference != NULL) {
-		fp = fopen(opt->filelist[fid].reference, "r");
-		if (fp == NULL) {
-			ml_err("Failed to open reference file : %s\n",
-			       opt->filelist[fid].reference);
-			ret = -errno;
+		ret = ml_read_file(opt->filelist[fid].reference, &fsize, &buffer);
+		if (ret != 0)
 			goto error;
-		}
 
-		if (fread(t->model[fid].reference, 1, t->model[fid].out_dsize, fp) !=
-		    t->model[fid].out_dsize) {
-			ml_err("Failed to read reference file : %s\n",
-			       opt->filelist[fid].reference);
-			ret = -errno;
-			fclose(fp);
+		if (fsize == t->model[fid].out_dsize) {
+			rte_memcpy(t->model[fid].reference, buffer, fsize);
+			rte_free(buffer);
+		} else {
+			ml_err("Invalid reference file, size = %zu (expected size = %" PRIu64 ")\n",
+			       fsize, t->model[fid].out_dsize);
+			ret = -EINVAL;
 			goto error;
 		}
-		fclose(fp);
 	}
 
 	/* create mempool for quantized input and output buffers. ml_request_initialize is
@@ -729,6 +713,8 @@ ml_inference_iomem_setup(struct ml_test *test, struct ml_options *opt, uint16_t
 		t->model[fid].io_pool = NULL;
 	}
 
+	rte_free(buffer);
+
 	return ret;
 }
 
diff --git a/app/test-mldev/test_model_common.c b/app/test-mldev/test_model_common.c
index c28e452f29..7a8c284d13 100644
--- a/app/test-mldev/test_model_common.c
+++ b/app/test-mldev/test_model_common.c
@@ -14,11 +14,11 @@
 int
 ml_model_load(struct ml_test *test, struct ml_options *opt, struct ml_model *model, uint16_t fid)
 {
-	struct test_common *t = ml_test_priv(test);
 	struct rte_ml_model_params model_params;
-	FILE *fp;
 	int ret;
 
+	RTE_SET_USED(test);
+
 	if (model->state == MODEL_LOADED)
 		return 0;
 
@@ -26,31 +26,10 @@ ml_model_load(struct ml_test *test, struct ml_options *opt, struct ml_model *mod
 		return -EINVAL;
 
 	/* read model binary */
-	fp = fopen(opt->filelist[fid].model, "r");
-	if (fp == NULL) {
-		ml_err("Failed to open model file : %s\n", opt->filelist[fid].model);
-		return -1;
-	}
-
-	fseek(fp, 0, SEEK_END);
-	model_params.size = ftell(fp);
-	fseek(fp, 0, SEEK_SET);
-
-	model_params.addr = rte_malloc_socket("ml_model", model_params.size,
-					      t->dev_info.min_align_size, opt->socket_id);
-	if (model_params.addr == NULL) {
-		ml_err("Failed to allocate memory for model: %s\n", opt->filelist[fid].model);
-		fclose(fp);
-		return -ENOMEM;
-	}
-
-	if (fread(model_params.addr, 1, model_params.size, fp) != model_params.size) {
-		ml_err("Failed to read model file : %s\n", opt->filelist[fid].model);
-		rte_free(model_params.addr);
-		fclose(fp);
-		return -1;
-	}
-	fclose(fp);
+	ret = ml_read_file(opt->filelist[fid].model, &model_params.size,
+			   (char **)&model_params.addr);
+	if (ret != 0)
+		return ret;
 
 	/* load model to device */
 	ret = rte_ml_model_load(opt->dev_id, &model_params, &model->id);
-- 
2.17.1

[PATCH v3] app/mldev: add internal function for file read

[Srikanth Yalavarthi]

Added internal function to read model, input and reference
files with required error checks. This change fixes the
unchecked return value and improper use of negative value
issues reported by coverity scan for file read operations.

Coverity issue: 383742, 383743
Fixes: f6661e6d9a3a ("app/mldev: validate model operations")
Fixes: da6793390596 ("app/mldev: support inference validation")

Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
---
v3:
* Fix incorrect use of rte_free with free

v2:
* Replace rte_malloc in ml_read_file with malloc

v1:
* Initial patch

 app/test-mldev/test_common.c           | 59 ++++++++++++++++++++++++++
 app/test-mldev/test_common.h           |  2 +
 app/test-mldev/test_inference_common.c | 54 +++++++++--------------
 app/test-mldev/test_model_common.c     | 39 ++++-------------
 4 files changed, 90 insertions(+), 64 deletions(-)

diff --git a/app/test-mldev/test_common.c b/app/test-mldev/test_common.c
index 016b31c6ba..d8a8e8a448 100644
--- a/app/test-mldev/test_common.c
+++ b/app/test-mldev/test_common.c
@@ -5,12 +5,71 @@
 #include <errno.h>

 #include <rte_common.h>
+#include <rte_malloc.h>
 #include <rte_memory.h>
 #include <rte_mldev.h>

 #include "ml_common.h"
 #include "test_common.h"

+int
+ml_read_file(char *file, size_t *size, char **buffer)
+{
+	char *file_buffer = NULL;
+	long file_size = 0;
+	int ret = 0;
+	FILE *fp;
+
+	fp = fopen(file, "r");
+	if (fp == NULL) {
+		ml_err("Failed to open file: %s\n", file);
+		return -EIO;
+	}
+
+	if (fseek(fp, 0, SEEK_END) == 0) {
+		file_size = ftell(fp);
+		if (file_size == -1) {
+			ret = -EIO;
+			goto error;
+		}
+
+		file_buffer = malloc(file_size);
+		if (file_buffer == NULL) {
+			ml_err("Failed to allocate memory: %s\n", file);
+			ret = -ENOMEM;
+			goto error;
+		}
+
+		if (fseek(fp, 0, SEEK_SET) != 0) {
+			ret = -EIO;
+			goto error;
+		}
+
+		if (fread(file_buffer, sizeof(char), file_size, fp) != (unsigned long)file_size) {
+			ml_err("Failed to read file : %s\n", file);
+			ret = -EIO;
+			goto error;
+		}
+		fclose(fp);
+	} else {
+		ret = -EIO;
+		goto error;
+	}
+
+	*buffer = file_buffer;
+	*size = file_size;
+
+	return 0;
+
+error:
+	free(file_buffer);
+
+	if (fp != NULL)
+		fclose(fp);
+
+	return ret;
+}
+
 bool
 ml_test_cap_check(struct ml_options *opt)
 {
diff --git a/app/test-mldev/test_common.h b/app/test-mldev/test_common.h
index a7b2ea652a..7e3634b0c6 100644
--- a/app/test-mldev/test_common.h
+++ b/app/test-mldev/test_common.h
@@ -24,4 +24,6 @@ int ml_test_device_close(struct ml_test *test, struct ml_options *opt);
 int ml_test_device_start(struct ml_test *test, struct ml_options *opt);
 int ml_test_device_stop(struct ml_test *test, struct ml_options *opt);

+int ml_read_file(char *file, size_t *size, char **buffer);
+
 #endif /* TEST_COMMON_H */
diff --git a/app/test-mldev/test_inference_common.c b/app/test-mldev/test_inference_common.c
index af831fc1bf..9a1c706e11 100644
--- a/app/test-mldev/test_inference_common.c
+++ b/app/test-mldev/test_inference_common.c
@@ -604,10 +604,10 @@ ml_inference_iomem_setup(struct ml_test *test, struct ml_options *opt, uint16_t
 	char mp_name[RTE_MEMPOOL_NAMESIZE];
 	const struct rte_memzone *mz;
 	uint64_t nb_buffers;
+	char *buffer = NULL;
 	uint32_t buff_size;
 	uint32_t mz_size;
-	uint32_t fsize;
-	FILE *fp;
+	size_t fsize;
 	int ret;

 	/* get input buffer size */
@@ -647,51 +647,35 @@ ml_inference_iomem_setup(struct ml_test *test, struct ml_options *opt, uint16_t
 		t->model[fid].reference = NULL;

 	/* load input file */
-	fp = fopen(opt->filelist[fid].input, "r");
-	if (fp == NULL) {
-		ml_err("Failed to open input file : %s\n", opt->filelist[fid].input);
-		ret = -errno;
+	ret = ml_read_file(opt->filelist[fid].input, &fsize, &buffer);
+	if (ret != 0)
 		goto error;
-	}

-	fseek(fp, 0, SEEK_END);
-	fsize = ftell(fp);
-	fseek(fp, 0, SEEK_SET);
-	if (fsize != t->model[fid].inp_dsize) {
-		ml_err("Invalid input file, size = %u (expected size = %" PRIu64 ")\n", fsize,
+	if (fsize == t->model[fid].inp_dsize) {
+		rte_memcpy(t->model[fid].input, buffer, fsize);
+		free(buffer);
+	} else {
+		ml_err("Invalid input file, size = %zu (expected size = %" PRIu64 ")\n", fsize,
 		       t->model[fid].inp_dsize);
 		ret = -EINVAL;
-		fclose(fp);
-		goto error;
-	}
-
-	if (fread(t->model[fid].input, 1, t->model[fid].inp_dsize, fp) != t->model[fid].inp_dsize) {
-		ml_err("Failed to read input file : %s\n", opt->filelist[fid].input);
-		ret = -errno;
-		fclose(fp);
 		goto error;
 	}
-	fclose(fp);

 	/* load reference file */
 	if (t->model[fid].reference != NULL) {
-		fp = fopen(opt->filelist[fid].reference, "r");
-		if (fp == NULL) {
-			ml_err("Failed to open reference file : %s\n",
-			       opt->filelist[fid].reference);
-			ret = -errno;
+		ret = ml_read_file(opt->filelist[fid].reference, &fsize, &buffer);
+		if (ret != 0)
 			goto error;
-		}

-		if (fread(t->model[fid].reference, 1, t->model[fid].out_dsize, fp) !=
-		    t->model[fid].out_dsize) {
-			ml_err("Failed to read reference file : %s\n",
-			       opt->filelist[fid].reference);
-			ret = -errno;
-			fclose(fp);
+		if (fsize == t->model[fid].out_dsize) {
+			rte_memcpy(t->model[fid].reference, buffer, fsize);
+			free(buffer);
+		} else {
+			ml_err("Invalid reference file, size = %zu (expected size = %" PRIu64 ")\n",
+			       fsize, t->model[fid].out_dsize);
+			ret = -EINVAL;
 			goto error;
 		}
-		fclose(fp);
 	}

 	/* create mempool for quantized input and output buffers. ml_request_initialize is
@@ -723,6 +707,8 @@ ml_inference_iomem_setup(struct ml_test *test, struct ml_options *opt, uint16_t
 		t->model[fid].io_pool = NULL;
 	}

+	free(buffer);
+
 	return ret;
 }

diff --git a/app/test-mldev/test_model_common.c b/app/test-mldev/test_model_common.c
index c28e452f29..8dbb0ff89f 100644
--- a/app/test-mldev/test_model_common.c
+++ b/app/test-mldev/test_model_common.c
@@ -14,11 +14,11 @@
 int
 ml_model_load(struct ml_test *test, struct ml_options *opt, struct ml_model *model, uint16_t fid)
 {
-	struct test_common *t = ml_test_priv(test);
 	struct rte_ml_model_params model_params;
-	FILE *fp;
 	int ret;

+	RTE_SET_USED(test);
+
 	if (model->state == MODEL_LOADED)
 		return 0;

@@ -26,43 +26,22 @@ ml_model_load(struct ml_test *test, struct ml_options *opt, struct ml_model *mod
 		return -EINVAL;

 	/* read model binary */
-	fp = fopen(opt->filelist[fid].model, "r");
-	if (fp == NULL) {
-		ml_err("Failed to open model file : %s\n", opt->filelist[fid].model);
-		return -1;
-	}
-
-	fseek(fp, 0, SEEK_END);
-	model_params.size = ftell(fp);
-	fseek(fp, 0, SEEK_SET);
-
-	model_params.addr = rte_malloc_socket("ml_model", model_params.size,
-					      t->dev_info.min_align_size, opt->socket_id);
-	if (model_params.addr == NULL) {
-		ml_err("Failed to allocate memory for model: %s\n", opt->filelist[fid].model);
-		fclose(fp);
-		return -ENOMEM;
-	}
-
-	if (fread(model_params.addr, 1, model_params.size, fp) != model_params.size) {
-		ml_err("Failed to read model file : %s\n", opt->filelist[fid].model);
-		rte_free(model_params.addr);
-		fclose(fp);
-		return -1;
-	}
-	fclose(fp);
+	ret = ml_read_file(opt->filelist[fid].model, &model_params.size,
+			   (char **)&model_params.addr);
+	if (ret != 0)
+		return ret;

 	/* load model to device */
 	ret = rte_ml_model_load(opt->dev_id, &model_params, &model->id);
 	if (ret != 0) {
 		ml_err("Failed to load model : %s\n", opt->filelist[fid].model);
 		model->state = MODEL_ERROR;
-		rte_free(model_params.addr);
+		free(model_params.addr);
 		return ret;
 	}

-	/* release mz */
-	rte_free(model_params.addr);
+	/* release buffer */
+	free(model_params.addr);

 	/* get model info */
 	ret = rte_ml_model_info_get(opt->dev_id, model->id, &model->info);
--
2.17.1

Re: [PATCH v3] app/mldev: add internal function for file read

[Stephen Hemminger]

On Wed, 3 May 2023 01:56:41 -0700
Srikanth Yalavarthi <syalavarthi@marvell.com> wrote:

> Added internal function to read model, input and reference
> files with required error checks. This change fixes the
> unchecked return value and improper use of negative value
> issues reported by coverity scan for file read operations.
> 
> Coverity issue: 383742, 383743
> Fixes: f6661e6d9a3a ("app/mldev: validate model operations")
> Fixes: da6793390596 ("app/mldev: support inference validation")
> 
> Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
> ---
> v3:
> * Fix incorrect use of rte_free with free
> 
> v2:
> * Replace rte_malloc in ml_read_file with malloc
> 
> v1:
> * Initial patch
> 
>  app/test-mldev/test_common.c           | 59 ++++++++++++++++++++++++++
>  app/test-mldev/test_common.h           |  2 +
>  app/test-mldev/test_inference_common.c | 54 +++++++++--------------
>  app/test-mldev/test_model_common.c     | 39 ++++-------------
>  4 files changed, 90 insertions(+), 64 deletions(-)
> 
> diff --git a/app/test-mldev/test_common.c b/app/test-mldev/test_common.c
> index 016b31c6ba..d8a8e8a448 100644
> --- a/app/test-mldev/test_common.c
> +++ b/app/test-mldev/test_common.c
> @@ -5,12 +5,71 @@
>  #include <errno.h>
> 
>  #include <rte_common.h>
> +#include <rte_malloc.h>
>  #include <rte_memory.h>
>  #include <rte_mldev.h>
> 
>  #include "ml_common.h"
>  #include "test_common.h"
> 
> +int
> +ml_read_file(char *file, size_t *size, char **buffer)
> +{
> +	char *file_buffer = NULL;
> +	long file_size = 0;
> +	int ret = 0;
> +	FILE *fp;
> +
> +	fp = fopen(file, "r");
> +	if (fp == NULL) {
> +		ml_err("Failed to open file: %s\n", file);
> +		return -EIO;
> +	}
> +
> +	if (fseek(fp, 0, SEEK_END) == 0) {
> +		file_size = ftell(fp);
> +		if (file_size == -1) {
> +			ret = -EIO;
> +			goto error;
> +		}
> +
> +		file_buffer = malloc(file_size);
> +		if (file_buffer == NULL) {
> +			ml_err("Failed to allocate memory: %s\n", file);
> +			ret = -ENOMEM;
> +			goto error;
> +		}
> +
> +		if (fseek(fp, 0, SEEK_SET) != 0) {
> +			ret = -EIO;
> +			goto error;
> +		}
> +
> +		if (fread(file_buffer, sizeof(char), file_size, fp) != (unsigned long)file_size) {
> +			ml_err("Failed to read file : %s\n", file);
> +			ret = -EIO;
> +			goto error;
> +		}
> +		fclose(fp);
> +	} else {


Granted this is a test program. But why did you ignore my feedback that this
is the slowest way to read a file. Stdio requires extra buffering, use regular read() or
better yet mmap().

RE: [EXT] Re: [PATCH v3] app/mldev: add internal function for file read

[Srikanth Yalavarthi]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: 03 May 2023 20:24
> To: Srikanth Yalavarthi <syalavarthi@marvell.com>
> Cc: Anup Prabhu <aprabhu@marvell.com>; dev@dpdk.org; Shivah Shankar
> Shankar Narayan Rao <sshankarnara@marvell.com>; Prince Takkar
> <ptakkar@marvell.com>
> Subject: [EXT] Re: [PATCH v3] app/mldev: add internal function for file read
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Wed, 3 May 2023 01:56:41 -0700
> Srikanth Yalavarthi <syalavarthi@marvell.com> wrote:
> 
> > Added internal function to read model, input and reference files with
> > required error checks. This change fixes the unchecked return value
> > and improper use of negative value issues reported by coverity scan
> > for file read operations.
> >
> > Coverity issue: 383742, 383743
> > Fixes: f6661e6d9a3a ("app/mldev: validate model operations")
> > Fixes: da6793390596 ("app/mldev: support inference validation")
> >
> > Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
> > ---
> > v3:
> > * Fix incorrect use of rte_free with free
> >
> > v2:
> > * Replace rte_malloc in ml_read_file with malloc
> >
> > v1:
> > * Initial patch
> >
> >  app/test-mldev/test_common.c           | 59 ++++++++++++++++++++++++++
> >  app/test-mldev/test_common.h           |  2 +
> >  app/test-mldev/test_inference_common.c | 54 +++++++++--------------
> >  app/test-mldev/test_model_common.c     | 39 ++++-------------
> >  4 files changed, 90 insertions(+), 64 deletions(-)
> >
> > diff --git a/app/test-mldev/test_common.c
> > b/app/test-mldev/test_common.c index 016b31c6ba..d8a8e8a448 100644
> > --- a/app/test-mldev/test_common.c
> > +++ b/app/test-mldev/test_common.c
> > @@ -5,12 +5,71 @@
> >  #include <errno.h>
> >
> >  #include <rte_common.h>
> > +#include <rte_malloc.h>
> >  #include <rte_memory.h>
> >  #include <rte_mldev.h>
> >
> >  #include "ml_common.h"
> >  #include "test_common.h"
> >
> > +int
> > +ml_read_file(char *file, size_t *size, char **buffer) {
> > +	char *file_buffer = NULL;
> > +	long file_size = 0;
> > +	int ret = 0;
> > +	FILE *fp;
> > +
> > +	fp = fopen(file, "r");
> > +	if (fp == NULL) {
> > +		ml_err("Failed to open file: %s\n", file);
> > +		return -EIO;
> > +	}
> > +
> > +	if (fseek(fp, 0, SEEK_END) == 0) {
> > +		file_size = ftell(fp);
> > +		if (file_size == -1) {
> > +			ret = -EIO;
> > +			goto error;
> > +		}
> > +
> > +		file_buffer = malloc(file_size);
> > +		if (file_buffer == NULL) {
> > +			ml_err("Failed to allocate memory: %s\n", file);
> > +			ret = -ENOMEM;
> > +			goto error;
> > +		}
> > +
> > +		if (fseek(fp, 0, SEEK_SET) != 0) {
> > +			ret = -EIO;
> > +			goto error;
> > +		}
> > +
> > +		if (fread(file_buffer, sizeof(char), file_size, fp) != (unsigned
> long)file_size) {
> > +			ml_err("Failed to read file : %s\n", file);
> > +			ret = -EIO;
> > +			goto error;
> > +		}
> > +		fclose(fp);
> > +	} else {
> 
> 
> Granted this is a test program. But why did you ignore my feedback that this
> is the slowest way to read a file. Stdio requires extra buffering, use regular
> read() or better yet mmap().

Agree on the improvement, but, considering that this is a test code and these operations are done in slow-path, I would prefer to have the implementation based on C library calls rather than using system calls.

Also, using system calls may not make this code portable? Though we are not supporting this app on platforms other than Linux, as of now.
Pls let me know what you think.

I had shared my additional comments on v2 patch.

Re: [EXT] Re: [PATCH v3] app/mldev: add internal function for file read

[Stephen Hemminger]

On Wed, 3 May 2023 14:59:40 +0000
Srikanth Yalavarthi <syalavarthi@marvell.com> wrote:

> > 
> > Granted this is a test program. But why did you ignore my feedback that this
> > is the slowest way to read a file. Stdio requires extra buffering, use regular
> > read() or better yet mmap().  
> 
> Agree on the improvement, but, considering that this is a test code and these operations are done in slow-path, I would prefer to have the implementation based on C library calls rather than using system calls.
> 
> Also, using system calls may not make this code portable? Though we are not supporting this app on platforms other than Linux, as of now.
> Pls let me know what you think.
> 
> I had shared my additional comments on v2 patch.

Using system calls read/write is used lots of places in DPDK already and is portable
to all the supported platforms.

Re: [EXT] Re: [PATCH v3] app/mldev: add internal function for file read

[Tyler Retzlaff]

On Wed, May 03, 2023 at 11:28:26AM -0700, Stephen Hemminger wrote:
> On Wed, 3 May 2023 14:59:40 +0000
> Srikanth Yalavarthi <syalavarthi@marvell.com> wrote:
> 
> > > 
> > > Granted this is a test program. But why did you ignore my feedback that this
> > > is the slowest way to read a file. Stdio requires extra buffering, use regular
> > > read() or better yet mmap().  
> > 
> > Agree on the improvement, but, considering that this is a test code and these operations are done in slow-path, I would prefer to have the implementation based on C library calls rather than using system calls.
> > 
> > Also, using system calls may not make this code portable? Though we are not supporting this app on platforms other than Linux, as of now.
> > Pls let me know what you think.
> > 
> > I had shared my additional comments on v2 patch.
> 
> Using system calls read/write is used lots of places in DPDK already and is portable
> to all the supported platforms.

well almost, the windows standard c library implements a subset of
POSIX.1 (ISO/IEC 9945-1:1996) and there should be a strong emphasis on
`a subset' as in it is not fully conformant to any specific POSIX standard.

also because they aren't technically part of the standard C library
(again POSIX is not standard C) they are exposed with different names on
windows by prepending a leading `_' to the names. so you get `_read' instead
of `read' for example.

you can force exposure of the non-conforming names (i.e. the POSIX
names) with the _CRT_DECLARE_NONSTDC_NAMES define but if you do and you use
them you may then get deprecation warnings.

anyway, i read above nobody cares if this code ever runs on anything but
Linux ~forever so i won't make it my business to comment further unless
there is a desire to include windows.

[PATCH v4] app/mldev: add internal function for file read

[Srikanth Yalavarthi]

Added internal function to read model, input and reference
files with required error checks. This change fixes the
unchecked return value and improper use of negative value
issues reported by coverity scan for file read operations.

Coverity issue: 383742, 383743
Fixes: f6661e6d9a3a ("app/mldev: validate model operations")
Fixes: da6793390596 ("app/mldev: support inference validation")

Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
---
v4:
* Drop use of fread and replace with systme calls

v3:
* Fix incorrect use of rte_free with free

v2:
* Replace rte_malloc in ml_read_file with malloc

 app/test-mldev/test_common.c           | 57 ++++++++++++++++++++++++++
 app/test-mldev/test_common.h           |  2 +
 app/test-mldev/test_inference_common.c | 56 ++++++++++---------------
 app/test-mldev/test_model_common.c     | 39 ++++--------------
 4 files changed, 90 insertions(+), 64 deletions(-)

diff --git a/app/test-mldev/test_common.c b/app/test-mldev/test_common.c
index 016b31c6ba..2449de0da9 100644
--- a/app/test-mldev/test_common.c
+++ b/app/test-mldev/test_common.c
@@ -5,12 +5,69 @@
 #include <errno.h>
 
 #include <rte_common.h>
+#include <rte_malloc.h>
 #include <rte_memory.h>
 #include <rte_mldev.h>
 
 #include "ml_common.h"
 #include "test_common.h"
 
+#include <fcntl.h>
+#include <sys/mman.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+int
+ml_read_file(char *file, size_t *size, char **buffer)
+{
+	char *file_buffer = NULL;
+	struct stat file_stat;
+	char *file_map;
+	int ret;
+	int fd;
+
+	fd = open(file, O_RDONLY);
+	if (fd == -1) {
+		ml_err("Failed to open file: %s\n", file);
+		return -errno;
+	}
+
+	if (fstat(fd, &file_stat) != 0) {
+		ml_err("fstat failed for file: %s\n", file);
+		return -errno;
+	}
+
+	file_buffer = malloc(file_stat.st_size);
+	if (file_buffer == NULL) {
+		ml_err("Failed to allocate memory: %s\n", file);
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	file_map = mmap(0, file_stat.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
+	if (file_map == MAP_FAILED) {
+		ml_err("Failed to map file: %s\n", file);
+		ret = -errno;
+		goto error;
+	}
+
+	rte_memcpy(file_buffer, file_map, file_stat.st_size);
+	munmap(file_map, file_stat.st_size);
+
+	*size = file_stat.st_size;
+	*buffer = file_buffer;
+
+	return 0;
+
+error:
+	if (file_buffer)
+		free(file_buffer);
+
+	close(fd);
+
+	return ret;
+}
+
 bool
 ml_test_cap_check(struct ml_options *opt)
 {
diff --git a/app/test-mldev/test_common.h b/app/test-mldev/test_common.h
index def108d5b2..793841a917 100644
--- a/app/test-mldev/test_common.h
+++ b/app/test-mldev/test_common.h
@@ -27,4 +27,6 @@ int ml_test_device_close(struct ml_test *test, struct ml_options *opt);
 int ml_test_device_start(struct ml_test *test, struct ml_options *opt);
 int ml_test_device_stop(struct ml_test *test, struct ml_options *opt);
 
+int ml_read_file(char *file, size_t *size, char **buffer);
+
 #endif /* TEST_COMMON_H */
diff --git a/app/test-mldev/test_inference_common.c b/app/test-mldev/test_inference_common.c
index 469ed35f6c..aa59066ff9 100644
--- a/app/test-mldev/test_inference_common.c
+++ b/app/test-mldev/test_inference_common.c
@@ -593,10 +593,10 @@ ml_inference_iomem_setup(struct ml_test *test, struct ml_options *opt, uint16_t
 	char mp_name[RTE_MEMPOOL_NAMESIZE];
 	const struct rte_memzone *mz;
 	uint64_t nb_buffers;
+	char *buffer = NULL;
 	uint32_t buff_size;
 	uint32_t mz_size;
-	uint32_t fsize;
-	FILE *fp;
+	size_t fsize;
 	int ret;
 
 	/* get input buffer size */
@@ -636,51 +636,36 @@ ml_inference_iomem_setup(struct ml_test *test, struct ml_options *opt, uint16_t
 		t->model[fid].reference = NULL;
 
 	/* load input file */
-	fp = fopen(opt->filelist[fid].input, "r");
-	if (fp == NULL) {
-		ml_err("Failed to open input file : %s\n", opt->filelist[fid].input);
-		ret = -errno;
+	ret = ml_read_file(opt->filelist[fid].input, &fsize, &buffer);
+	if (ret != 0)
 		goto error;
-	}
 
-	fseek(fp, 0, SEEK_END);
-	fsize = ftell(fp);
-	fseek(fp, 0, SEEK_SET);
-	if (fsize != t->model[fid].inp_dsize) {
-		ml_err("Invalid input file, size = %u (expected size = %" PRIu64 ")\n", fsize,
+	if (fsize == t->model[fid].inp_dsize) {
+		rte_memcpy(t->model[fid].input, buffer, fsize);
+		free(buffer);
+	} else {
+		ml_err("Invalid input file, size = %zu (expected size = %" PRIu64 ")\n", fsize,
 		       t->model[fid].inp_dsize);
 		ret = -EINVAL;
-		fclose(fp);
-		goto error;
-	}
-
-	if (fread(t->model[fid].input, 1, t->model[fid].inp_dsize, fp) != t->model[fid].inp_dsize) {
-		ml_err("Failed to read input file : %s\n", opt->filelist[fid].input);
-		ret = -errno;
-		fclose(fp);
 		goto error;
 	}
-	fclose(fp);
 
 	/* load reference file */
+	buffer = NULL;
 	if (t->model[fid].reference != NULL) {
-		fp = fopen(opt->filelist[fid].reference, "r");
-		if (fp == NULL) {
-			ml_err("Failed to open reference file : %s\n",
-			       opt->filelist[fid].reference);
-			ret = -errno;
+		ret = ml_read_file(opt->filelist[fid].reference, &fsize, &buffer);
+		if (ret != 0)
 			goto error;
-		}
 
-		if (fread(t->model[fid].reference, 1, t->model[fid].out_dsize, fp) !=
-		    t->model[fid].out_dsize) {
-			ml_err("Failed to read reference file : %s\n",
-			       opt->filelist[fid].reference);
-			ret = -errno;
-			fclose(fp);
+		if (fsize == t->model[fid].out_dsize) {
+			rte_memcpy(t->model[fid].reference, buffer, fsize);
+			free(buffer);
+		} else {
+			ml_err("Invalid reference file, size = %zu (expected size = %" PRIu64 ")\n",
+			       fsize, t->model[fid].out_dsize);
+			ret = -EINVAL;
 			goto error;
 		}
-		fclose(fp);
 	}
 
 	/* create mempool for quantized input and output buffers. ml_request_initialize is
@@ -712,6 +697,9 @@ ml_inference_iomem_setup(struct ml_test *test, struct ml_options *opt, uint16_t
 		t->model[fid].io_pool = NULL;
 	}
 
+	if (buffer)
+		free(buffer);
+
 	return ret;
 }
 
diff --git a/app/test-mldev/test_model_common.c b/app/test-mldev/test_model_common.c
index c28e452f29..8dbb0ff89f 100644
--- a/app/test-mldev/test_model_common.c
+++ b/app/test-mldev/test_model_common.c
@@ -14,11 +14,11 @@
 int
 ml_model_load(struct ml_test *test, struct ml_options *opt, struct ml_model *model, uint16_t fid)
 {
-	struct test_common *t = ml_test_priv(test);
 	struct rte_ml_model_params model_params;
-	FILE *fp;
 	int ret;
 
+	RTE_SET_USED(test);
+
 	if (model->state == MODEL_LOADED)
 		return 0;
 
@@ -26,43 +26,22 @@ ml_model_load(struct ml_test *test, struct ml_options *opt, struct ml_model *mod
 		return -EINVAL;
 
 	/* read model binary */
-	fp = fopen(opt->filelist[fid].model, "r");
-	if (fp == NULL) {
-		ml_err("Failed to open model file : %s\n", opt->filelist[fid].model);
-		return -1;
-	}
-
-	fseek(fp, 0, SEEK_END);
-	model_params.size = ftell(fp);
-	fseek(fp, 0, SEEK_SET);
-
-	model_params.addr = rte_malloc_socket("ml_model", model_params.size,
-					      t->dev_info.min_align_size, opt->socket_id);
-	if (model_params.addr == NULL) {
-		ml_err("Failed to allocate memory for model: %s\n", opt->filelist[fid].model);
-		fclose(fp);
-		return -ENOMEM;
-	}
-
-	if (fread(model_params.addr, 1, model_params.size, fp) != model_params.size) {
-		ml_err("Failed to read model file : %s\n", opt->filelist[fid].model);
-		rte_free(model_params.addr);
-		fclose(fp);
-		return -1;
-	}
-	fclose(fp);
+	ret = ml_read_file(opt->filelist[fid].model, &model_params.size,
+			   (char **)&model_params.addr);
+	if (ret != 0)
+		return ret;
 
 	/* load model to device */
 	ret = rte_ml_model_load(opt->dev_id, &model_params, &model->id);
 	if (ret != 0) {
 		ml_err("Failed to load model : %s\n", opt->filelist[fid].model);
 		model->state = MODEL_ERROR;
-		rte_free(model_params.addr);
+		free(model_params.addr);
 		return ret;
 	}
 
-	/* release mz */
-	rte_free(model_params.addr);
+	/* release buffer */
+	free(model_params.addr);
 
 	/* get model info */
 	ret = rte_ml_model_info_get(opt->dev_id, model->id, &model->info);
-- 
2.17.1

Re: [PATCH v4] app/mldev: add internal function for file read

[Stephen Hemminger]

On Wed, 7 Jun 2023 04:35:56 -0700
Srikanth Yalavarthi <syalavarthi@marvell.com> wrote:

> +	if (file_buffer)
> +		free(file_buffer);

Unnecessary if() since free(NULL) is ok.

[PATCH v5] app/mldev: add internal function for file read

[Srikanth Yalavarthi]

Added internal function to read model, input and reference
files with required error checks. This change fixes the
unchecked return value and improper use of negative value
issues reported by coverity scan for file read operations.

Coverity issue: 383742, 383743
Fixes: f6661e6d9a3a ("app/mldev: validate model operations")
Fixes: da6793390596 ("app/mldev: support inference validation")

Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
---
v5:
* Updated as per review comments

v4:
* Drop use of fread and replace with system calls

v3:
* Fix incorrect use of rte_free with free

v2:
* Replace rte_malloc in ml_read_file with malloc

 app/test-mldev/test_common.c           | 56 ++++++++++++++++++++++++++
 app/test-mldev/test_common.h           |  2 +
 app/test-mldev/test_inference_common.c | 56 ++++++++++----------------
 app/test-mldev/test_model_common.c     | 39 +++++-------------
 4 files changed, 89 insertions(+), 64 deletions(-)

diff --git a/app/test-mldev/test_common.c b/app/test-mldev/test_common.c
index 016b31c6ba..cc38e8af05 100644
--- a/app/test-mldev/test_common.c
+++ b/app/test-mldev/test_common.c
@@ -5,12 +5,68 @@
 #include <errno.h>
 
 #include <rte_common.h>
+#include <rte_malloc.h>
 #include <rte_memory.h>
 #include <rte_mldev.h>
 
 #include "ml_common.h"
 #include "test_common.h"
 
+#include <fcntl.h>
+#include <sys/mman.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+int
+ml_read_file(char *file, size_t *size, char **buffer)
+{
+	char *file_buffer = NULL;
+	struct stat file_stat;
+	char *file_map;
+	int ret;
+	int fd;
+
+	fd = open(file, O_RDONLY);
+	if (fd == -1) {
+		ml_err("Failed to open file: %s\n", file);
+		return -errno;
+	}
+
+	if (fstat(fd, &file_stat) != 0) {
+		ml_err("fstat failed for file: %s\n", file);
+		return -errno;
+	}
+
+	file_buffer = malloc(file_stat.st_size);
+	if (file_buffer == NULL) {
+		ml_err("Failed to allocate memory: %s\n", file);
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	file_map = mmap(0, file_stat.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
+	if (file_map == MAP_FAILED) {
+		ml_err("Failed to map file: %s\n", file);
+		ret = -errno;
+		goto error;
+	}
+
+	rte_memcpy(file_buffer, file_map, file_stat.st_size);
+	munmap(file_map, file_stat.st_size);
+
+	*size = file_stat.st_size;
+	*buffer = file_buffer;
+
+	return 0;
+
+error:
+	free(file_buffer);
+
+	close(fd);
+
+	return ret;
+}
+
 bool
 ml_test_cap_check(struct ml_options *opt)
 {
diff --git a/app/test-mldev/test_common.h b/app/test-mldev/test_common.h
index def108d5b2..793841a917 100644
--- a/app/test-mldev/test_common.h
+++ b/app/test-mldev/test_common.h
@@ -27,4 +27,6 @@ int ml_test_device_close(struct ml_test *test, struct ml_options *opt);
 int ml_test_device_start(struct ml_test *test, struct ml_options *opt);
 int ml_test_device_stop(struct ml_test *test, struct ml_options *opt);
 
+int ml_read_file(char *file, size_t *size, char **buffer);
+
 #endif /* TEST_COMMON_H */
diff --git a/app/test-mldev/test_inference_common.c b/app/test-mldev/test_inference_common.c
index d929ff6e61..8a6e4725df 100644
--- a/app/test-mldev/test_inference_common.c
+++ b/app/test-mldev/test_inference_common.c
@@ -599,10 +599,10 @@ ml_inference_iomem_setup(struct ml_test *test, struct ml_options *opt, uint16_t
 	char mp_name[RTE_MEMPOOL_NAMESIZE];
 	const struct rte_memzone *mz;
 	uint64_t nb_buffers;
+	char *buffer = NULL;
 	uint32_t buff_size;
 	uint32_t mz_size;
-	uint32_t fsize;
-	FILE *fp;
+	size_t fsize;
 	int ret;
 
 	/* get input buffer size */
@@ -642,51 +642,36 @@ ml_inference_iomem_setup(struct ml_test *test, struct ml_options *opt, uint16_t
 		t->model[fid].reference = NULL;
 
 	/* load input file */
-	fp = fopen(opt->filelist[fid].input, "r");
-	if (fp == NULL) {
-		ml_err("Failed to open input file : %s\n", opt->filelist[fid].input);
-		ret = -errno;
+	ret = ml_read_file(opt->filelist[fid].input, &fsize, &buffer);
+	if (ret != 0)
 		goto error;
-	}
 
-	fseek(fp, 0, SEEK_END);
-	fsize = ftell(fp);
-	fseek(fp, 0, SEEK_SET);
-	if (fsize != t->model[fid].inp_dsize) {
-		ml_err("Invalid input file, size = %u (expected size = %" PRIu64 ")\n", fsize,
+	if (fsize == t->model[fid].inp_dsize) {
+		rte_memcpy(t->model[fid].input, buffer, fsize);
+		free(buffer);
+	} else {
+		ml_err("Invalid input file, size = %zu (expected size = %" PRIu64 ")\n", fsize,
 		       t->model[fid].inp_dsize);
 		ret = -EINVAL;
-		fclose(fp);
-		goto error;
-	}
-
-	if (fread(t->model[fid].input, 1, t->model[fid].inp_dsize, fp) != t->model[fid].inp_dsize) {
-		ml_err("Failed to read input file : %s\n", opt->filelist[fid].input);
-		ret = -errno;
-		fclose(fp);
 		goto error;
 	}
-	fclose(fp);
 
 	/* load reference file */
+	buffer = NULL;
 	if (t->model[fid].reference != NULL) {
-		fp = fopen(opt->filelist[fid].reference, "r");
-		if (fp == NULL) {
-			ml_err("Failed to open reference file : %s\n",
-			       opt->filelist[fid].reference);
-			ret = -errno;
+		ret = ml_read_file(opt->filelist[fid].reference, &fsize, &buffer);
+		if (ret != 0)
 			goto error;
-		}
 
-		if (fread(t->model[fid].reference, 1, t->model[fid].out_dsize, fp) !=
-		    t->model[fid].out_dsize) {
-			ml_err("Failed to read reference file : %s\n",
-			       opt->filelist[fid].reference);
-			ret = -errno;
-			fclose(fp);
+		if (fsize == t->model[fid].out_dsize) {
+			rte_memcpy(t->model[fid].reference, buffer, fsize);
+			free(buffer);
+		} else {
+			ml_err("Invalid reference file, size = %zu (expected size = %" PRIu64 ")\n",
+			       fsize, t->model[fid].out_dsize);
+			ret = -EINVAL;
 			goto error;
 		}
-		fclose(fp);
 	}
 
 	/* create mempool for quantized input and output buffers. ml_request_initialize is
@@ -718,6 +703,9 @@ ml_inference_iomem_setup(struct ml_test *test, struct ml_options *opt, uint16_t
 		t->model[fid].io_pool = NULL;
 	}
 
+	if (buffer)
+		free(buffer);
+
 	return ret;
 }
 
diff --git a/app/test-mldev/test_model_common.c b/app/test-mldev/test_model_common.c
index c28e452f29..8dbb0ff89f 100644
--- a/app/test-mldev/test_model_common.c
+++ b/app/test-mldev/test_model_common.c
@@ -14,11 +14,11 @@
 int
 ml_model_load(struct ml_test *test, struct ml_options *opt, struct ml_model *model, uint16_t fid)
 {
-	struct test_common *t = ml_test_priv(test);
 	struct rte_ml_model_params model_params;
-	FILE *fp;
 	int ret;
 
+	RTE_SET_USED(test);
+
 	if (model->state == MODEL_LOADED)
 		return 0;
 
@@ -26,43 +26,22 @@ ml_model_load(struct ml_test *test, struct ml_options *opt, struct ml_model *mod
 		return -EINVAL;
 
 	/* read model binary */
-	fp = fopen(opt->filelist[fid].model, "r");
-	if (fp == NULL) {
-		ml_err("Failed to open model file : %s\n", opt->filelist[fid].model);
-		return -1;
-	}
-
-	fseek(fp, 0, SEEK_END);
-	model_params.size = ftell(fp);
-	fseek(fp, 0, SEEK_SET);
-
-	model_params.addr = rte_malloc_socket("ml_model", model_params.size,
-					      t->dev_info.min_align_size, opt->socket_id);
-	if (model_params.addr == NULL) {
-		ml_err("Failed to allocate memory for model: %s\n", opt->filelist[fid].model);
-		fclose(fp);
-		return -ENOMEM;
-	}
-
-	if (fread(model_params.addr, 1, model_params.size, fp) != model_params.size) {
-		ml_err("Failed to read model file : %s\n", opt->filelist[fid].model);
-		rte_free(model_params.addr);
-		fclose(fp);
-		return -1;
-	}
-	fclose(fp);
+	ret = ml_read_file(opt->filelist[fid].model, &model_params.size,
+			   (char **)&model_params.addr);
+	if (ret != 0)
+		return ret;
 
 	/* load model to device */
 	ret = rte_ml_model_load(opt->dev_id, &model_params, &model->id);
 	if (ret != 0) {
 		ml_err("Failed to load model : %s\n", opt->filelist[fid].model);
 		model->state = MODEL_ERROR;
-		rte_free(model_params.addr);
+		free(model_params.addr);
 		return ret;
 	}
 
-	/* release mz */
-	rte_free(model_params.addr);
+	/* release buffer */
+	free(model_params.addr);
 
 	/* get model info */
 	ret = rte_ml_model_info_get(opt->dev_id, model->id, &model->info);
-- 
2.17.1

RE: [EXT] Re: [PATCH v4] app/mldev: add internal function for file read

[Srikanth Yalavarthi]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: 07 June 2023 20:32
> To: Srikanth Yalavarthi <syalavarthi@marvell.com>
> Cc: Anup Prabhu <aprabhu@marvell.com>; dev@dpdk.org; Shivah Shankar
> Shankar Narayan Rao <sshankarnara@marvell.com>; Prince Takkar
> <ptakkar@marvell.com>
> Subject: [EXT] Re: [PATCH v4] app/mldev: add internal function for file read
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Wed, 7 Jun 2023 04:35:56 -0700
> Srikanth Yalavarthi <syalavarthi@marvell.com> wrote:
> 
> > +	if (file_buffer)
> > +		free(file_buffer);
> 
> Unnecessary if() since free(NULL) is ok.
Dropped if(). Updated in Version 5.

Re: [PATCH v5] app/mldev: add internal function for file read

[Stephen Hemminger]

On Wed, 7 Jun 2023 09:20:30 -0700
Srikanth Yalavarthi <syalavarthi@marvell.com> wrote:
The normal case leaks the open file descriptor

+
+	file_map = mmap(0, file_stat.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
+	if (file_map == MAP_FAILED) {
+		ml_err("Failed to map file: %s\n", file);
+		ret = -errno;
+		goto error;
+	}
+
+	rte_memcpy(file_buffer, file_map, file_stat.st_size);
+	munmap(file_map, file_stat.st_size);
+
+	*size = file_stat.st_size;
+	*buffer = file_buffer;
+
+	return 0;

Missing close()

> +	if (buffer)
> +		free(buffer);
> +

Another needless if()

[PATCH v6] app/mldev: add internal function for file read

[Srikanth Yalavarthi]

Added internal function to read model, input and reference
files with required error checks. This change fixes the
unchecked return value and improper use of negative value
issues reported by coverity scan for file read operations.

Coverity issue: 383742, 383743
Fixes: f6661e6d9a3a ("app/mldev: validate model operations")
Fixes: da6793390596 ("app/mldev: support inference validation")

Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
---
v6:
* Addressed additional review comments

v5:
* Updated as per review comments

v4:
* Drop use of fread and replace with system calls

v3:
* Fix incorrect use of rte_free with free

v2:
* Replace rte_malloc in ml_read_file with malloc

 app/test-mldev/test_common.c           | 56 ++++++++++++++++++++++++++
 app/test-mldev/test_common.h           |  2 +
 app/test-mldev/test_inference_common.c | 55 ++++++++++---------------
 app/test-mldev/test_model_common.c     | 39 +++++-------------
 4 files changed, 88 insertions(+), 64 deletions(-)

diff --git a/app/test-mldev/test_common.c b/app/test-mldev/test_common.c
index 016b31c6ba..891e8d0ca8 100644
--- a/app/test-mldev/test_common.c
+++ b/app/test-mldev/test_common.c
@@ -5,12 +5,68 @@
 #include <errno.h>
 
 #include <rte_common.h>
+#include <rte_malloc.h>
 #include <rte_memory.h>
 #include <rte_mldev.h>
 
 #include "ml_common.h"
 #include "test_common.h"
 
+#include <fcntl.h>
+#include <sys/mman.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+int
+ml_read_file(char *file, size_t *size, char **buffer)
+{
+	char *file_buffer = NULL;
+	struct stat file_stat;
+	char *file_map;
+	int ret;
+	int fd;
+
+	fd = open(file, O_RDONLY);
+	if (fd == -1) {
+		ml_err("Failed to open file: %s\n", file);
+		return -errno;
+	}
+
+	if (fstat(fd, &file_stat) != 0) {
+		ml_err("fstat failed for file: %s\n", file);
+		return -errno;
+	}
+
+	file_buffer = malloc(file_stat.st_size);
+	if (file_buffer == NULL) {
+		ml_err("Failed to allocate memory: %s\n", file);
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	file_map = mmap(0, file_stat.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
+	if (file_map == MAP_FAILED) {
+		ml_err("Failed to map file: %s\n", file);
+		ret = -errno;
+		goto error;
+	}
+
+	rte_memcpy(file_buffer, file_map, file_stat.st_size);
+	munmap(file_map, file_stat.st_size);
+	close(fd);
+
+	*size = file_stat.st_size;
+	*buffer = file_buffer;
+
+	return 0;
+
+error:
+	free(file_buffer);
+	close(fd);
+
+	return ret;
+}
+
 bool
 ml_test_cap_check(struct ml_options *opt)
 {
diff --git a/app/test-mldev/test_common.h b/app/test-mldev/test_common.h
index def108d5b2..793841a917 100644
--- a/app/test-mldev/test_common.h
+++ b/app/test-mldev/test_common.h
@@ -27,4 +27,6 @@ int ml_test_device_close(struct ml_test *test, struct ml_options *opt);
 int ml_test_device_start(struct ml_test *test, struct ml_options *opt);
 int ml_test_device_stop(struct ml_test *test, struct ml_options *opt);
 
+int ml_read_file(char *file, size_t *size, char **buffer);
+
 #endif /* TEST_COMMON_H */
diff --git a/app/test-mldev/test_inference_common.c b/app/test-mldev/test_inference_common.c
index d929ff6e61..231ef2defa 100644
--- a/app/test-mldev/test_inference_common.c
+++ b/app/test-mldev/test_inference_common.c
@@ -599,10 +599,10 @@ ml_inference_iomem_setup(struct ml_test *test, struct ml_options *opt, uint16_t
 	char mp_name[RTE_MEMPOOL_NAMESIZE];
 	const struct rte_memzone *mz;
 	uint64_t nb_buffers;
+	char *buffer = NULL;
 	uint32_t buff_size;
 	uint32_t mz_size;
-	uint32_t fsize;
-	FILE *fp;
+	size_t fsize;
 	int ret;
 
 	/* get input buffer size */
@@ -642,51 +642,36 @@ ml_inference_iomem_setup(struct ml_test *test, struct ml_options *opt, uint16_t
 		t->model[fid].reference = NULL;
 
 	/* load input file */
-	fp = fopen(opt->filelist[fid].input, "r");
-	if (fp == NULL) {
-		ml_err("Failed to open input file : %s\n", opt->filelist[fid].input);
-		ret = -errno;
+	ret = ml_read_file(opt->filelist[fid].input, &fsize, &buffer);
+	if (ret != 0)
 		goto error;
-	}
 
-	fseek(fp, 0, SEEK_END);
-	fsize = ftell(fp);
-	fseek(fp, 0, SEEK_SET);
-	if (fsize != t->model[fid].inp_dsize) {
-		ml_err("Invalid input file, size = %u (expected size = %" PRIu64 ")\n", fsize,
+	if (fsize == t->model[fid].inp_dsize) {
+		rte_memcpy(t->model[fid].input, buffer, fsize);
+		free(buffer);
+	} else {
+		ml_err("Invalid input file, size = %zu (expected size = %" PRIu64 ")\n", fsize,
 		       t->model[fid].inp_dsize);
 		ret = -EINVAL;
-		fclose(fp);
-		goto error;
-	}
-
-	if (fread(t->model[fid].input, 1, t->model[fid].inp_dsize, fp) != t->model[fid].inp_dsize) {
-		ml_err("Failed to read input file : %s\n", opt->filelist[fid].input);
-		ret = -errno;
-		fclose(fp);
 		goto error;
 	}
-	fclose(fp);
 
 	/* load reference file */
+	buffer = NULL;
 	if (t->model[fid].reference != NULL) {
-		fp = fopen(opt->filelist[fid].reference, "r");
-		if (fp == NULL) {
-			ml_err("Failed to open reference file : %s\n",
-			       opt->filelist[fid].reference);
-			ret = -errno;
+		ret = ml_read_file(opt->filelist[fid].reference, &fsize, &buffer);
+		if (ret != 0)
 			goto error;
-		}
 
-		if (fread(t->model[fid].reference, 1, t->model[fid].out_dsize, fp) !=
-		    t->model[fid].out_dsize) {
-			ml_err("Failed to read reference file : %s\n",
-			       opt->filelist[fid].reference);
-			ret = -errno;
-			fclose(fp);
+		if (fsize == t->model[fid].out_dsize) {
+			rte_memcpy(t->model[fid].reference, buffer, fsize);
+			free(buffer);
+		} else {
+			ml_err("Invalid reference file, size = %zu (expected size = %" PRIu64 ")\n",
+			       fsize, t->model[fid].out_dsize);
+			ret = -EINVAL;
 			goto error;
 		}
-		fclose(fp);
 	}
 
 	/* create mempool for quantized input and output buffers. ml_request_initialize is
@@ -718,6 +703,8 @@ ml_inference_iomem_setup(struct ml_test *test, struct ml_options *opt, uint16_t
 		t->model[fid].io_pool = NULL;
 	}
 
+	free(buffer);
+
 	return ret;
 }
 
diff --git a/app/test-mldev/test_model_common.c b/app/test-mldev/test_model_common.c
index c28e452f29..8dbb0ff89f 100644
--- a/app/test-mldev/test_model_common.c
+++ b/app/test-mldev/test_model_common.c
@@ -14,11 +14,11 @@
 int
 ml_model_load(struct ml_test *test, struct ml_options *opt, struct ml_model *model, uint16_t fid)
 {
-	struct test_common *t = ml_test_priv(test);
 	struct rte_ml_model_params model_params;
-	FILE *fp;
 	int ret;
 
+	RTE_SET_USED(test);
+
 	if (model->state == MODEL_LOADED)
 		return 0;
 
@@ -26,43 +26,22 @@ ml_model_load(struct ml_test *test, struct ml_options *opt, struct ml_model *mod
 		return -EINVAL;
 
 	/* read model binary */
-	fp = fopen(opt->filelist[fid].model, "r");
-	if (fp == NULL) {
-		ml_err("Failed to open model file : %s\n", opt->filelist[fid].model);
-		return -1;
-	}
-
-	fseek(fp, 0, SEEK_END);
-	model_params.size = ftell(fp);
-	fseek(fp, 0, SEEK_SET);
-
-	model_params.addr = rte_malloc_socket("ml_model", model_params.size,
-					      t->dev_info.min_align_size, opt->socket_id);
-	if (model_params.addr == NULL) {
-		ml_err("Failed to allocate memory for model: %s\n", opt->filelist[fid].model);
-		fclose(fp);
-		return -ENOMEM;
-	}
-
-	if (fread(model_params.addr, 1, model_params.size, fp) != model_params.size) {
-		ml_err("Failed to read model file : %s\n", opt->filelist[fid].model);
-		rte_free(model_params.addr);
-		fclose(fp);
-		return -1;
-	}
-	fclose(fp);
+	ret = ml_read_file(opt->filelist[fid].model, &model_params.size,
+			   (char **)&model_params.addr);
+	if (ret != 0)
+		return ret;
 
 	/* load model to device */
 	ret = rte_ml_model_load(opt->dev_id, &model_params, &model->id);
 	if (ret != 0) {
 		ml_err("Failed to load model : %s\n", opt->filelist[fid].model);
 		model->state = MODEL_ERROR;
-		rte_free(model_params.addr);
+		free(model_params.addr);
 		return ret;
 	}
 
-	/* release mz */
-	rte_free(model_params.addr);
+	/* release buffer */
+	free(model_params.addr);
 
 	/* get model info */
 	ret = rte_ml_model_info_get(opt->dev_id, model->id, &model->info);
-- 
2.17.1

RE: [EXT] Re: [PATCH v5] app/mldev: add internal function for file read

[Srikanth Yalavarthi]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: 07 June 2023 22:19
> To: Srikanth Yalavarthi <syalavarthi@marvell.com>
> Cc: Anup Prabhu <aprabhu@marvell.com>; dev@dpdk.org; Shivah Shankar
> Shankar Narayan Rao <sshankarnara@marvell.com>; Prince Takkar
> <ptakkar@marvell.com>
> Subject: [EXT] Re: [PATCH v5] app/mldev: add internal function for file read
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Wed, 7 Jun 2023 09:20:30 -0700
> Srikanth Yalavarthi <syalavarthi@marvell.com> wrote:
> The normal case leaks the open file descriptor
> 
> +
> +	file_map = mmap(0, file_stat.st_size, PROT_READ, MAP_PRIVATE, fd,
> 0);
> +	if (file_map == MAP_FAILED) {
> +		ml_err("Failed to map file: %s\n", file);
> +		ret = -errno;
> +		goto error;
> +	}
> +
> +	rte_memcpy(file_buffer, file_map, file_stat.st_size);
> +	munmap(file_map, file_stat.st_size);
> +
> +	*size = file_stat.st_size;
> +	*buffer = file_buffer;
> +
> +	return 0;
> 
> Missing close()
> 
> > +	if (buffer)
> > +		free(buffer);
> > +
> 
> Another needless if()

Updated in v6.

Re: [PATCH v6] app/mldev: add internal function for file read

[Thomas Monjalon]

07/06/2023 19:24, Srikanth Yalavarthi:
> Added internal function to read model, input and reference
> files with required error checks. This change fixes the
> unchecked return value and improper use of negative value
> issues reported by coverity scan for file read operations.
> 
> Coverity issue: 383742, 383743
> Fixes: f6661e6d9a3a ("app/mldev: validate model operations")
> Fixes: da6793390596 ("app/mldev: support inference validation")
> 
> Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>

Applied, thanks.