[ovs-dev] [PATCH v2 02/15] netdev: Return number of packet from netdev_pop_header()

[pravin shelar]

On Fri, May 6, 2016 at 12:35 PM, Jesse Gross <jesse at kernel.org> wrote:
> On Thu, Apr 21, 2016 at 6:54 PM, Pravin B Shelar <pshelar at ovn.org> wrote:
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 1e8a37c..5dcb862 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>>      case OVS_ACTION_ATTR_OUTPUT:
>> @@ -3775,8 +3774,12 @@ dp_execute_cb(void *aux_, struct dp_packet **packets, int cnt,
>>                     packets = tnl_pkt;
>>                  }
>>
>> -                err = netdev_pop_header(p->netdev, packets, cnt);
>> +                err = netdev_pop_header(p->netdev, packets, &cnt);
>> +                if (!cnt) {
>> +                    return;
>> +                }
>>                  if (!err) {
>
> I think that there is a difference in the handling of freeing packets
> based on the type of error that we encounter.
>
> If the protocol handler runs into an error on a single packet, it will
> call dp_packet_delete(). That seems correct to me.
>
> However, if tunnel header popping is completely unsupported then
> netdev_pop_handler() will return an error code. This will result in
> dp_netdev_drop_packets() being called, which will only free the
> packets if may_steal is false. This was presumably done because that's
> the only case where we clone but if may_steal is true then we're still
> expect to take ownership of the packet.
>
> This problem wasn't introduced in this patch but I think we could
> simplify things a little and avoid it. Right now, netdev_pop_header()
> effectively has two methods of indicating an error - the error code
> and cnt set to 0. We could make it just free the packets in the event
> that the pop action isn't supported and not make the caller worry
> about the details.

ok, I would add a patch to fix this issue later in the series.