Re: [dpdk-dev] [PATCH v2] app/test: replace TEST_SKIPPED with -ENOTSUP

[Ayuj Verma <ayverma@marvell.com>]

Hi Fiona,


Please see inline.


Thanks and regards

Ayuj Verma


________________________________
From: Trahe, Fiona <fiona.trahe@intel.com>
Sent: 09 April 2019 20:47
To: Ayuj Verma; akhil.goyal@nxp.com; Kusztal, ArkadiuszX; De Lara Guarch, Pablo
Cc: Shally Verma; Sunila Sahu; Kanaka Durga Kotamarthy; Arvind Desai; dev@dpdk.org; Trahe, Fiona
Subject: RE: [PATCH v2] app/test: replace TEST_SKIPPED with -ENOTSUP

Hi Ayuj,

> -----Original Message-----
> From: Ayuj Verma [mailto:ayverma@marvell.com]
> Sent: Tuesday, April 9, 2019 12:34 PM
> To: akhil.goyal@nxp.com; Trahe, Fiona <fiona.trahe@intel.com>; Kusztal, ArkadiuszX
> <arkadiuszx.kusztal@intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Cc: shallyv@marvell.com; ssahu@marvell.com; kkotamarthy@marvell.com; adesai@marvell.com;
> dev@dpdk.org; Ayuj Verma <ayverma@marvell.com>
> Subject: [PATCH v2] app/test: replace TEST_SKIPPED with -ENOTSUP
>
> Return -ENOTSUP for unsupported tests
>
> Signed-off-by: Ayuj Verma <ayverma@marvell.com>
> Signed-off-by: Shally Verma <shallyv@marvell.com>
> ---
>  app/test/test_cryptodev_asym.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/app/test/test_cryptodev_asym.c b/app/test/test_cryptodev_asym.c
> index d2efce9..feed3a8 100644
> --- a/app/test/test_cryptodev_asym.c
> +++ b/app/test/test_cryptodev_asym.c
> @@ -352,7 +352,7 @@ struct test_cases_array {
>                RTE_LOG(INFO, USER1,
>                                "Device doesn't support sign op with "
>                                "exponent key type. Test Skipped\n");
> -             return TEST_SKIPPED;
> +             return -ENOTSUP;
>        }
>
>        sess = rte_cryptodev_asym_session_create(sess_mpool);
> @@ -498,7 +498,7 @@ struct test_cases_array {
>                RTE_LOG(INFO, USER1,
>                                "Device doesn't support sign op with "
>                                "exponent key type. Test Skipped\n");
> -             return TEST_SKIPPED;
> +             return -ENOTSUP;
>        }
>
>        sess = rte_cryptodev_asym_session_create(sess_mpool);
> @@ -1261,7 +1261,7 @@ static inline void print_asym_capa(
>                &modinv_xform.xform_type, "modinv") < 0) {
>                RTE_LOG(ERR, USER1,
>                                 "Invalid ASYNC algorithm specified\n");
> -             return -1;
> +             return -ENOTSUP;
[Fiona] this looks more like a test code bug rather than an indication that the device doesn't support modinv. SO should still return -1.
Also - while you're updating, can you please fix the typo in the trace - ASYNC should be ASYMM

[Ayuj]  Each test execute if device supports that algorithm else it is skipped. Thus, here it checks if modinv is not supported in capability then skip the test, which looks okay to me.

So, why do you say it is a bug?

Probably message is not proper should have been "Device doesn't support MODINV"

Will update typo.


>        }
>
>        cap_idx.type = modinv_xform.xform_type;
> @@ -1273,7 +1273,7 @@ static inline void print_asym_capa(
>                modinv_xform.modinv.modulus.length)) {
>                RTE_LOG(ERR, USER1,
>                                 "Invalid MODULOUS length specified\n");
> -                             return -1;
> +                             return -ENOTSUP;
[Fiona] please update the trace to match the return, e.g. something like "modulus length %len not supported by this device"
[Ayuj] Sure.
>                }
>
>        sess = rte_cryptodev_asym_session_create(sess_mpool);
> @@ -1380,7 +1380,7 @@ static inline void print_asym_capa(
>                < 0) {
>                RTE_LOG(ERR, USER1,
>                                "Invalid ASYNC algorithm specified\n");
> -             return -1;
> +             return -ENOTSUP;
>        }
[Fiona] same as above, i.e. code bug. And typo in trace.
>
>        /* check for modlen capability */
> @@ -1391,7 +1391,7 @@ static inline void print_asym_capa(
>                        capability, modex_xform.modex.modulus.length)) {
>                RTE_LOG(ERR, USER1,
>                                "Invalid MODULOUS length specified\n");
> -                             return -1;
> +                             return -ENOTSUP;
[Fiona] same as above. Fix trace.


>                }
>
>        /* generate crypto op data structure */
> --
> 1.8.3.1