[v3] app/mldev: add internal function for file read
Checks
Commit Message
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(-)
--
2.17.1
Comments
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().
> -----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.
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.
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.
@@ -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)
{
@@ -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 */
@@ -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;
}
@@ -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);