[dpdk-dev] [PATCH v4 04/10] lib: introduce ipsec library

[Ananyev, Konstantin]

> >
> >
> > On 12/20/2018 7:36 PM, Ananyev, Konstantin wrote:
> > >
> > >>> diff --git a/lib/librte_ipsec/sa.c b/lib/librte_ipsec/sa.c
> > >>> new file mode 100644
> > >>> index 000000000..f927a82bf
> > >>> --- /dev/null
> > >>> +++ b/lib/librte_ipsec/sa.c
> > >>> @@ -0,0 +1,327 @@
> > >>> +/* SPDX-License-Identifier: BSD-3-Clause
> > >>> + * Copyright(c) 2018 Intel Corporation
> > >>> + */
> > >>> +
> > >>> +#include <rte_ipsec_sa.h>
> > >>> +#include <rte_esp.h>
> > >>> +#include <rte_ip.h>
> > >>> +#include <rte_errno.h>
> > >>> +
> > >>> +#include "sa.h"
> > >>> +#include "ipsec_sqn.h"
> > >>> +
> > >>> +/* some helper structures */
> > >>> +struct crypto_xform {
> > >>> +	struct rte_crypto_auth_xform *auth;
> > >>> +	struct rte_crypto_cipher_xform *cipher;
> > >>> +	struct rte_crypto_aead_xform *aead;
> > >>> +};
> > >> shouldn't this be union as aead cannot be with cipher and auth cases.
> > > That's used internally to collect/analyze xforms provided by prm->crypto_xform.
> >
> > >
> > >
> > >> extra line
> > >>> +
> > >>> +
> > >>> +static int
> > >>> +check_crypto_xform(struct crypto_xform *xform)
> > >>> +{
> > >>> +	uintptr_t p;
> > >>> +
> > >>> +	p = (uintptr_t)xform->auth | (uintptr_t)xform->cipher;
> > >> what is the intent of this?
> > > It is used below to check that if aead is present both cipher and auth
> > > are  not.
> > >
> > >>> +
> > >>> +	/* either aead or both auth and cipher should be not NULLs */
> > >>> +	if (xform->aead) {
> > >>> +		if (p)
> > >>> +			return -EINVAL;
> > >>> +	} else if (p == (uintptr_t)xform->auth) {
> > >>> +		return -EINVAL;
> > >>> +	}
> > >> This function does not look good. It will miss the case of cipher only
> > > Cipher only is not supported right now and  I am not aware about plans
> > > to support it in future.
> > > If someone would like to add cipher onl,then yes he/she probably would
> > > have to update this function.
> > I know that cipher_only is not supported and nobody will support it in
> > case of ipsec.
> > My point is if somebody gives only auth or only cipher xform, then this
> > function would not be able to detect that case and will not return error.
> 
> fill_crypto_xform() (the function below) will detect it and return an error:
> +		if (xf->type == RTE_CRYPTO_SYM_XFORM_AUTH) {
> +			if (xform->auth != NULL)
> +				return -EINVAL;


Please ignore the comment above - was thinking about different thing.
Yes extra check is needed for case when only cipher xform is provided.