[ovs-dev] [PATCH v4] packets: Use packet metadata initialization function instead of macro.

Gurucharan Shetty shettyg at nicira.com
Fri Mar 21 17:53:03 UTC 2014


On Fri, Mar 21, 2014 at 9:49 AM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
> Guru,
>
> Please find my comment below,
>
>   Jarno
>
> On Mar 20, 2014, at 6:13 PM, Gurucharan Shetty <shettyg at nicira.com> wrote:
>
>> Commit 03fbdf8d9c80a (lib/flow: Retain ODPP_NONE on flow_extract())
>> replaced packet metadata initialization function by a macro.
>> Visual studio does not like nested structure initialization that
>> is done in that macro.
>>
>> This commit, reverts the above mentioned commit partially by
>> re-introducing the metadata initialization function.
>>
>> CC: Jarno Rajahalme <jrajahalme at nicira.com>
>> Signed-off-by: Gurucharan Shetty <gshetty at nicira.com>
>> ---
>> lib/packets.c                 |   22 ++++++++++++++++++++++
>> lib/packets.h                 |    8 +++++---
>> ofproto/ofproto-dpif-upcall.c |    3 ++-
>> ofproto/ofproto-dpif.c        |    5 ++++-
>> 4 files changed, 33 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/packets.c b/lib/packets.c
>> index 65ba3f6..0143de6 100644
>> --- a/lib/packets.c
>> +++ b/lib/packets.c
>> @@ -976,3 +976,25 @@ packet_format_tcp_flags(struct ds *s, uint16_t tcp_flags)
>>         ds_put_cstr(s, "[800]");
>>     }
>> }
>> +
>> +void pkt_metadata_init(struct pkt_metadata *md, const struct flow_tnl *tnl,
>> +                       const uint32_t skb_priority,
>> +                       const uint32_t pkt_mark,
>> +                       const union flow_in_port *in_port)
>> +{
>> +    if (tnl) {
>> +        memcpy(&md->tunnel, tnl, sizeof(md->tunnel));
>> +    } else {
>> +        memset(&md->tunnel, 0, sizeof(md->tunnel));
>> +    }
>> +
>> +    md->skb_priority = skb_priority;
>> +    md->pkt_mark = pkt_mark;
>> +    md->in_port.odp_port = in_port ? in_port->odp_port : 0;
>> +}
>> +
>> +void pkt_metadata_from_flow(struct pkt_metadata *md, const struct flow *flow)
>> +{
>> +    pkt_metadata_init(md, &flow->tunnel, flow->skb_priority,
>> +                           flow->pkt_mark, &flow->in_port);
>> +}
>> diff --git a/lib/packets.h b/lib/packets.h
>> index 18a3b17..16fa66c 100644
>> --- a/lib/packets.h
>> +++ b/lib/packets.h
>> @@ -42,9 +42,11 @@ struct pkt_metadata {
>> #define PKT_METADATA_INITIALIZER(PORT) \
>>     (struct pkt_metadata){ { 0, 0, 0, 0, 0, 0}, 0, 0, {(PORT)} }
>>
>> -#define PKT_METADATA_INITIALIZER_FLOW(FLOW) \
>> -    (struct pkt_metadata){ (FLOW)->tunnel, (FLOW)->skb_priority, \
>> -            (FLOW)->pkt_mark, (FLOW)->in_port }
>> +void pkt_metadata_init(struct pkt_metadata *md, const struct flow_tnl *tnl,
>> +                       const uint32_t skb_priority,
>> +                       const uint32_t pkt_mark,
>> +                       const union flow_in_port *in_port);
>> +void pkt_metadata_from_flow(struct pkt_metadata *md, const struct flow *flow);
>>
>
> IMO this would better match the macro, can you try this one instead:
>
> static inline struct pkt_metadata pkt_metadata_from_flow(const struct flow *flow)
> {
>     struct pkt_metadata md;
>
>     md.tunnel = flow->tunnel;
>     md.skb_priority = flow->skb_priority;
>     md.pkt_mark = flow->pkt_mark;
>     md.in_port = flow->in_port;
>
>     return md;
> }
>
> I did not try this myself, hopefully no typos above. If this works, then the changes in packets.c are not necessary, and both call sites change to something like:
>
>> -            struct pkt_metadata md = PKT_METADATA_INITIALIZER_FLOW(&flow);
>> +            struct pkt_metadata md = pkt_metadata_from_flow(&flow);
Okay. I will send a v5.

>
>
> Regards,
>
>   Jarno
>
>
>> bool dpid_from_string(const char *s, uint64_t *dpidp);
>>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index 0048943..0c19dc0 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -1016,8 +1016,9 @@ handle_upcalls(struct handler *handler, struct list *upcalls)
>>         type = classify_upcall(upcall);
>>         if (type == MISS_UPCALL) {
>>             uint32_t hash;
>> -            struct pkt_metadata md = PKT_METADATA_INITIALIZER_FLOW(&flow);
>> +            struct pkt_metadata md;
>>
>> +            pkt_metadata_from_flow(&md, &flow);
>>             flow_extract(packet, &md, &miss->flow);
>>             hash = flow_hash(&miss->flow, 0);
>>             existing_miss = flow_miss_find(&misses, ofproto, &miss->flow,
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index 8ce439d..e33ad5e 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -3876,10 +3876,13 @@ parse_flow_and_packet(int argc, const char *argv[],
>>         if (!packet->size) {
>>             flow_compose(packet, flow);
>>         } else {
>> -            struct pkt_metadata md = PKT_METADATA_INITIALIZER_FLOW(flow);
>> +            union flow_in_port in_port = flow->in_port;
>> +            struct pkt_metadata md;
>>
>>             /* Use the metadata from the flow and the packet argument
>>              * to reconstruct the flow. */
>> +            pkt_metadata_init(&md, NULL, flow->skb_priority,
>> +                              flow->pkt_mark, &in_port);
>>             flow_extract(packet, &md, flow);
>>         }
>>     }
>> --
>> 1.7.9.5
>>
>



More information about the dev mailing list