Re: [dpdk-dev] [PATCH 2/2] lib/security: add SA lifetime configuration

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

Hi Akhil,
 
> Hi Konstantin,
> > > There are two options that we considered,
> > > 1. Extend the enum, rte_crypto_op_status,  to cover warnings [1]
> > > 2. There are reserved fields in rte_cryto_op structure. So we can use bits in
> > them to indicate various cases. [2]
> > >
> > > Both the submitted patches follow approach 1 (following how it's done
> > currently), but we can switch to approach 2 if we think there can be
> > > more such "warnings" that can occur simultaneously. Can you share your
> > thoughts on how we should extend the library to handle such
> > > cases?
> > >
> > > [1] https://doc.dpdk.org/api/rte__crypto_8h.html#afe16508b77c2a8dc5caf74a4e9850171
> > > [2] https://doc.dpdk.org/api/rte__crypto_8h_source.html
> >
> > My vote would probably be for option #2 (use one of the reserved fields for
> > it).
> > That way - existing code wouldn't need to be changed.
> 
> Adding a single enum or multiple enums is the same thing. Right wrt code changes?
> However, if the check is something like
> If (status != RTE_CRYPTO_OP_STATUS_SUCCESS)
> 	Report appropriate error number
> App code will need to be updated to take care the warnings in both options.
> It will be something like
> Option #1
> If (status != RTE_CRYPTO_OP_STATUS_SUCCESS) {
> 	If (status < RTE_CRYPTO_OP_STATUS_SUCCESS)
> 		Report appropriate error number.
> 	Else
> 		Report appropriate warning number probably in debug prints.
> }
> Option #2
> If (op->status != RTE_CRYPTO_OP_STATUS_SUCCESS) {
> 	If (op->status == RTE_CRYPTO_OP_STATUS_WARNING) {
> 		Report appropriate warning based on op->reserved[0]
> 	} else {
> 		Report appropriate error number
> 	}
> }
> Here both the options are same wrt performance.
> But in option #2, driver and app need to fill and decode 2 separate variables
> As against 1 variable in option #1
> 
> In both the options, there will be similar code changes.
> Do you suspect any other code change?

Hmm, I think there is some sort of contradiction here.
From Anoob original mail:
"Both the above will be an IPsec operation completed successfully but with additional information
that PMD can pass on to application for indicating status of offloads."
So my understanding after reading Anoob mail was :
a) warnings will be set when crypto-op completed successfully, i.e:
     op->status == RTE_CRYPTO_OP_STATUS_SUCCESS
b) It is not mandatory for the application to process the warnings.
    Yes it is a recommended but still an optional.

Though from your mail it seems visa-versa:
Warnings are just some extra error codes (op->status != RTE_CRYPTO_OP_STATUS_SUCCESS)
and obviously each app have to handle them.

So could you tell me which approach did you mean?
If these 'warnings' are just new error codes and app is required to handle them,
then why do we need to introduce 'warnings' at all?
Lets treat them as error - add new  RTE_CRYPTO_OP_STATUS_ error codes for them
and that's would be it. 
 
If processing them is optional, then I think we better have a new field for them
So app code will look like:
if (op->status == RTE_CRYPTO_OP_STATUS_SUCCESS) {
    if (op->warning != 0) {
        /* handle warning conditions here */ 
    }
    /* do normal success processing */
}

In that case existing apps will be continue to work without any modifications.
Yes, they would just ignore these new warnings, but nothing will be broken.

> > Again these warnings, it probably needs to be a bit-flags, correct?
> 
> We can deal with both bit flags as well as new enums in the status.
> I believe both are same and in fact using enum in application is more convenient
> for user, instead of decoding bit flags.
> However, it is personal choice. People may differ on that.

From what I understand from previous mails: same op can have multiple warnings set.
Let say both SOFT_LIMIT can be reached and L4 checksum is not correct.
That's why I presumed that warnings have to be a bit-flag. 

Konstantin