On Fri, Dec 9, 2016 at 8:20 AM, Daniele Di Proietto <[email protected]> wrote:
> The packet in userdata generated by ovn-controller when translating the > put_dhcpv6_opt action includes 16-bit integers. > > Currently these 16-bit integers are encoded with native endianness. > This is ok becase the only consumer of that userdata is ovn-controller > itself, but it means that the OpenFlow action we're generating might > not really be the same on different hosts. > > I think it's better to encode the userdata as big-endian, like the rest > of OpenFlow messages. > > This fixes a test failure on big-endian platforms, because the generated > OpenFlow bytes were different than expected (the expectation was > generated on a little endian machine). > > Now 'struct dhcp_opt6_header' is identical to 'struct > dhcpv6_opt_header', but I chose to keep them separate, because they > have different purposes. I also renamed the members to avoid confusion. > > I haven't tested this on a real setup. > > CC: Numan Siddique <[email protected]> > Reported-at: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=840770 > Signed-off-by: Daniele Di Proietto <[email protected]> > --- > Thanks. I tested it and it works as expected. Acked-by: Numan Siddique <[email protected]> > ovn/controller/pinctrl.c | 28 +++++++++++++--------------- > ovn/lib/actions.c | 19 ++++++++++++------- > ovn/lib/ovn-dhcp.h | 5 +++-- > tests/ovn.at | 8 ++++---- > 4 files changed, 32 insertions(+), 28 deletions(-) > > diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c > index db9e441..849fbce 100644 > --- a/ovn/controller/pinctrl.c > +++ b/ovn/controller/pinctrl.c > @@ -385,13 +385,13 @@ compose_out_dhcpv6_opts(struct ofpbuf *userdata, > return false; > } > > - uint8_t *userdata_opt_data = ofpbuf_try_pull(userdata, > - userdata_opt->len); > + size_t size = ntohs(userdata_opt->size); > + uint8_t *userdata_opt_data = ofpbuf_try_pull(userdata, size); > if (!userdata_opt_data) { > return false; > } > > - switch (userdata_opt->code) { > + switch (ntohs(userdata_opt->opt_code)) { > case DHCPV6_OPT_SERVER_ID_CODE: > { > /* The Server Identifier option carries a DUID > @@ -405,7 +405,7 @@ compose_out_dhcpv6_opts(struct ofpbuf *userdata, > out_dhcpv6_opts, sizeof *opt_server_id); > > opt_server_id->opt.code = htons(DHCPV6_OPT_SERVER_ID_CODE); > - opt_server_id->opt.len = htons(userdata_opt->len + 4); > + opt_server_id->opt.len = htons(size + 4); > opt_server_id->duid_type = htons(DHCPV6_DUID_LL); > opt_server_id->hw_type = htons(DHCPV6_HW_TYPE_ETH); > memcpy(&opt_server_id->mac, userdata_opt_data, > @@ -415,7 +415,7 @@ compose_out_dhcpv6_opts(struct ofpbuf *userdata, > > case DHCPV6_OPT_IA_ADDR_CODE: > { > - if (userdata_opt->len != sizeof(struct in6_addr)) { > + if (size != sizeof(struct in6_addr)) { > return false; > } > > @@ -444,9 +444,8 @@ compose_out_dhcpv6_opts(struct ofpbuf *userdata, > struct dhcpv6_opt_ia_addr *opt_ia_addr = ofpbuf_put_zeros( > out_dhcpv6_opts, sizeof *opt_ia_addr); > opt_ia_addr->opt.code = htons(DHCPV6_OPT_IA_ADDR_CODE); > - opt_ia_addr->opt.len = htons(userdata_opt->len + 8); > - memcpy(opt_ia_addr->ipv6.s6_addr, userdata_opt_data, > - userdata_opt->len); > + opt_ia_addr->opt.len = htons(size + 8); > + memcpy(opt_ia_addr->ipv6.s6_addr, userdata_opt_data, size); > opt_ia_addr->t1 = OVS_BE32_MAX; > opt_ia_addr->t2 = OVS_BE32_MAX; > break; > @@ -457,8 +456,8 @@ compose_out_dhcpv6_opts(struct ofpbuf *userdata, > struct dhcpv6_opt_header *opt_dns = ofpbuf_put_zeros( > out_dhcpv6_opts, sizeof *opt_dns); > opt_dns->code = htons(DHCPV6_OPT_DNS_SERVER_CODE); > - opt_dns->len = htons(userdata_opt->len); > - ofpbuf_put(out_dhcpv6_opts, userdata_opt_data, > userdata_opt->len); > + opt_dns->len = htons(size); > + ofpbuf_put(out_dhcpv6_opts, userdata_opt_data, size); > break; > } > > @@ -467,11 +466,10 @@ compose_out_dhcpv6_opts(struct ofpbuf *userdata, > struct dhcpv6_opt_header *opt_dsl = ofpbuf_put_zeros( > out_dhcpv6_opts, sizeof *opt_dsl); > opt_dsl->code = htons(DHCPV6_OPT_DOMAIN_SEARCH_CODE); > - opt_dsl->len = htons(userdata_opt->len + 2); > - uint8_t *data = ofpbuf_put_zeros(out_dhcpv6_opts, > - userdata_opt->len + 2); > - *data = userdata_opt->len; > - memcpy(data + 1, userdata_opt_data, userdata_opt->len); > + opt_dsl->len = htons(size + 2); > + uint8_t *data = ofpbuf_put_zeros(out_dhcpv6_opts, size + 2); > + *data = size; > + memcpy(data + 1, userdata_opt_data, size); > break; > } > > diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c > index df526c0..fa8f175 100644 > --- a/ovn/lib/actions.c > +++ b/ovn/lib/actions.c > @@ -1527,21 +1527,26 @@ encode_put_dhcpv6_option(const struct > ovnact_dhcp_option *o, > struct ofpbuf *ofpacts) > { > struct dhcp_opt6_header *opt = ofpbuf_put_uninit(ofpacts, sizeof > *opt); > - opt->code = o->option->code; > - > const union expr_constant *c = o->value.values; > size_t n_values = o->value.n_values; > + size_t size; > + > + opt->opt_code = htons(o->option->code); > + > if (!strcmp(o->option->type, "ipv6")) { > - opt->len = n_values * sizeof(struct in6_addr); > + size = n_values * sizeof(struct in6_addr); > + opt->size = htons(size); > for (size_t i = 0; i < n_values; i++) { > ofpbuf_put(ofpacts, &c[i].value.ipv6, sizeof(struct > in6_addr)); > } > } else if (!strcmp(o->option->type, "mac")) { > - opt->len = sizeof(struct eth_addr); > - ofpbuf_put(ofpacts, &c->value.mac, opt->len); > + size = sizeof(struct eth_addr); > + opt->size = htons(size); > + ofpbuf_put(ofpacts, &c->value.mac, size); > } else if (!strcmp(o->option->type, "str")) { > - opt->len = strlen(c->string); > - ofpbuf_put(ofpacts, c->string, opt->len); > + size = strlen(c->string); > + opt->size = htons(size); > + ofpbuf_put(ofpacts, c->string, size); > } > } > > diff --git a/ovn/lib/ovn-dhcp.h b/ovn/lib/ovn-dhcp.h > index ecfcc03..d5561ed 100644 > --- a/ovn/lib/ovn-dhcp.h > +++ b/ovn/lib/ovn-dhcp.h > @@ -109,9 +109,10 @@ dhcp_opts_destroy(struct hmap *dhcp_opts) > hmap_destroy(dhcp_opts); > } > > +/* Used in the OpenFlow PACKET_IN userdata */ > struct dhcp_opt6_header { > - uint16_t code; > - uint16_t len; > + ovs_be16 opt_code; > + ovs_be16 size; > }; > > /* Supported DHCPv6 Message Types */ > diff --git a/tests/ovn.at b/tests/ovn.at > index 38fd1e5..cb21210 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -950,17 +950,17 @@ put_nd(inport, nd.target, nd.sll); > > # put_dhcpv6_opts > reg1[0] = put_dhcpv6_opts(ia_addr = ae70::4, server_id = > 00:00:00:00:10:02); > - encodes as controller(userdata=00.00.00. > 05.00.00.00.00.00.01.de.10.00.00.00.40.05.00.10.00.ae.70.00. > 00.00.00.00.00.00.00.00.00.00.00.00.04.02.00.06.00.00.00.00. > 00.10.02,pause) > + encodes as controller(userdata=00.00.00. > 05.00.00.00.00.00.01.de.10.00.00.00.40.00.05.00.10.ae.70.00. > 00.00.00.00.00.00.00.00.00.00.00.00.04.00.02.00.06.00.00.00. > 00.10.02,pause) > reg1[0] = put_dhcpv6_opts(); > encodes as controller(userdata=00.00.00. > 05.00.00.00.00.00.01.de.10.00.00.00.40,pause) > reg1[0] = put_dhcpv6_opts(dns_server={ae70::1,ae70::2}); > formats as reg1[0] = put_dhcpv6_opts(dns_server = {ae70::1, ae70::2}); > - encodes as controller(userdata=00.00.00. > 05.00.00.00.00.00.01.de.10.00.00.00.40.17.00.20.00.ae.70.00. > 00.00.00.00.00.00.00.00.00.00.00.00.01.ae.70.00.00.00.00.00. > 00.00.00.00.00.00.00.00.02,pause) > + encodes as controller(userdata=00.00.00. > 05.00.00.00.00.00.01.de.10.00.00.00.40.00.17.00.20.ae.70.00. > 00.00.00.00.00.00.00.00.00.00.00.00.01.ae.70.00.00.00.00.00. > 00.00.00.00.00.00.00.00.02,pause) > reg1[0] = put_dhcpv6_opts(server_id=12:34:56:78:9a:bc, > dns_server={ae70::1,ae89::2}); > formats as reg1[0] = put_dhcpv6_opts(server_id = 12:34:56:78:9a:bc, > dns_server = {ae70::1, ae89::2}); > - encodes as controller(userdata=00.00.00. > 05.00.00.00.00.00.01.de.10.00.00.00.40.02.00.06.00.12.34.56. > 78.9a.bc.17.00.20.00.ae.70.00.00.00.00.00.00.00.00.00.00.00. > 00.00.01.ae.89.00.00.00.00.00.00.00.00.00.00.00.00.00.02,pause) > + encodes as controller(userdata=00.00.00. > 05.00.00.00.00.00.01.de.10.00.00.00.40.00.02.00.06.12.34.56. > 78.9a.bc.00.17.00.20.ae.70.00.00.00.00.00.00.00.00.00.00.00. > 00.00.01.ae.89.00.00.00.00.00.00.00.00.00.00.00.00.00.02,pause) > reg1[0] = put_dhcpv6_opts(domain_search = "ovn.org"); > - encodes as controller(userdata=00.00.00. > 05.00.00.00.00.00.01.de.10.00.00.00.40.18.00.07.00.6f.76.6e. > 2e.6f.72.67,pause) > + encodes as controller(userdata=00.00.00. > 05.00.00.00.00.00.01.de.10.00.00.00.40.00.18.00.07.6f.76.6e. > 2e.6f.72.67,pause) > reg1[0] = put_dhcpv6_opts(x = 1.2.3.4); > Syntax error at `x' expecting DHCPv6 option name. > reg1[0] = put_dhcpv6_opts(ia_addr=ae70::4, "hi"); > -- > 2.10.2 > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
