On Fri, May 6, 2016 at 12:35 PM, Jesse Gross <je...@kernel.org> wrote:
> On Thu, Apr 21, 2016 at 6:54 PM, Pravin B Shelar <pshe...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
