Re: [PATCH v2 1/2] net/mlx4: split the definitions to the header file

[Vasily Philipov <vasilyf@mellanox.com>]

Hi Ferruh,

> -----Original Message-----
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Wednesday, February 22, 2017 21:05
> To: Vasily Philipov <vasilyf@mellanox.com>; dev@dpdk.org
> Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; Nélio Laranjeiro
> <nelio.laranjeiro@6wind.com>
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/mlx4: split the definitions to the
> header file
> 
> On 2/22/2017 1:42 PM, Vasily Philipov wrote:
> > Make some structs/defines visible from different source files by
> > placing them into mlx4.h header.
> >
> > Signed-off-by: Vasily Philipov <vasilyf@mellanox.com>
> > ---
> >  drivers/net/mlx4/mlx4.c | 183
> > ++--------------------------------------------
> >  drivers/net/mlx4/mlx4.h | 187
> > +++++++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 189 insertions(+), 181 deletions(-)
> >
> > diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c index
> > 79efaaa..82ccac8 100644
> > --- a/drivers/net/mlx4/mlx4.c
> > +++ b/drivers/net/mlx4/mlx4.c
> > @@ -1,8 +1,8 @@
> >  /*-
> >   *   BSD LICENSE
> >   *
> > - *   Copyright 2012-2015 6WIND S.A.
> > - *   Copyright 2012 Mellanox.
> > + *   Copyright 2012-2017 6WIND S.A.
> > + *   Copyright 2012-2017 Mellanox.
> 
> Can someone knowledgeable about Copyright help please?
> 
> What is the year field in Copyright line for?
> And above change updates Copyright from 2012 to 2012-2017, is this correct?
> 

The year line was changes in order to show when the file was changed the last time...

> >   *
> >   *   Redistribution and use in source and binary forms, with or without
> >   *   modification, are permitted provided that the following conditions
> > @@ -68,10 +68,6 @@
> >  #pragma GCC diagnostic error "-Wpedantic"
> >  #endif
> 
> Above invisible lines are  "#include <infiniband/verbs.h>" wrapped with
> #pragma for pedantic.
> 
> That piece moved to "mlx4.h" [1], which included a few lines later, so can
> these line be removed from this line?
> 
> >
> > -/* DPDK headers don't like -pedantic. */ -#ifdef PEDANTIC -#pragma
> > GCC diagnostic ignored "-Wpedantic"
> > -#endif
> 
> Comment says "DPDK headers don't like -pedantic", won't removing
> #pragma cause compile error with pedantic option?
> 

It is not necessary anymore, was fixed with the next commit:

commit c0362128c57a0ad22ea311a9657bb15a44b70793
Author: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Date:   Mon Jun 29 11:34:52 2015 +0200

    eal: fix pedantic build of mlx4 debug mode

> >  #include <rte_ether.h>
> >  #include <rte_ethdev.h>
> >  #include <rte_dev.h>
> > @@ -86,9 +82,6 @@
> >  #include <rte_log.h>
> >  #include <rte_alarm.h>
> >  #include <rte_memory.h>
> > -#ifdef PEDANTIC
> > -#pragma GCC diagnostic error "-Wpedantic"
> > -#endif
> >
> >  /* Generated configuration header. */  #include "mlx4_autoconf.h"
> > @@ -96,21 +89,6 @@
> >  /* PMD header. */
> >  #include "mlx4.h"
> >
> <...>
> 
> > diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h index
> > 4c7505e..70c9ecd 100644
> > --- a/drivers/net/mlx4/mlx4.h
> > +++ b/drivers/net/mlx4/mlx4.h
> <...>
> > +
> > +/* Verbs header. */
> > +/* ISO C doesn't support unnamed structs/unions, disabling -pedantic.
> > +*/ #ifdef PEDANTIC #pragma GCC diagnostic ignored "-Wpedantic"
> > +#endif
> > +#include <infiniband/verbs.h>
> > +#ifdef PEDANTIC
> > +#pragma GCC diagnostic error "-Wpedantic"
> > +#endif
> 
> --> [1]
> 
> <...>
> 
> > +
> > +void priv_lock(struct priv *priv);
> > +void priv_unlock(struct priv *priv);
> 
> It can be good to mention in commit log that these functions are now
> exported.
> 
> > +
> >  #endif /* RTE_PMD_MLX4_H_ */
> >

I will fix the rest of the issues and will send the v3 patches.

Thank you,
Vasily