[ovs-dev] [PATCH 2/7] datapath: Eliminate memset() from flow_extract.

Jesse Gross jesse at nicira.com
Thu Jun 19 21:54:49 UTC 2014


On Thu, Jun 19, 2014 at 2:01 PM, Pravin Shelar <pshelar at nicira.com> wrote:
> On Thu, Jun 19, 2014 at 1:46 PM, Jesse Gross <jesse at nicira.com> wrote:
>> On Thu, Jun 19, 2014 at 1:30 PM, Pravin Shelar <pshelar at nicira.com> wrote:
>>> On Tue, Jun 10, 2014 at 4:47 PM, Jesse Gross <jesse at nicira.com> wrote:
>>>> @@ -520,18 +534,24 @@ int ovs_flow_extract(struct sk_buff *skb, u16 in_port, struct sw_flow_key *key)
>>>>                                 key->tp.src = tcp->source;
>>>>                                 key->tp.dst = tcp->dest;
>>>>                                 key->tp.flags = TCP_FLAGS_BE16(tcp);
>>>> +                       } else {
>>>> +                               memset(&key->tp, 0, sizeof(key->tp));
>>>>                         }
>>>>                 } else if (key->ip.proto == IPPROTO_UDP) {
>>>>                         if (udphdr_ok(skb)) {
>>>>                                 struct udphdr *udp = udp_hdr(skb);
>>>>                                 key->tp.src = udp->source;
>>>>                                 key->tp.dst = udp->dest;
>>>> +                       } else {
>>>> +                               memset(&key->tp, 0, sizeof(key->tp));
>>>>                         }
>>> if we combine above if condition we can have common memset for key->tp.
>>
>> I'm not sure I understand what you mean here - each protocol has a
>> different check for the header being present. What would a combined
>> version look like?
>
> I was thinking of something like:
>
> if (key->ip.proto == IPPROTO_TCP && tcphdr_ok(skb)) {
>      struct tcphdr *tcp = tcp_hdr(skb);
>      key->tp.src = tcp->source;
>      key->tp.dst = tcp->dest;
>      key->tp.flags = TCP_FLAGS_BE16(tcp);
> } else if (key->ip.proto == IPPROTO_UDP &&udphdr_ok(skb)) {
>      .....
>      .....
> }else if (...) {
>      .....
> } else {
>      memset(&key->tp, 0, sizeof(key->tp));
> }

OK, I see what you mean now. It does save some code but I guess it
seems a little less obvious to me what is going on. This would change
the condition for clearing the fields from there being an error to not
having a valid protocol in the list. In practice this is essentially
the same but logically it's a little different.



More information about the dev mailing list