[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(-)
  

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.

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. You can make it alphabetical, but the include has to be retained.

> 
>  /** Crypto operation types */
> 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
  

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?

> 
> 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?

>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 */
> > 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
  

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
  

Hi Anoob,

> > >
> > > 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.
> 
Agreed
> >
> > >
> > > 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.

Agreed.
> 
> 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.
I am ok with this.

> 
> >
> > >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.

Agreed.
> 
> > > > 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