On Thu, Jul 27, 2017 at 02:05:22PM -0700, Justin Pettit wrote:
>
> > On Jul 27, 2017, at 1:54 PM, Ben Pfaff <b...@ovn.org> wrote:
> >
> > On Thu, Jul 13, 2017 at 11:30:49PM -0700, Justin Pettit wrote:
> >> Signed-off-by: Justin Pettit <jpet...@ovn.org>
> >> ---
> >> ofproto/ofproto-dpif-rid.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
> >> index d546b150b938..26c2357007b2 100644
> >> --- a/ofproto/ofproto-dpif-rid.c
> >> +++ b/ofproto/ofproto-dpif-rid.c
> >> @@ -146,6 +146,7 @@ frozen_state_hash(const struct frozen_state *state)
> >> hash = hash_bytes64(ALIGNED_CAST(const uint64_t *,
> >> state->action_set),
> >> state->action_set_len, hash);
> >> }
> >> + hash = hash_int(state->ofpacts_len, hash);
> >> if (state->ofpacts_len) {
> >> hash = hash_bytes64(ALIGNED_CAST(const uint64_t *, state->ofpacts),
> >> state->ofpacts_len, hash);
> >
> > Can you explain the benefit of this change? hash_bytes64() already uses
> > the number of bytes hashed as one of the inputs to the hash.
>
> hash_bytes64() is only called if the action length is non-zero.
Yes, but even so, the hash should still distinguish states with no
actions from states with actions.
> However, I was on the fence about making this change, since it wasn't
> clear if it would be that valuable. The main reason was just to make
> it consistent with how "action_set" is handled right above it.
>
> I'm happy to drop the patch if you prefer.
I'd be more inclined to remove the hash of action_set_len, because it is
unnecessary for the same reason that hash of ofpacts_len is unnecessary.
Thanks,
Ben.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
