| Message ID | 20211119050857.973548-1-frode.nordahl@canonical.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] tests: Handle endianness in netlink policy test | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Hello Frode, This patch does appear to fix the test case on big-endian systems. On Fri, 2021-11-19 at 06:08 +0100, Frode Nordahl wrote: > The netlink policy unit test contains test fixture data that is > subject to endianness and currently fails on big endian systems. > > Add helper that ensures correct byte order for the affected data. > > Fixes: bfee9f6c0115 ("netlink: Add support for parsing link layer address.") > Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com> > --- > tests/test-netlink-policy.c | 60 +++++++++++++++++++++++-------------- > 1 file changed, 38 insertions(+), 22 deletions(-) > > diff --git a/tests/test-netlink-policy.c b/tests/test-netlink-policy.c > index 5f2bf7101..3050faebc 100644 > --- a/tests/test-netlink-policy.c > +++ b/tests/test-netlink-policy.c > @@ -32,6 +32,22 @@ _nla_len(const struct nlattr *nla) { > return nla->nla_len - NLA_HDRLEN; > } > > +/* The header part of the test fixture data is subject to endianness. This > + * helper makes sure they are put into the buffer in the right order. */ > +static void > +prepare_ofpbuf_fixture(struct ofpbuf *buf, uint8_t *data, size_t size) { > + uint16_t hword; > + > + ovs_assert(size > 4); > + > + ofpbuf_init(buf, size); > + hword = data[0] | data[1] << 8; This is pretty minor, but what do you think about using htobe16 instead? > + ofpbuf_put(buf, &hword, sizeof(hword)); > + hword = data[2] | data[3] << 8; > + ofpbuf_put(buf, &hword, sizeof(hword)); > + ofpbuf_put(buf, &data[4], size - sizeof(hword) * 2); > +} > + > static void > test_nl_policy_parse_ll_addr(struct ovs_cmdl_context *ctx OVS_UNUSED) { > struct nl_policy policy[] = { > @@ -41,7 +57,7 @@ test_nl_policy_parse_ll_addr(struct ovs_cmdl_context *ctx OVS_UNUSED) { > struct nlattr *attrs[ARRAY_SIZE(policy)]; > uint8_t fixture_nl_data_policy_short[] = { > /* too short according to policy */ > - 0x04, 0x00, 0x2a, 0x00, > + 0x04, 0x00, 0x2a, 0x00, 0x00, > }; > uint8_t fixture_nl_data_policy_long[] = { > /* too long according to policy */ > @@ -62,37 +78,37 @@ test_nl_policy_parse_ll_addr(struct ovs_cmdl_context *ctx OVS_UNUSED) { > /* valid policy but data neither eth_addr nor ib_addr */ > 0x0b, 0x00, 0x2a, 0x00, 0x00, 0x53, 0x00, 0x00, 0x00, 0x2a, 0x00, > }; > - struct ofpbuf *buf; > + struct ofpbuf buf; > > /* confirm policy fails with too short data */ > - buf = ofpbuf_clone_data(&fixture_nl_data_policy_short, > - sizeof(fixture_nl_data_policy_short)); > - ovs_assert(!nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy))); > - ofpbuf_delete(buf); > + prepare_ofpbuf_fixture(&buf, fixture_nl_data_policy_short, > + sizeof(fixture_nl_data_policy_short)); > + ovs_assert(!nl_policy_parse(&buf, 0, policy, attrs, ARRAY_SIZE(policy))); > + ofpbuf_uninit(&buf); > memset(&attrs, 0, sizeof(*attrs)); > > /* confirm policy fails with too long data */ > - buf = ofpbuf_clone_data(&fixture_nl_data_policy_long, > - sizeof(fixture_nl_data_policy_long)); > - ovs_assert(!nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy))); > - ofpbuf_delete(buf); > + prepare_ofpbuf_fixture(&buf, fixture_nl_data_policy_long, > + sizeof(fixture_nl_data_policy_long)); > + ovs_assert(!nl_policy_parse(&buf, 0, policy, attrs, ARRAY_SIZE(policy))); > + ofpbuf_uninit(&buf); > memset(&attrs, 0, sizeof(*attrs)); > > /* confirm policy passes and interpret valid ethernet lladdr */ > - buf = ofpbuf_clone_data(&fixture_nl_data_eth, > - sizeof(fixture_nl_data_eth)); > - ovs_assert(nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy))); > + prepare_ofpbuf_fixture(&buf, fixture_nl_data_eth, > + sizeof(fixture_nl_data_eth)); > + ovs_assert(nl_policy_parse(&buf, 0, policy, attrs, ARRAY_SIZE(policy))); > ovs_assert((_nla_len(attrs[42]) == sizeof(struct eth_addr))); > struct eth_addr eth_expect = ETH_ADDR_C(00,53,00,00,00,2a); > struct eth_addr eth_parsed = nl_attr_get_eth_addr(attrs[42]); > ovs_assert((!memcmp(ð_expect, ð_parsed, sizeof(struct eth_addr)))); > - ofpbuf_delete(buf); > + ofpbuf_uninit(&buf); > memset(&attrs, 0, sizeof(*attrs)); > > /* confirm policy passes and interpret valid infiniband lladdr */ > - buf = ofpbuf_clone_data(&fixture_nl_data_ib, > - sizeof(fixture_nl_data_ib)); > - ovs_assert(nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy))); > + prepare_ofpbuf_fixture(&buf, fixture_nl_data_ib, > + sizeof(fixture_nl_data_ib)); > + ovs_assert(nl_policy_parse(&buf, 0, policy, attrs, ARRAY_SIZE(policy))); > ovs_assert((_nla_len(attrs[42]) == sizeof(struct ib_addr))); > struct ib_addr ib_expect = { > .ia = { > @@ -102,18 +118,18 @@ test_nl_policy_parse_ll_addr(struct ovs_cmdl_context *ctx OVS_UNUSED) { > }; > struct ib_addr ib_parsed = nl_attr_get_ib_addr(attrs[42]); > ovs_assert((!memcmp(&ib_expect, &ib_parsed, sizeof(struct eth_addr)))); > - ofpbuf_delete(buf); > + ofpbuf_uninit(&buf); > memset(&attrs, 0, sizeof(*attrs)); > > /* confirm we're able to detect invalid data that passes policy check, this > * can happen because the policy defines the data to be between the > * currently known lladdr sizes of 6 (ETH_ALEN) and 20 (INFINIBAND_ALEN) */ > - buf = ofpbuf_clone_data(&fixture_nl_data_invalid, > - sizeof(fixture_nl_data_invalid)); > - ovs_assert(nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy))); > + prepare_ofpbuf_fixture(&buf, fixture_nl_data_invalid, > + sizeof(fixture_nl_data_invalid)); > + ovs_assert(nl_policy_parse(&buf, 0, policy, attrs, ARRAY_SIZE(policy))); > ovs_assert(_nla_len(attrs[42]) != sizeof(struct eth_addr) > && _nla_len(attrs[42]) != sizeof(struct ib_addr)); > - ofpbuf_delete(buf); > + ofpbuf_uninit(&buf); > memset(&attrs, 0, sizeof(*attrs)); > } >
On 11/25/21 19:49, Mike Pattrick wrote: > Hello Frode, > > This patch does appear to fix the test case on big-endian systems. > > On Fri, 2021-11-19 at 06:08 +0100, Frode Nordahl wrote: >> The netlink policy unit test contains test fixture data that is >> subject to endianness and currently fails on big endian systems. >> >> Add helper that ensures correct byte order for the affected data. >> >> Fixes: bfee9f6c0115 ("netlink: Add support for parsing link layer address.") >> Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com> >> --- >> tests/test-netlink-policy.c | 60 +++++++++++++++++++++++-------------- >> 1 file changed, 38 insertions(+), 22 deletions(-) >> >> diff --git a/tests/test-netlink-policy.c b/tests/test-netlink-policy.c >> index 5f2bf7101..3050faebc 100644 >> --- a/tests/test-netlink-policy.c >> +++ b/tests/test-netlink-policy.c >> @@ -32,6 +32,22 @@ _nla_len(const struct nlattr *nla) { >> return nla->nla_len - NLA_HDRLEN; >> } >> >> +/* The header part of the test fixture data is subject to endianness. This >> + * helper makes sure they are put into the buffer in the right order. */ >> +static void >> +prepare_ofpbuf_fixture(struct ofpbuf *buf, uint8_t *data, size_t size) { >> + uint16_t hword; >> + >> + ovs_assert(size > 4); >> + >> + ofpbuf_init(buf, size); >> + hword = data[0] | data[1] << 8; > > This is pretty minor, but what do you think about using htobe16 > instead? This doesn't sound very portable, so it's probably better to not use. > >> + ofpbuf_put(buf, &hword, sizeof(hword)); Frode, please, don't parenthesize the argument of 'sizeof' if that is not necessary. I know that it was like that in this file before, but since you're touching almost all the sizeofs in this file, it would be great to fix the style there as well. Best regards, Ilya Maximets.
On Fri, Dec 3, 2021 at 3:53 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 11/25/21 19:49, Mike Pattrick wrote: > > Hello Frode, > > > > This patch does appear to fix the test case on big-endian systems. > > > > On Fri, 2021-11-19 at 06:08 +0100, Frode Nordahl wrote: > >> The netlink policy unit test contains test fixture data that is > >> subject to endianness and currently fails on big endian systems. > >> > >> Add helper that ensures correct byte order for the affected data. > >> > >> Fixes: bfee9f6c0115 ("netlink: Add support for parsing link layer address.") > >> Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com> > >> --- > >> tests/test-netlink-policy.c | 60 +++++++++++++++++++++++-------------- > >> 1 file changed, 38 insertions(+), 22 deletions(-) > >> > >> diff --git a/tests/test-netlink-policy.c b/tests/test-netlink-policy.c > >> index 5f2bf7101..3050faebc 100644 > >> --- a/tests/test-netlink-policy.c > >> +++ b/tests/test-netlink-policy.c > >> @@ -32,6 +32,22 @@ _nla_len(const struct nlattr *nla) { > >> return nla->nla_len - NLA_HDRLEN; > >> } > >> > >> +/* The header part of the test fixture data is subject to endianness. This > >> + * helper makes sure they are put into the buffer in the right order. */ > >> +static void > >> +prepare_ofpbuf_fixture(struct ofpbuf *buf, uint8_t *data, size_t size) { > >> + uint16_t hword; > >> + > >> + ovs_assert(size > 4); > >> + > >> + ofpbuf_init(buf, size); > >> + hword = data[0] | data[1] << 8; > > > > This is pretty minor, but what do you think about using htobe16 > > instead? > > This doesn't sound very portable, so it's probably better to not use. Ack, I'll not introduce calls from endian.h here. I'll investigate if I can find other more human readable and still portable ways of dealing with this if that is a concern for the current proposal. > > > >> + ofpbuf_put(buf, &hword, sizeof(hword)); > > Frode, please, don't parenthesize the argument of 'sizeof' if that > is not necessary. I know that it was like that in this file before, > but since you're touching almost all the sizeofs in this file, it > would be great to fix the style there as well. Thank you for pointing that out, I will update the use of the `sizeof` operator in this file as part of this fix. I see that this is also mentioned in the style guide, I will make sure to study it in more detail. Cheers!
diff --git a/tests/test-netlink-policy.c b/tests/test-netlink-policy.c index 5f2bf7101..3050faebc 100644 --- a/tests/test-netlink-policy.c +++ b/tests/test-netlink-policy.c @@ -32,6 +32,22 @@ _nla_len(const struct nlattr *nla) { return nla->nla_len - NLA_HDRLEN; } +/* The header part of the test fixture data is subject to endianness. This + * helper makes sure they are put into the buffer in the right order. */ +static void +prepare_ofpbuf_fixture(struct ofpbuf *buf, uint8_t *data, size_t size) { + uint16_t hword; + + ovs_assert(size > 4); + + ofpbuf_init(buf, size); + hword = data[0] | data[1] << 8; + ofpbuf_put(buf, &hword, sizeof(hword)); + hword = data[2] | data[3] << 8; + ofpbuf_put(buf, &hword, sizeof(hword)); + ofpbuf_put(buf, &data[4], size - sizeof(hword) * 2); +} + static void test_nl_policy_parse_ll_addr(struct ovs_cmdl_context *ctx OVS_UNUSED) { struct nl_policy policy[] = { @@ -41,7 +57,7 @@ test_nl_policy_parse_ll_addr(struct ovs_cmdl_context *ctx OVS_UNUSED) { struct nlattr *attrs[ARRAY_SIZE(policy)]; uint8_t fixture_nl_data_policy_short[] = { /* too short according to policy */ - 0x04, 0x00, 0x2a, 0x00, + 0x04, 0x00, 0x2a, 0x00, 0x00, }; uint8_t fixture_nl_data_policy_long[] = { /* too long according to policy */ @@ -62,37 +78,37 @@ test_nl_policy_parse_ll_addr(struct ovs_cmdl_context *ctx OVS_UNUSED) { /* valid policy but data neither eth_addr nor ib_addr */ 0x0b, 0x00, 0x2a, 0x00, 0x00, 0x53, 0x00, 0x00, 0x00, 0x2a, 0x00, }; - struct ofpbuf *buf; + struct ofpbuf buf; /* confirm policy fails with too short data */ - buf = ofpbuf_clone_data(&fixture_nl_data_policy_short, - sizeof(fixture_nl_data_policy_short)); - ovs_assert(!nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy))); - ofpbuf_delete(buf); + prepare_ofpbuf_fixture(&buf, fixture_nl_data_policy_short, + sizeof(fixture_nl_data_policy_short)); + ovs_assert(!nl_policy_parse(&buf, 0, policy, attrs, ARRAY_SIZE(policy))); + ofpbuf_uninit(&buf); memset(&attrs, 0, sizeof(*attrs)); /* confirm policy fails with too long data */ - buf = ofpbuf_clone_data(&fixture_nl_data_policy_long, - sizeof(fixture_nl_data_policy_long)); - ovs_assert(!nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy))); - ofpbuf_delete(buf); + prepare_ofpbuf_fixture(&buf, fixture_nl_data_policy_long, + sizeof(fixture_nl_data_policy_long)); + ovs_assert(!nl_policy_parse(&buf, 0, policy, attrs, ARRAY_SIZE(policy))); + ofpbuf_uninit(&buf); memset(&attrs, 0, sizeof(*attrs)); /* confirm policy passes and interpret valid ethernet lladdr */ - buf = ofpbuf_clone_data(&fixture_nl_data_eth, - sizeof(fixture_nl_data_eth)); - ovs_assert(nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy))); + prepare_ofpbuf_fixture(&buf, fixture_nl_data_eth, + sizeof(fixture_nl_data_eth)); + ovs_assert(nl_policy_parse(&buf, 0, policy, attrs, ARRAY_SIZE(policy))); ovs_assert((_nla_len(attrs[42]) == sizeof(struct eth_addr))); struct eth_addr eth_expect = ETH_ADDR_C(00,53,00,00,00,2a); struct eth_addr eth_parsed = nl_attr_get_eth_addr(attrs[42]); ovs_assert((!memcmp(ð_expect, ð_parsed, sizeof(struct eth_addr)))); - ofpbuf_delete(buf); + ofpbuf_uninit(&buf); memset(&attrs, 0, sizeof(*attrs)); /* confirm policy passes and interpret valid infiniband lladdr */ - buf = ofpbuf_clone_data(&fixture_nl_data_ib, - sizeof(fixture_nl_data_ib)); - ovs_assert(nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy))); + prepare_ofpbuf_fixture(&buf, fixture_nl_data_ib, + sizeof(fixture_nl_data_ib)); + ovs_assert(nl_policy_parse(&buf, 0, policy, attrs, ARRAY_SIZE(policy))); ovs_assert((_nla_len(attrs[42]) == sizeof(struct ib_addr))); struct ib_addr ib_expect = { .ia = { @@ -102,18 +118,18 @@ test_nl_policy_parse_ll_addr(struct ovs_cmdl_context *ctx OVS_UNUSED) { }; struct ib_addr ib_parsed = nl_attr_get_ib_addr(attrs[42]); ovs_assert((!memcmp(&ib_expect, &ib_parsed, sizeof(struct eth_addr)))); - ofpbuf_delete(buf); + ofpbuf_uninit(&buf); memset(&attrs, 0, sizeof(*attrs)); /* confirm we're able to detect invalid data that passes policy check, this * can happen because the policy defines the data to be between the * currently known lladdr sizes of 6 (ETH_ALEN) and 20 (INFINIBAND_ALEN) */ - buf = ofpbuf_clone_data(&fixture_nl_data_invalid, - sizeof(fixture_nl_data_invalid)); - ovs_assert(nl_policy_parse(buf, 0, policy, attrs, ARRAY_SIZE(policy))); + prepare_ofpbuf_fixture(&buf, fixture_nl_data_invalid, + sizeof(fixture_nl_data_invalid)); + ovs_assert(nl_policy_parse(&buf, 0, policy, attrs, ARRAY_SIZE(policy))); ovs_assert(_nla_len(attrs[42]) != sizeof(struct eth_addr) && _nla_len(attrs[42]) != sizeof(struct ib_addr)); - ofpbuf_delete(buf); + ofpbuf_uninit(&buf); memset(&attrs, 0, sizeof(*attrs)); }
The netlink policy unit test contains test fixture data that is subject to endianness and currently fails on big endian systems. Add helper that ensures correct byte order for the affected data. Fixes: bfee9f6c0115 ("netlink: Add support for parsing link layer address.") Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com> --- tests/test-netlink-policy.c | 60 +++++++++++++++++++++++-------------- 1 file changed, 38 insertions(+), 22 deletions(-)