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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
