On 12/11/2015 04:29 PM, Joe Stringer wrote:
> On 10 December 2015 at 09:12, Russell Bryant <russ...@ovn.org> wrote:
>> A previous commit fixed this code to match changes to the conntrack
>> state bit assignments. This patch further updates the code to use
>> the defined constants to ensure this code adapts automatically to any
>> possible future changes.
>>
>> Signed-off-by: Russell Bryant <russ...@ovn.org>
>> Requested-by: Joe Stringer <j...@ovn.org>
>
> Thanks, minor comment below.
>
>> lib/packets.h | 25 +++++++++++++++++--------
>> ovn/controller/lflow.c | 22 +++++++++++++++-------
>> 2 files changed, 32 insertions(+), 15 deletions(-)
>>
>> diff --git a/lib/packets.h b/lib/packets.h
>> index edf140b..c50e316 100644
>> --- a/lib/packets.h
>> +++ b/lib/packets.h
>> @@ -733,14 +733,23 @@ struct tcp_header {
>> BUILD_ASSERT_DECL(TCP_HEADER_LEN == sizeof(struct tcp_header));
>>
>> /* Connection states */
>> -#define CS_NEW 0x01
>> -#define CS_ESTABLISHED 0x02
>> -#define CS_RELATED 0x04
>> -#define CS_REPLY_DIR 0x08
>> -#define CS_INVALID 0x10
>> -#define CS_TRACKED 0x20
>> -#define CS_SRC_NAT 0x40
>> -#define CS_DST_NAT 0x80
>> +#define CS_NEW_BIT 0
>> +#define CS_ESTABLISHED_BIT 1
>> +#define CS_RELATED_BIT 2
>> +#define CS_REPLY_DIR_BIT 3
>> +#define CS_INVALID_BIT 4
>> +#define CS_TRACKED_BIT 5
>> +#define CS_SRC_NAT_BIT 6
>> +#define CS_DST_NAT_BIT 7
>
> Would these be appropriate to be an enum instead?
Sure, I'll post a v2 in a minute. Here's the incremetal diff:
> diff --git a/lib/packets.h b/lib/packets.h
> index c50e316..36bd759 100644
> --- a/lib/packets.h
> +++ b/lib/packets.h
> @@ -733,23 +733,27 @@ struct tcp_header {
> BUILD_ASSERT_DECL(TCP_HEADER_LEN == sizeof(struct tcp_header));
>
> /* Connection states */
> -#define CS_NEW_BIT 0
> -#define CS_ESTABLISHED_BIT 1
> -#define CS_RELATED_BIT 2
> -#define CS_REPLY_DIR_BIT 3
> -#define CS_INVALID_BIT 4
> -#define CS_TRACKED_BIT 5
> -#define CS_SRC_NAT_BIT 6
> -#define CS_DST_NAT_BIT 7
> -
> -#define CS_NEW (1 << CS_NEW_BIT)
> -#define CS_ESTABLISHED (1 << CS_ESTABLISHED_BIT)
> -#define CS_RELATED (1 << CS_RELATED_BIT)
> -#define CS_REPLY_DIR (1 << CS_REPLY_DIR_BIT)
> -#define CS_INVALID (1 << CS_INVALID_BIT)
> -#define CS_TRACKED (1 << CS_TRACKED_BIT)
> -#define CS_SRC_NAT (1 << CS_SRC_NAT_BIT)
> -#define CS_DST_NAT (1 << CS_DST_NAT_BIT)
> +enum {
> + CS_NEW_BIT = 0,
> + CS_ESTABLISHED_BIT = 1,
> + CS_RELATED_BIT = 2,
> + CS_REPLY_DIR_BIT = 3,
> + CS_INVALID_BIT = 4,
> + CS_TRACKED_BIT = 5,
> + CS_SRC_NAT_BIT = 6,
> + CS_DST_NAT_BIT = 7,
> +};
I know the integers aren't necessary above, but I kind of like
specifying them anyway since the specific values are important.
> +
> +enum {
> + CS_NEW = (1 << CS_NEW_BIT),
> + CS_ESTABLISHED = (1 << CS_ESTABLISHED_BIT),
> + CS_RELATED = (1 << CS_RELATED_BIT),
> + CS_REPLY_DIR = (1 << CS_REPLY_DIR_BIT),
> + CS_INVALID = (1 << CS_INVALID_BIT),
> + CS_TRACKED = (1 << CS_TRACKED_BIT),
> + CS_SRC_NAT = (1 << CS_SRC_NAT_BIT),
> + CS_DST_NAT = (1 << CS_DST_NAT_BIT),
> +};
and I went ahead and used an enum here, as well.
--
Russell Bryant
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
