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

[Srikanth Yalavarthi <syalavarthi@marvell.com>]

> -----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.