Re: [dpdk-dev] [PATCH v1] lib/crypto: fix alphabetical ordering of headers

[Anoob Joseph <anoobj@marvell.com>]

Hi Shally,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Shally Verma
> Sent: Monday, April 22, 2019 6:23 PM
> To: Anoob Joseph <anoobj@marvell.com>; Ayuj Verma
> <ayverma@marvell.com>; akhil.goyal@nxp.com;
> arkadiuszx.kusztal@intel.com; fiona.trahe@intel.com
> Cc: Sunila Sahu <ssahu@marvell.com>; Kanaka Durga Kotamarthy
> <kkotamarthy@marvell.com>; Arvind Desai <adesai@marvell.com>;
> dev@dpdk.org; Ayuj Verma <ayverma@marvell.com>
> Subject: RE: [dpdk-dev] [PATCH v1] lib/crypto: fix alphabetical ordering of
> headers
> 
> Hi Anoob
> 
> > -----Original Message-----
> > From: Anoob Joseph
> > Sent: Thursday, April 18, 2019 9:28 AM
> > To: Ayuj Verma <ayverma@marvell.com>; akhil.goyal@nxp.com;
> > arkadiuszx.kusztal@intel.com; fiona.trahe@intel.com
> > Cc: Shally Verma <shallyv@marvell.com>; Sunila Sahu
> > <ssahu@marvell.com>; Kanaka Durga Kotamarthy
> > <kkotamarthy@marvell.com>; Arvind Desai <adesai@marvell.com>;
> > dev@dpdk.org; Ayuj Verma <ayverma@marvell.com>
> > Subject: RE: [dpdk-dev] [PATCH v1] lib/crypto: fix alphabetical
> > ordering of headers
> >
> > Hi Ayuj, Akhil, Fiona, Shally,
> >
> > I think there are couple of issues with this patch. The real issue
> > here is the dependency of rte_crypto_asym.h on rte_crypto_sym.h. If
> > rte_crypto_asym.h doesn't include all the headers it uses, every file
> > which includes rte_crypto_asym.h will have to include
> > rte_crypto_asym.h's dependencies, which is not the right practice.
> > Keeping it alphabetical etc comes later.
> 
> [Shally] We can change title something to "include dependencies into asym
> header files" , but then should I assume that You agree to this patch?

[Anoob] Yes. The title and description has to be changed to better reflect what we are trying to solve. I'm in agreement with what the patch is trying to achieve.

> 
> >
> > So the patch has to be rephrased to better reflect the purpose.
> >
> > Also please see inline.
> >
> > Thanks,
> > Anoob
> >
> > > -----Original Message-----
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of Ayuj Verma
> > > Sent: Friday, April 12, 2019 12:33 PM
> > > To: akhil.goyal@nxp.com; arkadiuszx.kusztal@intel.com;
> > > fiona.trahe@intel.com
> > > Cc: Shally Verma <shallyv@marvell.com>; Sunila Sahu
> > > <ssahu@marvell.com>; Kanaka Durga Kotamarthy
> > > <kkotamarthy@marvell.com>; Arvind Desai <adesai@marvell.com>;
> > > dev@dpdk.org; Ayuj Verma <ayverma@marvell.com>
> > > Subject: [dpdk-dev] [PATCH v1] lib/crypto: fix alphabetical ordering
> > > of headers
> > >
> > > include rte_crypto_sym.h in rte_crypto_asym.h in place of including
> > > it in rte_crypto.h.
> > >
> > > Signed-off-by: Ayuj Verma <ayverma@marvell.com>
> > > Signed-off-by: Shally Verma <shallyv@marvell.com>
> > >
> > > ---
> > >  lib/librte_cryptodev/rte_crypto.h      | 1 -
> > >  lib/librte_cryptodev/rte_crypto_asym.h | 2 ++
> > >  2 files changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/lib/librte_cryptodev/rte_crypto.h
> > > b/lib/librte_cryptodev/rte_crypto.h
> > > index fd5ef3a..17dccdf 100644
> > > --- a/lib/librte_cryptodev/rte_crypto.h
> > > +++ b/lib/librte_cryptodev/rte_crypto.h
> > > @@ -22,7 +22,6 @@
> > >  #include <rte_mempool.h>
> > >  #include <rte_common.h>
> > >
> > > -#include "rte_crypto_sym.h"
> > >  #include "rte_crypto_asym.h"
> >
> > [Anoob] If rte_crypto.h uses anything defined in rte_crypto_sym.h,
> > then you cannot remove the include.
> 
> [Shally] so you mean we should do #include of sym.h in both, this and
> rte_crypto_asym.h file?

[Anoob] If this file uses anything from rte_crypto_sym.h, then ' #include "rte_crypto_sym.h" ' has to be retained. Here the patch is removing the include.

Instead may be the includes can be rearranged to make it follow alphabetical order. Again, that need not be part of this patch. If Akhil is okay with having includes violating the alphabetical sequence, this change can be deferred.

> 
> >You can make it alphabetical, but the include has to be retained.
> [Shally] Sorry didn't get what you exactly mean by this?
> 
> Thanks
> Shally
> 
> >
> 
> > >
> > >  /** Crypto operation types */

[Anoob] IMO, only the following change is needed. 

> > > diff --git a/lib/librte_cryptodev/rte_crypto_asym.h
> > > b/lib/librte_cryptodev/rte_crypto_asym.h
> > > index 5e43620..a55923a 100644
> > > --- a/lib/librte_cryptodev/rte_crypto_asym.h
> > > +++ b/lib/librte_cryptodev/rte_crypto_asym.h
> > > @@ -25,6 +25,8 @@
> > >  #include <rte_mempool.h>
> > >  #include <rte_common.h>
> > >
> > > +#include "rte_crypto_sym.h"
> > > +
> > >  typedef struct rte_crypto_param_t {
> > >  	uint8_t *data;
> > >  	/**< pointer to buffer holding data */
> > > --
> > > 1.8.3.1