[dpdk-dev] lib/cryptodev: fix API digest length comments

  Fix misleading comments clarifying setting of digest length.

Fixes: d11b0f30df88 ("cryptodev: introduce API and framework for crypto devices")

Cc: stable@dpdk.org
Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
---
 lib/librte_cryptodev/rte_crypto_sym.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
  

On 25/04/2017 5:56 PM, Trahe, Fiona wrote:
> Fix misleading comments clarifying setting of digest length.
>
> Fixes: d11b0f30df88 ("cryptodev: introduce API and framework for crypto devices")
>
> Cc: stable@dpdk.org
> Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
> ---
>  lib/librte_cryptodev/rte_crypto_sym.h | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/lib/librte_cryptodev/rte_crypto_sym.h b/lib/librte_cryptodev/rte_crypto_sym.h
> index eb7b530..12f1583 100644
> --- a/lib/librte_cryptodev/rte_crypto_sym.h
> +++ b/lib/librte_cryptodev/rte_crypto_sym.h
> @@ -310,11 +310,10 @@ struct rte_crypto_auth_xform {
>  	 * this specifies the length of the digest to be compared for the
>  	 * session.
>  	 *
> +	 * It is the caller's responsibility to ensure that the
> +	 * digest length is compliant with the hash algorithm being used.
>  	 * If the value is less than the maximum length allowed by the hash,
> -	 * the result shall be truncated.  If the value is greater than the
> -	 * maximum length allowed by the hash then an error will be generated
> -	 * by *rte_cryptodev_sym_session_create* or by the
> -	 * *rte_cryptodev_sym_enqueue_burst* if using session-less APIs.
> +	 * the result shall be truncated.
>  	 */

I don't think this comment change is valid, we already validate many of 
the parameters which are passed into session creation, such as key 
lengths etc, if we are not validating digest length now I think we 
should be, maybe this is a gap in our unit tests.

>
>  	uint32_t add_auth_data_length;
> @@ -597,7 +596,9 @@ struct rte_crypto_sym_op {
>  			phys_addr_t phys_addr;
>  			/**< Physical address of digest */
>  			uint16_t length;
> -			/**< Length of digest */
> +			/**< Length of digest. This must be the same value as
> +			 * @ref rte_crypto_auth_xform.digest_length.
> +			 */
>  		} digest; /**< Digest parameters */
>
>  		struct {
>
  

Hi Declan,

> -----Original Message-----
> From: Doherty, Declan
> Sent: Friday, April 28, 2017 10:22 AM
> To: Trahe, Fiona <fiona.trahe@intel.com>; dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> stable@dpdk.org
> Subject: Re: [PATCH] lib/cryptodev: fix API digest length comments
> 
> On 25/04/2017 5:56 PM, Trahe, Fiona wrote:
> > Fix misleading comments clarifying setting of digest length.
> >
> > Fixes: d11b0f30df88 ("cryptodev: introduce API and framework for crypto
> devices")
> >
> > Cc: stable@dpdk.org
> > Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
> > ---
> >  lib/librte_cryptodev/rte_crypto_sym.h | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/librte_cryptodev/rte_crypto_sym.h
> b/lib/librte_cryptodev/rte_crypto_sym.h
> > index eb7b530..12f1583 100644
> > --- a/lib/librte_cryptodev/rte_crypto_sym.h
> > +++ b/lib/librte_cryptodev/rte_crypto_sym.h
> > @@ -310,11 +310,10 @@ struct rte_crypto_auth_xform {
> >  	 * this specifies the length of the digest to be compared for the
> >  	 * session.
> >  	 *
> > +	 * It is the caller's responsibility to ensure that the
> > +	 * digest length is compliant with the hash algorithm being used.
> >  	 * If the value is less than the maximum length allowed by the hash,
> > -	 * the result shall be truncated.  If the value is greater than the
> > -	 * maximum length allowed by the hash then an error will be
> generated
> > -	 * by *rte_cryptodev_sym_session_create* or by the
> > -	 * *rte_cryptodev_sym_enqueue_burst* if using session-less APIs.
> > +	 * the result shall be truncated.
> >  	 */
> 
> I don't think this comment change is valid, we already validate many of
> the parameters which are passed into session creation, such as key
> lengths etc, if we are not validating digest length now I think we
> should be, maybe this is a gap in our unit tests.
> 
Neither the API nor any of the PMDs validate the digest_length at present.
I agree, they probably should, but it's a bit late to add this in 17.05,
as it would be quite a bit of code churn, each PMD would have to check 
against the range in their Capabilities structure.
So the next best thing for this release in my opinion is to remove the comment as
it is misleading and out of sync with the implementation.
In the next release we should remove the comments saying it's the callers 
responsibility from both digest_length and auth key length and add 
the param checks to each PMD.

> >
> >  	uint32_t add_auth_data_length;
> > @@ -597,7 +596,9 @@ struct rte_crypto_sym_op {
> >  			phys_addr_t phys_addr;
> >  			/**< Physical address of digest */
> >  			uint16_t length;
> > -			/**< Length of digest */
> > +			/**< Length of digest. This must be the same value as
> > +			 * @ref rte_crypto_auth_xform.digest_length.
> > +			 */
> >  		} digest; /**< Digest parameters */
> >
> >  		struct {
> >
  

On 28/04/2017 2:38 PM, Trahe, Fiona wrote:
> Hi Declan,
>
>> -----Original Message-----
...

>> I don't think this comment change is valid, we already validate many of
>> the parameters which are passed into session creation, such as key
>> lengths etc, if we are not validating digest length now I think we
>> should be, maybe this is a gap in our unit tests.
>>
> Neither the API nor any of the PMDs validate the digest_length at present.
> I agree, they probably should, but it's a bit late to add this in 17.05,
> as it would be quite a bit of code churn, each PMD would have to check
> against the range in their Capabilities structure.
> So the next best thing for this release in my opinion is to remove the comment as
> it is misleading and out of sync with the implementation.
> In the next release we should remove the comments saying it's the callers
> responsibility from both digest_length and auth key length and add
> the param checks to each PMD.

Oh, I guess it's better to make the comment reflect the current 
implementation and then fix in next release.

>
....
>
  

On 25/04/2017 5:56 PM, Trahe, Fiona wrote:
> Fix misleading comments clarifying setting of digest length.
>
> Fixes: d11b0f30df88 ("cryptodev: introduce API and framework for crypto devices")
>
> Cc: stable@dpdk.org
> Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
> ---
...
>

Acked-by: Declan Doherty <declan.doherty@intel.com>
  

> -----Original Message-----
> From: Doherty, Declan
> Sent: Friday, April 28, 2017 3:57 PM
> To: Trahe, Fiona; dev@dpdk.org
> Cc: De Lara Guarch, Pablo; stable@dpdk.org
> Subject: Re: [PATCH] lib/cryptodev: fix API digest length comments
> 
> On 25/04/2017 5:56 PM, Trahe, Fiona wrote:
> > Fix misleading comments clarifying setting of digest length.
> >
> > Fixes: d11b0f30df88 ("cryptodev: introduce API and framework for crypto
> devices")
> >
> > Cc: stable@dpdk.org
> > Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
> > ---
> ...
> >
> 
> Acked-by: Declan Doherty <declan.doherty@intel.com>

Applied to dpdk-next-crypto.
Thanks,

Pablo