diff mbox series

[v13,09/10] app/testpmd: fix unused function warnings

Message ID 1620241931-28435-10-git-send-email-jizh@linux.microsoft.com (mailing list archive)
State Superseded, archived
Delegated to: Andrew Rybchenko
Headers
Series app/testpmd: enable testpmd on Windows |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Jie Zhou May 5, 2021, 7:12 p.m. UTC 
  Function print_fdir_mask and print_fdir_flex_payload is only called
when either i40e or ixgbe presents. Add #if defined to remove
"unused function" compilation warning.

Signed-off-by: Jie Zhou <jizh@microsoft.com>
Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
Acked-by: Tal Shnaiderman <talshn@nvidia.com>
---
 app/test-pmd/config.c | 82 +++++++++++++++++++++----------------------
 1 file changed, 41 insertions(+), 41 deletions(-)

Comments

Dmitry Kozlyuk June 20, 2021, 11:30 p.m. UTC | #1 
2021-05-05 12:12 (UTC-0700), Jie Zhou:
> Function print_fdir_mask and print_fdir_flex_payload is only called
> when either i40e or ixgbe presents. Add #if defined to remove
> "unused function" compilation warning.
> 
> Signed-off-by: Jie Zhou <jizh@microsoft.com>
> Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
> Acked-by: Tal Shnaiderman <talshn@nvidia.com>
> ---
>  app/test-pmd/config.c | 82 +++++++++++++++++++++----------------------
>  1 file changed, 41 insertions(+), 41 deletions(-)

Code inside #ifdef isn't compile-checked, it's better to avoid.
The only case we can't is when i40e or ixgbe API is called directly.
I'd rather remove #ifdef whenever possible and mark maybe-unused entities,
like this:

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 3723317ab4..97a577fec0 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -4488,8 +4488,6 @@ flowtype_to_str(uint16_t flow_type)
        return NULL;
 }
 
