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

Trahe, Fiona fiona.trahe at intel.com
Fri Apr 12 12:25:05 CEST 2019


Hi Ayuj

From: Ayuj Verma [mailto:ayverma at marvell.com]
Sent: Friday, April 12, 2019 8:08 AM
To: Trahe, Fiona <fiona.trahe at intel.com>; akhil.goyal at nxp.com; Kusztal, ArkadiuszX <arkadiuszx.kusztal at intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch at intel.com>
Cc: Shally Verma <shallyv at marvell.com>; Sunila Sahu <ssahu at marvell.com>; Kanaka Durga Kotamarthy <kkotamarthy at marvell.com>; Arvind Desai <adesai at marvell.com>; dev at dpdk.org
Subject: Re: [PATCH v2] app/test: replace TEST_SKIPPED with -ENOTSUP


Hi Fiona,



Please see inline.



Thanks and regards

Ayuj Verma

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

Hi Ayuj,

> -----Original Message-----
> From: Ayuj Verma [mailto:ayverma at marvell.com]
> Sent: Tuesday, April 9, 2019 12:34 PM
> To: akhil.goyal at nxp.com<mailto:akhil.goyal at nxp.com>; Trahe, Fiona <fiona.trahe at intel.com<mailto:fiona.trahe at intel.com>>; Kusztal, ArkadiuszX
> <arkadiuszx.kusztal at intel.com<mailto:arkadiuszx.kusztal at intel.com>>; De Lara Guarch, Pablo <pablo.de.lara.guarch at intel.com<mailto:pablo.de.lara.guarch at intel.com>>
> Cc: shallyv at marvell.com<mailto:shallyv at marvell.com>; ssahu at marvell.com<mailto:ssahu at marvell.com>; kkotamarthy at marvell.com<mailto:kkotamarthy at marvell.com>; adesai at marvell.com<mailto:adesai at marvell.com>;
> dev at dpdk.org<mailto:dev at dpdk.org>; Ayuj Verma <ayverma at marvell.com<mailto:ayverma at marvell.com>>
> Subject: [PATCH v2] app/test: replace TEST_SKIPPED with -ENOTSUP
>
> Return -ENOTSUP for unsupported tests
>
> Signed-off-by: Ayuj Verma <ayverma at marvell.com<mailto:ayverma at marvell.com>>
> Signed-off-by: Shally Verma <shallyv at marvell.com<mailto:shallyv at 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.

[Fiona] This is checking that the "modinv" string matches an element in the API enum. It's not checking anything to do with the device.

The device capability is retrieved by the following line:
capability = rte_cryptodev_asym_capability_get(dev_id,

                                                                    &cap_idx);

But actually is not checked. It should have a NULL check before moving on to the len check,

Else there can be a segfault in the len check function.

Can you add that NULL check please - and that should return -ENOTSUP.

Same with the modex test.


More information about the dev mailing list