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.nord...@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)); > } > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev