diff mbox series

[v2,6/7] drivers: remove POSIX dependencies

Message ID 20210221012831.14643-7-dmitry.kozliuk@gmail.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series eal/windows: do not expose POSIX symbols |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Dmitry Kozlyuk Feb. 21, 2021, 1:28 a.m. UTC 
  Replace POSIX strncasecmp() with EAL rte_strncasecmp().
Replace POSIX strtok_r() with EAL rte_strtok().
Replace POSIX strdup() with EAL rte_strdup().

Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
i40e: checkpatches.sh complains about long lines (it's ~85).
      I doubt that mechanical fix would keep the code readable.
      It's on 5th level of indentation, so I'd extract a function,
      but would like to hear from maintainers first.

 drivers/bus/pci/private.h             |  2 +-
 drivers/bus/vdev/vdev.c               |  4 +-
 drivers/bus/vdev/vdev_params.c        |  3 +-
 drivers/common/mlx5/mlx5_common_pci.c |  4 +-
 drivers/common/mlx5/mlx5_common_pci.h |  1 +
 drivers/net/i40e/i40e_ethdev.c        | 56 +++++++++++++--------------
 6 files changed, 36 insertions(+), 34 deletions(-)

Comments

Tal Shnaiderman Feb. 21, 2021, 8:59 a.m. UTC | #1 
> Subject: [dpdk-dev] [PATCH v2 6/7] drivers: remove POSIX dependencies
> 
> External email: Use caution opening links or attachments
> 
> 
> Replace POSIX strncasecmp() with EAL rte_strncasecmp().
> Replace POSIX strtok_r() with EAL rte_strtok().
> Replace POSIX strdup() with EAL rte_strdup().
> 
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
> i40e: checkpatches.sh complains about long lines (it's ~85).
>       I doubt that mechanical fix would keep the code readable.
>       It's on 5th level of indentation, so I'd extract a function,
>       but would like to hear from maintainers first.
> 
>  drivers/bus/pci/private.h             |  2 +-
>  drivers/bus/vdev/vdev.c               |  4 +-
>  drivers/bus/vdev/vdev_params.c        |  3 +-
>  drivers/common/mlx5/mlx5_common_pci.c |  4 +-

bus_cmdline_options_handler in mlx5_common_pci.c has a call to strdup which needs to be renamed to rte_strdup
(Also failed CI: https://lab.dpdk.org/results/dashboard/patchsets/15674/ )
Andrew Rybchenko Feb. 21, 2021, 3:54 p.m. UTC | #2 
On 2/21/21 11:59 AM, Tal Shnaiderman wrote:
>> Subject: [dpdk-dev] [PATCH v2 6/7] drivers: remove POSIX dependencies
>>
>> External email: Use caution opening links or attachments
>>
>>
>> Replace POSIX strncasecmp() with EAL rte_strncasecmp().
>> Replace POSIX strtok_r() with EAL rte_strtok().
>> Replace POSIX strdup() with EAL rte_strdup().
>>
>> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
>> ---
>> i40e: checkpatches.sh complains about long lines (it's ~85).
>>        I doubt that mechanical fix would keep the code readable.
>>        It's on 5th level of indentation, so I'd extract a function,
>>        but would like to hear from maintainers first.
>>
>>   drivers/bus/pci/private.h             |  2 +-
>>   drivers/bus/vdev/vdev.c               |  4 +-
>>   drivers/bus/vdev/vdev_params.c        |  3 +-
>>   drivers/common/mlx5/mlx5_common_pci.c |  4 +-
> 
> bus_cmdline_options_handler in mlx5_common_pci.c has a call to strdup which needs to be renamed to rte_strdup
> (Also failed CI: https://lab.dpdk.org/results/dashboard/patchsets/15674/ )
> 


Frankly speaking I don't understand why such changes are useful/needed.
Patch description does not explain/prove it.
Dmitry Kozlyuk Feb. 21, 2021, 5:05 p.m. UTC | #3 
On Sun, 21 Feb 2021 18:54:01 +0300, Andrew Rybchenko wrote:
> On 2/21/21 11:59 AM, Tal Shnaiderman wrote:
> >> Subject: [dpdk-dev] [PATCH v2 6/7] drivers: remove POSIX dependencies
> >>
> >> External email: Use caution opening links or attachments
> >>
> >>
> >> Replace POSIX strncasecmp() with EAL rte_strncasecmp().
> >> Replace POSIX strtok_r() with EAL rte_strtok().
> >> Replace POSIX strdup() with EAL rte_strdup().
> >>
> >> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> >> ---
> >> i40e: checkpatches.sh complains about long lines (it's ~85).
> >>        I doubt that mechanical fix would keep the code readable.
> >>        It's on 5th level of indentation, so I'd extract a function,
> >>        but would like to hear from maintainers first.
> >>
> >>   drivers/bus/pci/private.h             |  2 +-
> >>   drivers/bus/vdev/vdev.c               |  4 +-
> >>   drivers/bus/vdev/vdev_params.c        |  3 +-
> >>   drivers/common/mlx5/mlx5_common_pci.c |  4 +-  
> > 
> > bus_cmdline_options_handler in mlx5_common_pci.c has a call to strdup which needs to be renamed to rte_strdup
> > (Also failed CI: https://lab.dpdk.org/results/dashboard/patchsets/15674/ )
> >   
> 
> 
> Frankly speaking I don't understand why such changes are useful/needed.
> Patch description does not explain/prove it.

Please see the cover letter (better to partially repeat in each patch?):

	https://mails.dpdk.org/archives/dev/2021-February/199981.html

Driver code using POSIX functions is not really portable without a shim.
Having a shim in public API is wrong, it makes DPDK expose non-DPDK symbols.

An alternative approach would be to make the shim DPDK-internal. It would
require just a change in #includes for drivers and libs. My doubt is whether
EAL should emulate POSIX.
diff mbox series

Patch

diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h
index f566943f5..5648916d1 100644
--- a/drivers/bus/pci/private.h
+++ b/drivers/bus/pci/private.h
@@ -92,7 +92,7 @@  struct mapped_pci_resource {
 	TAILQ_ENTRY(mapped_pci_resource) next;
 
 	struct rte_pci_addr pci_addr;
-	char path[PATH_MAX];
+	char path[RTE_PATH_MAX];
 	int nb_maps;
 	struct pci_map maps[PCI_MAX_RESOURCE];
 	struct pci_msix_table msix_table;
diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
index 9a673347a..a490d26a2 100644
--- a/drivers/bus/vdev/vdev.c
+++ b/drivers/bus/vdev/vdev.c
@@ -245,9 +245,9 @@  alloc_devargs(const char *name, const char *args)
 
 	devargs->bus = &rte_vdev_bus;
 	if (args)
-		devargs->args = strdup(args);
+		devargs->args = rte_strdup(args);
 	else
-		devargs->args = strdup("");
+		devargs->args = rte_strdup("");
 
 	ret = strlcpy(devargs->name, name, sizeof(devargs->name));
 	if (ret < 0 || ret >= (int)sizeof(devargs->name)) {
diff --git a/drivers/bus/vdev/vdev_params.c b/drivers/bus/vdev/vdev_params.c
index 6f74704d1..64848f4c2 100644
--- a/drivers/bus/vdev/vdev_params.c
+++ b/drivers/bus/vdev/vdev_params.c
@@ -8,6 +8,7 @@ 
 #include <rte_bus.h>
 #include <rte_kvargs.h>
 #include <rte_errno.h>
+#include <rte_string_fns.h>
 
 #include "vdev_logs.h"
 #include "vdev_private.h"
@@ -31,7 +32,7 @@  vdev_dev_match(const struct rte_device *dev,
 	char *name;
 
 	/* cannot pass const dev->name to rte_kvargs_process() */
-	name = strdup(dev->name);
+	name = rte_strdup(dev->name);
 	if (name == NULL)
 		return -1;
 	ret = rte_kvargs_process(kvlist,
diff --git a/drivers/common/mlx5/mlx5_common_pci.c b/drivers/common/mlx5/mlx5_common_pci.c
index 2b657686d..37f4182e2 100644
--- a/drivers/common/mlx5/mlx5_common_pci.c
+++ b/drivers/common/mlx5/mlx5_common_pci.c
@@ -88,7 +88,7 @@  bus_cmdline_options_handler(__rte_unused const char *key,
 		return *ret;
 	}
 	nstr_org = nstr;
-	found = strtok_r(nstr, ":", &refstr);
+	found = rte_strtok(nstr, ":", &refstr);
 	if (!found)
 		goto err;
 	do {
@@ -102,7 +102,7 @@  bus_cmdline_options_handler(__rte_unused const char *key,
 			goto err;
 		}
 		*ret |= class_val;
-		found = strtok_r(NULL, ":", &refstr);
+		found = rte_strtok(NULL, ":", &refstr);
 	} while (found);
 err:
 	free(nstr_org);
diff --git a/drivers/common/mlx5/mlx5_common_pci.h b/drivers/common/mlx5/mlx5_common_pci.h
index de89bb98b..729d0f4cc 100644
--- a/drivers/common/mlx5/mlx5_common_pci.h
+++ b/drivers/common/mlx5/mlx5_common_pci.h
@@ -45,6 +45,7 @@  extern "C" {
 
 #include <rte_pci.h>
 #include <rte_bus_pci.h>
+#include <rte_string_fns.h>
 
 #include <mlx5_common.h>
 
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index d7cd04989..5b806d201 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -12036,110 +12036,110 @@  i40e_update_customized_ptype(struct rte_eth_dev *dev, uint8_t *pkg,
 				memset(name, 0, sizeof(name));
 				strcpy(name, proto[n].name);
 				PMD_DRV_LOG(INFO, "name = %s\n", name);
-				if (!strncasecmp(name, "PPPOE", 5))
+				if (!rte_strncasecmp(name, "PPPOE", 5))
 					ptype_mapping[i].sw_ptype |=
 						RTE_PTYPE_L2_ETHER_PPPOE;
-				else if (!strncasecmp(name, "IPV4FRAG", 8) &&
+				else if (!rte_strncasecmp(name, "IPV4FRAG", 8) &&
 					 !in_tunnel) {
 					ptype_mapping[i].sw_ptype |=
 						RTE_PTYPE_L3_IPV4_EXT_UNKNOWN;
 					ptype_mapping[i].sw_ptype |=
 						RTE_PTYPE_L4_FRAG;
-				} else if (!strncasecmp(name, "IPV4FRAG", 8) &&
+				} else if (!rte_strncasecmp(name, "IPV4FRAG", 8) &&
 					   in_tunnel) {
 					ptype_mapping[i].sw_ptype |=
 					    RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN;
 					ptype_mapping[i].sw_ptype |=
 						RTE_PTYPE_INNER_L4_FRAG;
-				} else if (!strncasecmp(name, "OIPV4", 5)) {
+				} else if (!rte_strncasecmp(name, "OIPV4", 5)) {
 					ptype_mapping[i].sw_ptype |=
 						RTE_PTYPE_L3_IPV4_EXT_UNKNOWN;
 					in_tunnel = true;
-				} else if (!strncasecmp(name, "IPV4", 4) &&
+				} else if (!rte_strncasecmp(name, "IPV4", 4) &&
 					   !in_tunnel)
 					ptype_mapping[i].sw_ptype |=
 						RTE_PTYPE_L3_IPV4_EXT_UNKNOWN;
-				else if (!strncasecmp(name, "IPV4", 4) &&
+				else if (!rte_strncasecmp(name, "IPV4", 4) &&
 					 in_tunnel)
 					ptype_mapping[i].sw_ptype |=
 					    RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN;
-				else if (!strncasecmp(name, "IPV6FRAG", 8) &&
+				else if (!rte_strncasecmp(name, "IPV6FRAG", 8) &&
 					 !in_tunnel) {
 					ptype_mapping[i].sw_ptype |=
 						RTE_PTYPE_L3_IPV6_EXT_UNKNOWN;
 					ptype_mapping[i].sw_ptype |=
 						RTE_PTYPE_L4_FRAG;
-				} else if (!strncasecmp(name, "IPV6FRAG", 8) &&
+				} else if (!rte_strncasecmp(name, "IPV6FRAG", 8) &&
 					   in_tunnel) {
 					ptype_mapping[i].sw_ptype |=
 					    RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN;
 					ptype_mapping[i].sw_ptype |=
 						RTE_PTYPE_INNER_L4_FRAG;
-				} else if (!strncasecmp(name, "OIPV6", 5)) {
+				} else if (!rte_strncasecmp(name, "OIPV6", 5)) {
 					ptype_mapping[i].sw_ptype |=
 						RTE_PTYPE_L3_IPV6_EXT_UNKNOWN;
 					in_tunnel = true;
-				} else if (!strncasecmp(name, "IPV6", 4) &&
+				} else if (!rte_strncasecmp(name, "IPV6", 4) &&
 					   !in_tunnel)
 					ptype_mapping[i].sw_ptype |=
 						RTE_PTYPE_L3_IPV6_EXT_UNKNOWN;
-				else if (!strncasecmp(name, "IPV6", 4) &&
+				else if (!rte_strncasecmp(name, "IPV6", 4) &&
 					 in_tunnel)
 					ptype_mapping[i].sw_ptype |=
 					    RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN;
-				else if (!strncasecmp(name, "UDP", 3) &&
+				else if (!rte_strncasecmp(name, "UDP", 3) &&
 					 !in_tunnel)
 					ptype_mapping[i].sw_ptype |=
 						RTE_PTYPE_L4_UDP;
-				else if (!strncasecmp(name, "UDP", 3) &&
+				else if (!rte_strncasecmp(name, "UDP", 3) &&
 					 in_tunnel)
 					ptype_mapping[i].sw_ptype |=
 						RTE_PTYPE_INNER_L4_UDP;
-				else if (!strncasecmp(name, "TCP", 3) &&
+				else if (!rte_strncasecmp(name, "TCP", 3) &&
 					 !in_tunnel)
 					ptype_mapping[i].sw_ptype |=
 						RTE_PTYPE_L4_TCP;
-				else if (!strncasecmp(name, "TCP", 3) &&
+				else if (!rte_strncasecmp(name, "TCP", 3) &&
 					 in_tunnel)
 					ptype_mapping[i].sw_ptype |=
 						RTE_PTYPE_INNER_L4_TCP;
-				else if (!strncasecmp(name, "SCTP", 4) &&
+				else if (!rte_strncasecmp(name, "SCTP", 4) &&
 					 !in_tunnel)
 					ptype_mapping[i].sw_ptype |=
 						RTE_PTYPE_L4_SCTP;
-				else if (!strncasecmp(name, "SCTP", 4) &&
+				else if (!rte_strncasecmp(name, "SCTP", 4) &&
 					 in_tunnel)
 					ptype_mapping[i].sw_ptype |=
 						RTE_PTYPE_INNER_L4_SCTP;
-				else if ((!strncasecmp(name, "ICMP", 4) ||
-					  !strncasecmp(name, "ICMPV6", 6)) &&
+				else if ((!rte_strncasecmp(name, "ICMP", 4) ||
+					  !rte_strncasecmp(name, "ICMPV6", 6)) &&
 					 !in_tunnel)
 					ptype_mapping[i].sw_ptype |=
 						RTE_PTYPE_L4_ICMP;
-				else if ((!strncasecmp(name, "ICMP", 4) ||
-					  !strncasecmp(name, "ICMPV6", 6)) &&
+				else if ((!rte_strncasecmp(name, "ICMP", 4) ||
+					  !rte_strncasecmp(name, "ICMPV6", 6)) &&
 					 in_tunnel)
 					ptype_mapping[i].sw_ptype |=
 						RTE_PTYPE_INNER_L4_ICMP;
-				else if (!strncasecmp(name, "GTPC", 4)) {
+				else if (!rte_strncasecmp(name, "GTPC", 4)) {
 					ptype_mapping[i].sw_ptype |=
 						RTE_PTYPE_TUNNEL_GTPC;
 					in_tunnel = true;
-				} else if (!strncasecmp(name, "GTPU", 4)) {
+				} else if (!rte_strncasecmp(name, "GTPU", 4)) {
 					ptype_mapping[i].sw_ptype |=
 						RTE_PTYPE_TUNNEL_GTPU;
 					in_tunnel = true;
-				} else if (!strncasecmp(name, "ESP", 3)) {
+				} else if (!rte_strncasecmp(name, "ESP", 3)) {
 					ptype_mapping[i].sw_ptype |=
 						RTE_PTYPE_TUNNEL_ESP;
 					in_tunnel = true;
-				} else if (!strncasecmp(name, "GRENAT", 6)) {
+				} else if (!rte_strncasecmp(name, "GRENAT", 6)) {
 					ptype_mapping[i].sw_ptype |=
 						RTE_PTYPE_TUNNEL_GRENAT;
 					in_tunnel = true;
-				} else if (!strncasecmp(name, "L2TPV2CTL", 9) ||
-					   !strncasecmp(name, "L2TPV2", 6) ||
-					   !strncasecmp(name, "L2TPV3", 6)) {
+				} else if (!rte_strncasecmp(name, "L2TPV2CTL", 9) ||
+					   !rte_strncasecmp(name, "L2TPV2", 6) ||
+					   !rte_strncasecmp(name, "L2TPV3", 6)) {
 					ptype_mapping[i].sw_ptype |=
 						RTE_PTYPE_TUNNEL_L2TP;
 					in_tunnel = true;