Re: [dpdk-dev] [PATCH v2 2/6] app/test: add basic dmadev instance tests

[Bruce Richardson <bruce.richardson@intel.com>]

On Wed, Sep 01, 2021 at 09:24:12PM +0200, Mattias Rönnblom wrote:
> On 2021-09-01 18:32, Bruce Richardson wrote:
> > Run basic sanity tests for configuring, starting and stopping a dmadev
> > instance to help validate drivers. This also provides the framework for
> > future tests for data-path operation.
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >   app/test/test_dmadev.c | 81 ++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 81 insertions(+)
> > 
> > diff --git a/app/test/test_dmadev.c b/app/test/test_dmadev.c
> > index bb01e86483..12f7c69629 100644
> > --- a/app/test/test_dmadev.c
> > +++ b/app/test/test_dmadev.c
> > @@ -2,6 +2,7 @@
> >    * Copyright(c) 2021 HiSilicon Limited.
> >    * Copyright(c) 2021 Intel Corporation.
> >    */
> > +#include <inttypes.h>
> >   #include <rte_common.h>
> >   #include <rte_dev.h>
> > @@ -13,6 +14,77 @@
> >   /* from test_dmadev_api.c */
> >   extern int test_dmadev_api(uint16_t dev_id);
> > +#define PRINT_ERR(...) print_err(__func__, __LINE__, __VA_ARGS__)
> > +
> > +static inline int
> 
> Remove inline.
>
While I understand it's probably not doing a lot having "inline" there, any
particular reason why you think it should be removed?
 
> > +__rte_format_printf(3, 4)
> > +print_err(const char *func, int lineno, const char *format, ...)
> > +{
> > +	va_list ap;
> > +	int ret;
> > +
> > +	ret = fprintf(stderr, "In %s:%d - ", func, lineno);
> Check return code here, and return on error.
> 
> > +	va_start(ap, format);
> > +	ret += vfprintf(stderr, format, ap);
> 
> ..and here.
> 
> > +	va_end(ap);
> > +
> > +	return ret;
> 
> A negative return value in one call and an valid byte count result for the
> other should produce an error, but here it might not.
> 
> You might argue this is just test code, but then I suggest not checking the
> return values at all.
> 

Indeed the return value is never checked anywhere in the calls to PRINT_ERR
macro, and since the writes are going to stderr it's pretty low risk.
Therefore, I'll remove the return value handling completely as you suggest.

/Bruce