dpdk-pcap.c compilation warnings

[idogoshen]

fxlb commented 3 days ago •
Many warnings with clang-16:

pcap-dpdk.c:335:15: warning: variable 'gather_len' set but not used [-Wunused-but-set-variable]
        unsigned int gather_len =0;
                     ^
pcap-dpdk.c:474:32: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'u_int' (aka 'unsigned int') [-Wshorten-64-to-32]
                ps->ps_recv = pd->curr_stats.ipackets;
                            ~ ~~~~~~~~~~~~~~~^~~~~~~~
pcap-dpdk.c:475:32: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'u_int' (aka 'unsigned int') [-Wshorten-64-to-32]
                ps->ps_drop = pd->curr_stats.ierrors;
                            ~ ~~~~~~~~~~~~~~~^~~~~~~
pcap-dpdk.c:477:34: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'u_int' (aka 'unsigned int') [-Wshorten-64-to-32]
                ps->ps_ifdrop = pd->curr_stats.imissed;
                              ~ ~~~~~~~~~~~~~~~^~~~~~~
pcap-dpdk.c:550:13: warning: implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'int' [-Wshorten-64-to-32]
        for (int i=prefix_len; device[i]; i++){
                 ~ ^~~~~~~~~~

@fxlb
Member
fxlb commented 3 days ago
Other warnings with GCC 12.2:

pcap-dpdk.c: In function 'pcap_dpdk_activate':
pcap-dpdk.c:785:52: warning: '%s' directive output may be truncated writing up to 255 bytes into a region of size 236 [-Wformat-truncation=]
  785 |                             "Can't open device %s: %s",
      |                                                    ^~
  786 |                             p->opt.device, dpdk_pre_init_errbuf);
      |                                            ~~~~~~~~~~~~~~~~~~~~
pcap-dpdk.c:784:25: note: 'snprintf' output 21 or more bytes (assuming 276) into a destination of size 256
  784 |                         snprintf(p->errbuf, PCAP_ERRBUF_SIZE,
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  785 |                             "Can't open device %s: %s",
      |                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~
  786 |                             p->opt.device, dpdk_pre_init_errbuf);
      |                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
pcap-dpdk.c: In function 'pcap_dpdk_findalldevs':
pcap-dpdk.c:1037:59: warning: '%s' directive output may be truncated writing up to 255 bytes into a region of size 227 [-Wformat-truncation=]
 1037 |                             "Can't look for DPDK devices: %s",
      |                                                           ^~
 1038 |                             dpdk_pre_init_errbuf);
      |                             ~~~~~~~~~~~~~~~~~~~~           
pcap-dpdk.c:1036:25: note: 'snprintf' output between 30 and 285 bytes into a destination of size 256
 1036 |                         snprintf(ebuf, PCAP_ERRBUF_SIZE,
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1037 |                             "Can't look for DPDK devices: %s",
      |                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1038 |                             dpdk_pre_init_errbuf);
      |                             ~~~~~~~~~~~~~~~~~~~~~

@idogoshen
Contributor
Author
idogoshen commented 3 days ago
@fxlb This are “old” warnings and not related to this patch
Attached make output for latest release 1.10.5 with same warnings

libpcap-1.10.5-make.txt

@fxlb
Member
fxlb commented 3 days ago
OK. Could a DPDK expert fix these warnings and perform non-regression testing?

@guyharris
Member
guyharris commented 2 days ago
Many warnings with clang-16:

pcap-dpdk.c:335:15: warning: variable 'gather_len' set but not used [-Wunused-but-set-variable]
        unsigned int gather_len =0;
                     ^
pcap-dpdk.c:474:32: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'u_int' (aka 'unsigned int') [-Wshorten-64-to-32]
                ps->ps_recv = pd->curr_stats.ipackets;
                            ~ ~~~~~~~~~~~~~~~^~~~~~~~
pcap-dpdk.c:475:32: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'u_int' (aka 'unsigned int') [-Wshorten-64-to-32]
                ps->ps_drop = pd->curr_stats.ierrors;
                            ~ ~~~~~~~~~~~~~~~^~~~~~~
pcap-dpdk.c:477:34: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'u_int' (aka 'unsigned int') [-Wshorten-64-to-32]
                ps->ps_ifdrop = pd->curr_stats.imissed;
                              ~ ~~~~~~~~~~~~~~~^~~~~~~
