Re: [dpdk-dev] [PATCH v8 4/4] net: provide IP-related API on any OS

[Olivier Matz <olivier.matz@6wind.com>]

Hi Dmitry,

On Thu, Apr 08, 2021 at 01:22:49AM +0300, Dmitry Kozlyuk wrote:
> Users of <rte_ip.h> relied on it to provide IP-related defines,
> like IPPROTO_* constants, but still had to include POSIX headers
> for inet_pton() and other standard IP-related facilities.
> 
> Extend <rte_ip.h> so that it is a single header to gain access
> to IP-related facilities on any OS. Use it to replace POSIX includes
> in components enabled on Windows. Move missing constants from Windows
> networking shim to OS shim header and include it where needed.
> 
> Remove Windows networking shim that is no longer needed.
> 
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
>  drivers/net/i40e/i40e_fdir.c                 |  1 +
>  drivers/net/mlx5/mlx5.h                      |  1 -
>  drivers/net/mlx5/mlx5_flow.c                 |  4 +--
>  drivers/net/mlx5/mlx5_flow.h                 |  3 +-
>  drivers/net/mlx5/mlx5_mac.c                  |  1 -
>  examples/cmdline/commands.c                  |  5 ---
>  examples/cmdline/parse_obj_list.c            |  2 --
>  lib/librte_cmdline/cmdline.c                 |  1 -
>  lib/librte_cmdline/cmdline_parse.c           |  2 --
>  lib/librte_cmdline/cmdline_parse_etheraddr.c |  6 ----
>  lib/librte_cmdline/cmdline_parse_ipaddr.c    |  6 ----
>  lib/librte_cmdline/cmdline_parse_ipaddr.h    |  2 +-
>  lib/librte_eal/windows/include/arpa/inet.h   | 30 ----------------
>  lib/librte_eal/windows/include/netinet/in.h  | 38 --------------------
>  lib/librte_eal/windows/include/netinet/ip.h  | 10 ------
>  lib/librte_eal/windows/include/rte_os_shim.h |  8 +++++
>  lib/librte_eal/windows/include/sys/socket.h  | 24 -------------
>  lib/librte_ethdev/rte_ethdev.c               | 12 +++----
>  lib/librte_ethdev/rte_ethdev_core.h          |  1 -
>  lib/librte_net/rte_ip.h                      |  7 ++++
>  lib/librte_net/rte_net.c                     |  1 +
>  21 files changed, 24 insertions(+), 141 deletions(-)
>  delete mode 100644 lib/librte_eal/windows/include/arpa/inet.h
>  delete mode 100644 lib/librte_eal/windows/include/netinet/in.h
>  delete mode 100644 lib/librte_eal/windows/include/netinet/ip.h
>  delete mode 100644 lib/librte_eal/windows/include/sys/socket.h

I see it has already been discussed for posix functions like close() or
strdup(), so I won't reopen the door too long ;)

Since DPDK is a network-oriented project, it provides network defines or
structure, prefixed with rte_. This API is on some aspects more complete
than the one provided in libc (for instance, more protocol headers are
available) . So, to me, it would make sense to define RTE_IPPROTO_* and
replace usages of IPPROTO_*, and avoid inclusions of network libc
headers in DPDK code.

This can be done later, if there is a consensus.

> diff --git a/drivers/net/i40e/i40e_fdir.c b/drivers/net/i40e/i40e_fdir.c
> index c572d003cb..e7361bf520 100644
> --- a/drivers/net/i40e/i40e_fdir.c
> +++ b/drivers/net/i40e/i40e_fdir.c
> @@ -22,6 +22,7 @@
>  #include <rte_sctp.h>
>  #include <rte_hash_crc.h>
>  #include <rte_bitmap.h>
> +#include <rte_os_shim.h>
>  

If I understand the logic, rte_ip.h provides OS-specific IP-related
stuff (like IPPROTO_*), and rte_os_shim.h provides the POSIX definitions
that are missing after including rte_ip.h.

Would it make sense to include rte_os_shim.h from rte_ip.h, so that
including rte_ip.h is always sufficient? Or is it because we want to
avoid implicit inclusion of rte_os_shim.h?

Thanks,
Olivier