-#if defined(RTE_NET_I40E) || defined(RTE_NET_IXGBE)
-
 static inline void
 print_fdir_mask(struct rte_eth_fdir_masks *mask)
 {
@@ -4590,6 +4588,9 @@ get_fdir_info(portid_t port_id, struct rte_eth_fdir_info *fdir_info,
 {
        int ret = -ENOTSUP;
 
+       RTE_SET_USED(fdir_info);
+       RTE_SET_USED(fdir_stat);
+
 #ifdef RTE_NET_I40E
        if (ret == -ENOTSUP) {
                ret = rte_pmd_i40e_get_fdir_info(port_id, fdir_info);
@@ -4686,8 +4687,6 @@ fdir_get_infos(portid_t port_id)
               fdir_stats_border, fdir_stats_border);
 }
 
-#endif /* RTE_NET_I40E || RTE_NET_IXGBE */
-
 void
 fdir_set_flex_mask(portid_t port_id, struct rte_eth_fdir_flex_mask *cfg)
 {
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index d61a055bdd..a40ee902e8 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -917,9 +917,7 @@ int all_ports_stopped(void);
 int port_is_stopped(portid_t port_id);
 int port_is_started(portid_t port_id);
 void pmd_test_exit(void);
-#if defined(RTE_NET_I40E) || defined(RTE_NET_IXGBE)
 void fdir_get_infos(portid_t port_id);
-#endif
 void fdir_set_flex_mask(portid_t port_id,
                           struct rte_eth_fdir_flex_mask *cfg);
 void fdir_set_flex_payload(portid_t port_id,
Jie Zhou June 23, 2021, 9:26 p.m. UTC | #2 
On Mon, Jun 21, 2021 at 02:30:53AM +0300, Dmitry Kozlyuk wrote:
> 2021-05-05 12:12 (UTC-0700), Jie Zhou:
> > Function print_fdir_mask and print_fdir_flex_payload is only called
> > when either i40e or ixgbe presents. Add #if defined to remove
> > "unused function" compilation warning.
> > 
> > Signed-off-by: Jie Zhou <jizh@microsoft.com>
> > Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
> > Acked-by: Tal Shnaiderman <talshn@nvidia.com>
> > ---
> >  app/test-pmd/config.c | 82 +++++++++++++++++++++----------------------
> >  1 file changed, 41 insertions(+), 41 deletions(-)
> 
> Code inside #ifdef isn't compile-checked, it's better to avoid.
> The only case we can't is when i40e or ixgbe API is called directly.
> I'd rather remove #ifdef whenever possible and mark maybe-unused entities,
> like this:
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 3723317ab4..97a577fec0 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -4488,8 +4488,6 @@ flowtype_to_str(uint16_t flow_type)
>         return NULL;
>  }
>  
> -#if defined(RTE_NET_I40E) || defined(RTE_NET_IXGBE)
> -
>  static inline void
>  print_fdir_mask(struct rte_eth_fdir_masks *mask)
>  {
> @@ -4590,6 +4588,9 @@ get_fdir_info(portid_t port_id, struct rte_eth_fdir_info *fdir_info,
>  {
>         int ret = -ENOTSUP;
>  
> +       RTE_SET_USED(fdir_info);
> +       RTE_SET_USED(fdir_stat);
> +
>  #ifdef RTE_NET_I40E
>         if (ret == -ENOTSUP) {
>                 ret = rte_pmd_i40e_get_fdir_info(port_id, fdir_info);
> @@ -4686,8 +4687,6 @@ fdir_get_infos(portid_t port_id)
>                fdir_stats_border, fdir_stats_border);
>  }
>  
> -#endif /* RTE_NET_I40E || RTE_NET_IXGBE */
> -
>  void
>  fdir_set_flex_mask(portid_t port_id, struct rte_eth_fdir_flex_mask *cfg)
>  {
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index d61a055bdd..a40ee902e8 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -917,9 +917,7 @@ int all_ports_stopped(void);
>  int port_is_stopped(portid_t port_id);
>  int port_is_started(portid_t port_id);
>  void pmd_test_exit(void);
> -#if defined(RTE_NET_I40E) || defined(RTE_NET_IXGBE)
>  void fdir_get_infos(portid_t port_id);
> -#endif
>  void fdir_set_flex_mask(portid_t port_id,
>                            struct rte_eth_fdir_flex_mask *cfg);
>  void fdir_set_flex_payload(portid_t port_id,

Hi Dmitry, I agree that should avoid the #ifdef as much as possible. But in this case, I am not quite sure if I followed your comment correctly. Someone originally introduced these i40e and ixgbe related fdir functions (print_fdir_mask, print_fdir_flex_payload, print_fdir_flex_mask, print_fdir_flow_type, get_fdir_info, fdir_get_infos) into testpmd with adding the #if defined(RTE_NET_I40E) || defined(RTE_NET_IXGBE) for 4 out of 6 functions and left 2 of them outside the #ifdef which caused compilation "unused function" warning. What I did here is just move the starting point of #ifdef to also include those 2 missed functions (print_fdir_mask and print_fdir_flex_payload). IMO the original author would be in better place to reducing the unneccary #ifdef in a proper way.
Tyler Retzlaff June 24, 2021, 3:45 p.m. UTC | #3 
On Wed, Jun 23, 2021 at 02:26:32PM -0700, Jie Zhou wrote:
> On Mon, Jun 21, 2021 at 02:30:53AM +0300, Dmitry Kozlyuk wrote:
> > -
> >  void
> >  fdir_set_flex_mask(portid_t port_id, struct rte_eth_fdir_flex_mask *cfg)
> >  {
> > diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> > index d61a055bdd..a40ee902e8 100644
> > --- a/app/test-pmd/testpmd.h
> > +++ b/app/test-pmd/testpmd.h
> > @@ -917,9 +917,7 @@ int all_ports_stopped(void);
> >  int port_is_stopped(portid_t port_id);
> >  int port_is_started(portid_t port_id);
> >  void pmd_test_exit(void);
> > -#if defined(RTE_NET_I40E) || defined(RTE_NET_IXGBE)
> >  void fdir_get_infos(portid_t port_id);
> > -#endif
> >  void fdir_set_flex_mask(portid_t port_id,
> >                            struct rte_eth_fdir_flex_mask *cfg);
> >  void fdir_set_flex_payload(portid_t port_id,
> 
> Hi Dmitry, I agree that should avoid the #ifdef as much as possible. But in this case, I am not quite sure if I followed your comment correctly. Someone originally introduced these i40e and ixgbe related fdir functions (print_fdir_mask, print_fdir_flex_payload, print_fdir_flex_mask, print_fdir_flow_type, get_fdir_info, fdir_get_infos) into testpmd with adding the #if defined(RTE_NET_I40E) || defined(RTE_NET_IXGBE) for 4 out of 6 functions and left 2 of them outside the #ifdef which caused compilation "unused function" warning. What I did here is just move the starting point of #ifdef to also include those 2 missed functions (print_fdir_mask and print_fdir_flex_payload). IMO the original author would be in better place to reducing the unneccary #ifdef in a proper way.

i think i have to agree with jie here. there are limits to how many
defects we should have to correct which are unrelated change. if this is
critical i think it would be best if the maintainer provide a patch
cleaning up the code they own.

let's not hold this patch up over it because of it being a broad change
we lose a lot of time rebasing where either the maintainer or author
could follow up with a narrow change to correct this.
Dmitry Kozlyuk June 24, 2021, 6:44 p.m. UTC | #4 
2021-06-24 08:45 (UTC-0700), Tyler Retzlaff:
> On Wed, Jun 23, 2021 at 02:26:32PM -0700, Jie Zhou wrote:
> > On Mon, Jun 21, 2021 at 02:30:53AM +0300, Dmitry Kozlyuk wrote:  
> > > -
> > >  void
> > >  fdir_set_flex_mask(portid_t port_id, struct rte_eth_fdir_flex_mask *cfg)
> > >  {
> > > diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> > > index d61a055bdd..a40ee902e8 100644
> > > --- a/app/test-pmd/testpmd.h
> > > +++ b/app/test-pmd/testpmd.h
> > > @@ -917,9 +917,7 @@ int all_ports_stopped(void);
> > >  int port_is_stopped(portid_t port_id);
> > >  int port_is_started(portid_t port_id);
> > >  void pmd_test_exit(void);
> > > -#if defined(RTE_NET_I40E) || defined(RTE_NET_IXGBE)
> > >  void fdir_get_infos(portid_t port_id);
> > > -#endif
> > >  void fdir_set_flex_mask(portid_t port_id,
> > >                            struct rte_eth_fdir_flex_mask *cfg);
> > >  void fdir_set_flex_payload(portid_t port_id,  
> > 
> > Hi Dmitry, I agree that should avoid the #ifdef as much as possible. But in this case, I am not quite sure if I followed your comment correctly. Someone originally introduced these i40e and ixgbe related fdir functions (print_fdir_mask, print_fdir_flex_payload, print_fdir_flex_mask, print_fdir_flow_type, get_fdir_info, fdir_get_infos) into testpmd with adding the #if defined(RTE_NET_I40E) || defined(RTE_NET_IXGBE) for 4 out of 6 functions and left 2 of them outside the #ifdef which caused compilation "unused function" warning. What I did here is just move the starting point of #ifdef to also include those 2 missed functions (print_fdir_mask and print_fdir_flex_payload). IMO the original author would be in better place to reducing the unneccary #ifdef in a proper way.  
> 
> i think i have to agree with jie here. there are limits to how many
> defects we should have to correct which are unrelated change. if this is
> critical i think it would be best if the maintainer provide a patch
> cleaning up the code they own.
> 
> let's not hold this patch up over it because of it being a broad change
> we lose a lot of time rebasing where either the maintainer or author
> could follow up with a narrow change to correct this.

Fair enough, the patch doesn't add technical debt at least.

Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
Jie Zhou June 24, 2021, 9:36 p.m. UTC | #5 
On Thu, Jun 24, 2021 at 09:44:04PM +0300, Dmitry Kozlyuk wrote:
> 2021-06-24 08:45 (UTC-0700), Tyler Retzlaff:
> > On Wed, Jun 23, 2021 at 02:26:32PM -0700, Jie Zhou wrote:
> > > On Mon, Jun 21, 2021 at 02:30:53AM +0300, Dmitry Kozlyuk wrote:  
> > > > -
> > > >  void
> > > >  fdir_set_flex_mask(portid_t port_id, struct rte_eth_fdir_flex_mask *cfg)
> > > >  {
> > > > diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> > > > index d61a055bdd..a40ee902e8 100644
> > > > --- a/app/test-pmd/testpmd.h
> > > > +++ b/app/test-pmd/testpmd.h
> > > > @@ -917,9 +917,7 @@ int all_ports_stopped(void);
> > > >  int port_is_stopped(portid_t port_id);
> > > >  int port_is_started(portid_t port_id);
> > > >  void pmd_test_exit(void);
> > > > -#if defined(RTE_NET_I40E) || defined(RTE_NET_IXGBE)
> > > >  void fdir_get_infos(portid_t port_id);
> > > > -#endif
> > > >  void fdir_set_flex_mask(portid_t port_id,
> > > >                            struct rte_eth_fdir_flex_mask *cfg);
> > > >  void fdir_set_flex_payload(portid_t port_id,  
> > > 
> > > Hi Dmitry, I agree that should avoid the #ifdef as much as possible. But in this case, I am not quite sure if I followed your comment correctly. Someone originally introduced these i40e and ixgbe related fdir functions (print_fdir_mask, print_fdir_flex_payload, print_fdir_flex_mask, print_fdir_flow_type, get_fdir_info, fdir_get_infos) into testpmd with adding the #if defined(RTE_NET_I40E) || defined(RTE_NET_IXGBE) for 4 out of 6 functions and left 2 of them outside the #ifdef which caused compilation "unused function" warning. What I did here is just move the starting point of #ifdef to also include those 2 missed functions (print_fdir_mask and print_fdir_flex_payload). IMO the original author would be in better place to reducing the unneccary #ifdef in a proper way.  
> > 
> > i think i have to agree with jie here. there are limits to how many
> > defects we should have to correct which are unrelated change. if this is
> > critical i think it would be best if the maintainer provide a patch
> > cleaning up the code they own.
> > 
> > let's not hold this patch up over it because of it being a broad change
> > we lose a lot of time rebasing where either the maintainer or author
> > could follow up with a narrow change to correct this.
> 
> Fair enough, the patch doesn't add technical debt at least.
> 
> Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

Thanks Dmitry. Can you please help Ack on V14 which I sent out yesterday? Otherwise, I will send out V15 to carry over this ack from this V13.
diff mbox series

Patch

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index e2e1a31f2..00a65f442 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -4446,6 +4446,47 @@  set_record_burst_stats(uint8_t on_off)
 	record_burst_stats = on_off;
 }
 
+static char*
+flowtype_to_str(uint16_t flow_type)
+{
+	struct flow_type_info {
+		char str[32];
+		uint16_t ftype;
+	};
+
+	uint8_t i;
+	static struct flow_type_info flowtype_str_table[] = {
+		{"raw", RTE_ETH_FLOW_RAW},
+		{"ipv4", RTE_ETH_FLOW_IPV4},
+		{"ipv4-frag", RTE_ETH_FLOW_FRAG_IPV4},
+		{"ipv4-tcp", RTE_ETH_FLOW_NONFRAG_IPV4_TCP},
+		{"ipv4-udp", RTE_ETH_FLOW_NONFRAG_IPV4_UDP},
+		{"ipv4-sctp", RTE_ETH_FLOW_NONFRAG_IPV4_SCTP},
+		{"ipv4-other", RTE_ETH_FLOW_NONFRAG_IPV4_OTHER},
+		{"ipv6", RTE_ETH_FLOW_IPV6},
+		{"ipv6-frag", RTE_ETH_FLOW_FRAG_IPV6},
+		{"ipv6-tcp", RTE_ETH_FLOW_NONFRAG_IPV6_TCP},
+		{"ipv6-udp", RTE_ETH_FLOW_NONFRAG_IPV6_UDP},
+		{"ipv6-sctp", RTE_ETH_FLOW_NONFRAG_IPV6_SCTP},
+		{"ipv6-other", RTE_ETH_FLOW_NONFRAG_IPV6_OTHER},
+		{"l2_payload", RTE_ETH_FLOW_L2_PAYLOAD},
+		{"port", RTE_ETH_FLOW_PORT},
+		{"vxlan", RTE_ETH_FLOW_VXLAN},
+		{"geneve", RTE_ETH_FLOW_GENEVE},
+		{"nvgre", RTE_ETH_FLOW_NVGRE},
+		{"vxlan-gpe", RTE_ETH_FLOW_VXLAN_GPE},
+	};
+
+	for (i = 0; i < RTE_DIM(flowtype_str_table); i++) {
+		if (flowtype_str_table[i].ftype == flow_type)
+			return flowtype_str_table[i].str;
+	}
+
+	return NULL;
+}
+
+#if defined(RTE_NET_I40E) || defined(RTE_NET_IXGBE)
+
 static inline void
 print_fdir_mask(struct rte_eth_fdir_masks *mask)
 {
@@ -4505,47 +4546,6 @@  print_fdir_flex_payload(struct rte_eth_fdir_flex_conf *flex_conf, uint32_t num)
 	printf("\n");
 }
 
-static char *
-flowtype_to_str(uint16_t flow_type)
-{
-	struct flow_type_info {
-		char str[32];
-		uint16_t ftype;
-	};
-
-	uint8_t i;
-	static struct flow_type_info flowtype_str_table[] = {
-		{"raw", RTE_ETH_FLOW_RAW},
-		{"ipv4", RTE_ETH_FLOW_IPV4},
-		{"ipv4-frag", RTE_ETH_FLOW_FRAG_IPV4},
-		{"ipv4-tcp", RTE_ETH_FLOW_NONFRAG_IPV4_TCP},
-		{"ipv4-udp", RTE_ETH_FLOW_NONFRAG_IPV4_UDP},
-		{"ipv4-sctp", RTE_ETH_FLOW_NONFRAG_IPV4_SCTP},
-		{"ipv4-other", RTE_ETH_FLOW_NONFRAG_IPV4_OTHER},
-		{"ipv6", RTE_ETH_FLOW_IPV6},
-		{"ipv6-frag", RTE_ETH_FLOW_FRAG_IPV6},
-		{"ipv6-tcp", RTE_ETH_FLOW_NONFRAG_IPV6_TCP},
-		{"ipv6-udp", RTE_ETH_FLOW_NONFRAG_IPV6_UDP},
-		{"ipv6-sctp", RTE_ETH_FLOW_NONFRAG_IPV6_SCTP},
-		{"ipv6-other", RTE_ETH_FLOW_NONFRAG_IPV6_OTHER},
-		{"l2_payload", RTE_ETH_FLOW_L2_PAYLOAD},
-		{"port", RTE_ETH_FLOW_PORT},
-		{"vxlan", RTE_ETH_FLOW_VXLAN},
-		{"geneve", RTE_ETH_FLOW_GENEVE},
-		{"nvgre", RTE_ETH_FLOW_NVGRE},
-		{"vxlan-gpe", RTE_ETH_FLOW_VXLAN_GPE},
-	};
-
-	for (i = 0; i < RTE_DIM(flowtype_str_table); i++) {
-		if (flowtype_str_table[i].ftype == flow_type)
-			return flowtype_str_table[i].str;
-	}
-
-	return NULL;
-}
-
-#if defined(RTE_NET_I40E) || defined(RTE_NET_IXGBE)
-
 static inline void
 print_fdir_flex_mask(struct rte_eth_fdir_flex_conf *flex_conf, uint32_t num)
 {