[ovs-dev,2/3] ofproto-dpif-rid: Store tunnel metadata in frozen metadata directly.
diff mbox

Message ID F5C1200B-35E5-40FF-A9CC-AD3B6C3CD3E1@ovn.org
State Accepted
Headers show

Commit Message

Justin Pettit July 28, 2017, 2:26 a.m. UTC
> On Jul 27, 2017, at 4:24 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> On Thu, Jul 13, 2017 at 11:30:50PM -0700, Justin Pettit wrote:
>> "recirc_id_node" contains a 'state_metadata_tunnel' member field.  The
>> "frozen_metadata" structure used by "recird_id_node" had a 'tunnel'
>> member that always pointed to 'state_metadata_tunnel".  This commit just
>> stores the tunnel information directly in "frozen_metadata" instead of
>> accessing it through a pointer.
>> 
>> Signed-off-by: Justin Pettit <jpettit@ovn.org>
> 
> Would you mind adding something to the commit message to explain why
> this is a good thing?  I think I can guess, but rationale is important.
> 
> Acked-by: Ben Pfaff <blp@ovn.org>

Thanks.  I pushed this to master with some added rationale.  I also added the patch below to address some Travis errors when using an older version of gcc.

--Justin


-=-=-=-=-=-=-=-=-

Comments

Ben Pfaff July 28, 2017, 5:27 a.m. UTC | #1
On Thu, Jul 27, 2017 at 07:26:49PM -0700, Justin Pettit wrote:
> 
> > On Jul 27, 2017, at 4:24 PM, Ben Pfaff <blp@ovn.org> wrote:
> > 
> > On Thu, Jul 13, 2017 at 11:30:50PM -0700, Justin Pettit wrote:
> >> "recirc_id_node" contains a 'state_metadata_tunnel' member field.  The
> >> "frozen_metadata" structure used by "recird_id_node" had a 'tunnel'
> >> member that always pointed to 'state_metadata_tunnel".  This commit just
> >> stores the tunnel information directly in "frozen_metadata" instead of
> >> accessing it through a pointer.
> >> 
> >> Signed-off-by: Justin Pettit <jpettit@ovn.org>
> > 
> > Would you mind adding something to the commit message to explain why
> > this is a good thing?  I think I can guess, but rationale is important.
> > 
> > Acked-by: Ben Pfaff <blp@ovn.org>
> 
> Thanks.  I pushed this to master with some added rationale.  I also
> added the patch below to address some Travis errors when using an
> older version of gcc.

Sweet, I see that my suggestion worked ;-)
Justin Pettit July 28, 2017, 5:28 a.m. UTC | #2
> On Jul 27, 2017, at 10:27 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> On Thu, Jul 27, 2017 at 07:26:49PM -0700, Justin Pettit wrote:
>> 
>>> On Jul 27, 2017, at 4:24 PM, Ben Pfaff <blp@ovn.org> wrote:
>>> 
>>> On Thu, Jul 13, 2017 at 11:30:50PM -0700, Justin Pettit wrote:
>>>> "recirc_id_node" contains a 'state_metadata_tunnel' member field.  The
>>>> "frozen_metadata" structure used by "recird_id_node" had a 'tunnel'
>>>> member that always pointed to 'state_metadata_tunnel".  This commit just
>>>> stores the tunnel information directly in "frozen_metadata" instead of
>>>> accessing it through a pointer.
>>>> 
>>>> Signed-off-by: Justin Pettit <jpettit@ovn.org>
>>> 
>>> Would you mind adding something to the commit message to explain why
>>> this is a good thing?  I think I can guess, but rationale is important.
>>> 
>>> Acked-by: Ben Pfaff <blp@ovn.org>
>> 
>> Thanks.  I pushed this to master with some added rationale.  I also
>> added the patch below to address some Travis errors when using an
>> older version of gcc.
> 
> Sweet, I see that my suggestion worked ;-)

Nailed it.  Thanks!

--Justin

Patch
diff mbox

diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
index 013534c511f0..5355a83a0f88 100644
--- a/ofproto/ofproto-dpif-rid.c
+++ b/ofproto/ofproto-dpif-rid.c
@@ -291,9 +291,12 @@  recirc_alloc_id(struct ofproto_dpif *ofproto)
     struct frozen_state state = {
         .table_id = TBL_INTERNAL,
         .ofproto_uuid = ofproto->uuid,
-        .metadata = { .tunnel.ip_dst = htonl(0),
-                      .tunnel.ipv6_dst = in6addr_any,
-                      .in_port = OFPP_NONE },
+        .metadata = {
+            .tunnel = {
+                .ip_dst = htonl(0),
+                .ipv6_dst = in6addr_any,
+            },
+            .in_port = OFPP_NONE },
     };
     return recirc_alloc_id__(&state, frozen_state_hash(&state))->id;