Re: [PATCH 1/4] lib/librte_port: add PCAP file support to source port

["Zhang, Roy Fan" <roy.fan.zhang@intel.com>]

Hi Panu,

Thank you for the careful review and comments.

On 28/01/2016 11:54, Panu Matilainen wrote:
> On 01/27/2016 07:39 PM, Fan Zhang wrote:
>> Originally, source ports in librte_port is an input port used as packet
>> generator. Similar to Linux kernel /dev/zero character device, it
>> generates null packets. This patch adds optional PCAP file support to
>> source port: instead of sending NULL packets, the source port generates
>> packets copied from a PCAP file. To increase the performance, the 
>> packets
>> in the file are loaded to memory initially, and copied to mbufs in 
>> circular
>> manner. Users can enable or disable this feature by setting
>> CONFIG_RTE_PORT_PCAP compiler option "y" or "n".
>>
>> Signed-off-by: Fan Zhang <roy.fan.zhang@intel.com>
>> Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
>> ---
>>   config/common_bsdapp                   |   1 +
>>   config/common_linuxapp                 |   1 +
>>   lib/librte_port/Makefile               |   4 +
>>   lib/librte_port/rte_port_source_sink.c | 190 
>> +++++++++++++++++++++++++++++++++
>>   lib/librte_port/rte_port_source_sink.h |   7 ++
>>   mk/rte.app.mk                          |   1 +
>>   6 files changed, 204 insertions(+)
>>
> [...]
>> +#ifdef RTE_PORT_PCAP
>> +
>> +/**
>> + * Load PCAP file, allocate and copy packets in the file to memory
>> + *
>> + * @param p
>> + *   Parameters for source port
>> + * @param port
>> + *   Handle to source port
>> + * @param socket_id
>> + *   Socket id where the memory is created
>> + * @return
>> + *   0 on SUCCESS
>> + *   error code otherwise
>> + */
>> +static int
>> +pcap_source_load(struct rte_port_source_params *p,
>> +        struct rte_port_source *port,
>> +        int socket_id)
>> +{
> [...]
>> +#else
>> +static int
>> +pcap_source_load(__rte_unused struct rte_port_source_params *p,
>> +        struct rte_port_source *port,
>> +        __rte_unused int socket_id)
>> +{
>> +    port->pkt_buff = NULL;
>> +    port->pkt_len = NULL;
>> +    port->pkts = NULL;
>> +    port->pkt_index = 0;
>> +
>> +    return 0;
>> +}
>> +#endif
>
> Same as in patch 3/4, shouldn't this return -ENOTSUP when pcap support 
> is not built in, instead of success?

Thank you for the suggestion. I will make the change in V2.

> [...]
>
>> diff --git a/lib/librte_port/rte_port_source_sink.h 
>> b/lib/librte_port/rte_port_source_sink.h
>> index 0f9be79..6f39bec 100644
>> --- a/lib/librte_port/rte_port_source_sink.h
>> +++ b/lib/librte_port/rte_port_source_sink.h
>> @@ -53,6 +53,13 @@ extern "C" {
>>   struct rte_port_source_params {
>>       /** Pre-initialized buffer pool */
>>       struct rte_mempool *mempool;
>> +    /** The full path of the pcap file to read packets from */
>> +    char *file_name;
>> +    /** The number of bytes to be read from each packet in the
>> +     *  pcap file. If this value is 0, the whole packet is read;
>> +     *  if it is bigger than packet size, the generated packets
>> +     *  will contain the whole packet */
>> +    uint32_t n_bytes_per_pkt;
>>   };
>
> This is a likely ABI-break. It "only" appends to the struct, which 
> might in some cases be okay but only when there's no sensible use for 
> the struct within arrays or embedded in structs. The ip_pipeline 
> example for example embeds struct rte_port_source_params within 
> another struct which is could be thought of as an indication that 
> other applications might be doing this as well.
>
> An ABI break for librte_port has not been announced for 2.3 so you'd 
> need to announce the intent to do so in 2.4 now, and then either wait 
> till post 2.3 or wrap this in CONFIG_RTE_NEXT_ABI.
>     - Panu -

Before Submitting the patch I have run validate-abi script, the 
validation results showed "compatible" on all libraries.

Also, the patch's added line in the rte_port_source_sink.c was ensured 
that, if the CONFIG_RTE_PORT_PCAP compiler option is set to "n" (by 
default), the added read-only elements in the new rte_source_params 
structure won't be touched.
CONFIG_RTE_PORT_PCAP compiler option lies in config/comm_bsdapp and 
config/com_linuxapp, and is freshly added in this patch.

If an application is built on top of latest release version of 
rte_port_source_sink library, it shall work fine with the new library if 
the new CONFIG_RTE_PORT_PCAP opition is left the default "n".
ip_pipeline example works fine this way.

I hope this changes your mind on breaking the ABI.

Best regards,
Fan