On Thu, Jan 21, 2016 at 3:41 PM, Jesse Gross <je...@kernel.org> wrote:
> On Sun, Jan 10, 2016 at 11:18 PM, Pravin B Shelar <pshe...@nicira.com> wrote:
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index c746cc2..215e9b6 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -3529,8 +3528,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;
>> +                }
>
> Is this safe in the event that may_steal is false? It seems like the
> caller could free a packet that we are trying to hold.
>
if may_steal is false then we make a copy of the batch of packet, ref
dp_netdev_clone_pkt_batch().

>> diff --git a/lib/netdev.c b/lib/netdev.c
>> index e3b70b1..e16a3be 100644
>> --- a/lib/netdev.c
>> +++ b/lib/netdev.c
>> @@ -758,12 +758,18 @@ netdev_pop_header(struct netdev *netdev, struct 
>> dp_packet **buffers, int cnt)
>>      for (i = 0; i < cnt; i++) {
>>          int err;
>>
>> -        err = netdev->netdev_class->pop_header(buffers[i]);
>> +        err = netdev->netdev_class->pop_header(&buffers[i]);
>>          if (err) {
>>              dp_packet_clear(buffers[i]);
>>          }
>>      }
>
> The current mechanism of releasing a packet in the event of an error
> is a little hacky - it's just setting the size to zero and relying on
> the fact that dp_netdev_input() will drop packets that don't have an
> Ethernet header. It seems like with this we could do better and just
> remove it from the list.
>
ok, I will incorporate a fix in this patch.

> I also wonder if we could return a special error code to indicate that
> a packet was held. That way we would only have a single mechanism for
> a pop_header handler to report the status and we could just check the
> value here to avoid freeing the memory.

ok.

> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to