Re: [dpdk-dev] [PATCH] test/crypto: skip plain text compare for null cipher OOP

[Anoob Joseph <anoobj@marvell.com>]

Hi Pablo,

Thanks for the review. Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Sent: Wednesday, November 10, 2021 4:54 PM
> To: Anoob Joseph <anoobj@marvell.com>; Akhil Goyal
> <gakhil@marvell.com>; Doherty, Declan <declan.doherty@intel.com>;
> Zhang, Roy Fan <roy.fan.zhang@intel.com>
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Archana Muniganti
> <marchana@marvell.com>; Tejasree Kondoj <ktejasree@marvell.com>;
> Hemant Agrawal <hemant.agrawal@nxp.com>; Nicolau, Radu
> <radu.nicolau@intel.com>; Power, Ciara <ciara.power@intel.com>;
> Gagandeep Singh <g.singh@nxp.com>; dev@dpdk.org
> Subject: [EXT] RE: [dpdk-dev] [PATCH] test/crypto: skip plain text compare
> for null cipher OOP
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hi Anoob,
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Anoob Joseph
> > Sent: Monday, November 8, 2021 3:20 PM
> > To: Akhil Goyal <gakhil@marvell.com>; Doherty, Declan
> > <declan.doherty@intel.com>; Zhang, Roy Fan <roy.fan.zhang@intel.com>
> > Cc: Anoob Joseph <anoobj@marvell.com>; Jerin Jacob
> > <jerinj@marvell.com>; Archana Muniganti <marchana@marvell.com>;
> > Tejasree Kondoj <ktejasree@marvell.com>; Hemant Agrawal
> > <hemant.agrawal@nxp.com>; Nicolau, Radu <radu.nicolau@intel.com>;
> > Power, Ciara <ciara.power@intel.com>; Gagandeep Singh
> > <g.singh@nxp.com>; dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH] test/crypto: skip plain text compare for
> > null cipher OOP
> >
> > NULL cipher is used for validating auth only cases. With out of place
> > processing, validating plain text should not be done as the PMD is
> > only expected to update auth data.
> >
> > Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> > ---
> >  app/test/test_cryptodev.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
> > index e54a1a9..964f44f 100644
> > --- a/app/test/test_cryptodev.c
> > +++ b/app/test/test_cryptodev.c
> > @@ -7490,6 +7490,22 @@ test_mixed_auth_cipher(const struct
> > mixed_cipher_auth_test_data *tdata,
> >  				tdata->digest_enc.len);
> >  	}
> >
> > +	/*
> > +	 * NULL cipher is used for auth only cases where only authentication
> > +	 * is done. With verify operation, MAC would be validated by the
> PMD.
> > +	 * With generate operation, verify MAC generated by the PMD.
> > +	 */
> > +	if (op_mode == OUT_OF_PLACE &&
> > +	    tdata->cipher_algo == RTE_CRYPTO_CIPHER_NULL) {
> 
> Why only checking for OUT_OF_PLACE? As far as cipher algorithm is NULL,
> only digest should be checked.

[Anoob] Agreed. I will make this change in v2.
 
> Also, looking at the code, there is this same check, but with hardcoded tag
> length.
> Could you rearrange the code to have less lines and generic?

[Anoob] Yes. This looks better and the code would be self-explanatory as well. Will make this change in v2.

> 
> Something like:
> 
> <pseudo-code>
> if (!verify)
> 	check_digest
> 
> if (cipher_algo != NULL)
> 	check_ciphertext/plaintext
> 
> check op status
> </pseudo-code>
> 
> Also, I think this change is applicable in test_mixed_auth_cipher_sgl.

[Anoob] Yes. I'll make that change as well.

> 
> Thanks,
> Pablo