pcap-dpdk.c:550:13: warning: implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'int' [-Wshorten-64-to-32]
        for (int i=prefix_len; device[i]; i++){
                 ~ ^~~~~~~~~~

Ultimately, this requires a new API for getting capture statistics that uses 64-bit numbers. (That API should probably supply the equivalent of a pcapNowGeneric Interface Statistics Block; that would make it easier to add new statistics and to indicate which statistics are, and aren't, available.)

In the short term, we should probably just cast the statistics to bpf_u_int32. (I don't know what code using libpcap would break if we use C99 integer types and included <stdtypes.h> in pcap.h.)

@guyharris
Member
guyharris commented 2 days ago
Other warnings with GCC 12.2:

pcap-dpdk.c: In function 'pcap_dpdk_activate':
pcap-dpdk.c:785:52: warning: '%s' directive output may be truncated writing up to 255 bytes into a region of size 236 [-Wformat-truncation=]
  785 |                             "Can't open device %s: %s",
      |                                                    ^~
  786 |                             p->opt.device, dpdk_pre_init_errbuf);
      |                                            ~~~~~~~~~~~~~~~~~~~~
pcap-dpdk.c:784:25: note: 'snprintf' output 21 or more bytes (assuming 276) into a destination of size 256
  784 |                         snprintf(p->errbuf, PCAP_ERRBUF_SIZE,
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  785 |                             "Can't open device %s: %s",
      |                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~
  786 |                             p->opt.device, dpdk_pre_init_errbuf);
      |                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
pcap-dpdk.c: In function 'pcap_dpdk_findalldevs':
pcap-dpdk.c:1037:59: warning: '%s' directive output may be truncated writing up to 255 bytes into a region of size 227 [-Wformat-truncation=]
 1037 |                             "Can't look for DPDK devices: %s",
      |                                                           ^~
 1038 |                             dpdk_pre_init_errbuf);
      |                             ~~~~~~~~~~~~~~~~~~~~           
pcap-dpdk.c:1036:25: note: 'snprintf' output between 30 and 285 bytes into a destination of size 256
 1036 |                         snprintf(ebuf, PCAP_ERRBUF_SIZE,
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1037 |                             "Can't look for DPDK devices: %s",
      |                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1038 |                             dpdk_pre_init_errbuf);
      |                             ~~~~~~~~~~~~~~~~~~~~~

This one really needs us to have a new API that returns un-truncated error message strings. We'd use pcapint_asprintf() to generate error strings and store a pointer to the last error string in the pcap_t, have pcap_geterr_long() (or whatever its name is) return an untruncated error string, and have pcap_geterr() copy the saved error string to the fixed-length buffer (and make sure it doesn't truncate any UTF-8 strings).

[guyharris]

pcap-dpdk.c:335:15: warning: variable 'gather_len' set but not used [-Wunused-but-set-variable]
        unsigned int gather_len =0;
                     ^

Somebody familiar with the DPDK library should look at that to determine why dpdk_gather_data() is being called, and its return value is being assigned to gather_len, but that return value is never used, and to decide whether the fix is to 1) not save the return value of dpdk_gather_data() or 2) use that return value in some fashion.

[guyharris]

pcap-dpdk.c:550:13: warning: implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'int' [-Wshorten-64-to-32]
        for (int i=prefix_len; device[i]; i++){
                 ~ ^~~~~~~~~~

That one is probably just the result of a habit of using int for array indices; I changed int to size_t in bc34ac5.

[guyharris]

pcap-dpdk.c:474:32: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'u_int' (aka 'unsigned int') [-Wshorten-64-to-32]
                ps->ps_recv = pd->curr_stats.ipackets;
                            ~ ~~~~~~~~~~~~~~~^~~~~~~~
pcap-dpdk.c:475:32: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'u_int' (aka 'unsigned int') [-Wshorten-64-to-32]
                ps->ps_drop = pd->curr_stats.ierrors;
                            ~ ~~~~~~~~~~~~~~~^~~~~~~
pcap-dpdk.c:477:34: warning: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'u_int' (aka 'unsigned int') [-Wshorten-64-to-32]
                ps->ps_ifdrop = pd->curr_stats.imissed;
                              ~ ~~~~~~~~~~~~~~~^~~~~~~

Fixing those requires a "get statistics" API that returns 64-bit integers.

Suppressing the warnings just requires casting those to u_int before assigning them; I did the latter in fafab22

[idogoshen]

dpdk_gather_data() is called because dpdk pkt may be segmented over a chain of mbufs
refer to https://doc.dpdk.org/guides/prog_guide/mbuf_lib.html#figure-mbuf2

struct __rte_cache_aligned rte_mbuf {
...
            uint32_t pkt_len;         /**< Total pkt len: sum of all segments. */
            uint16_t data_len;        /**< Amount of data in segment buffer. */
...
}

In the common case the entire packet fits into one mbuf and this is covered by pcap-dpdk.c#L399 if (m->nb_segs == 1)and saves the memcpy.
In the segmented case the packet should be serialized and this is what dpdk_gather_data() do.
(actually it would be better to use dpdk rte_pktmbuf_read() api for it instead of doing all this “manually”)

Having said that to suppress the warning I think it would be enough to just

1. not save the return value of dpdk_gather_data()