On Thu, May 7, 2020 at 11:27 PM Ankur Sharma <svc.mail....@nutanix.com>
wrote:
> From: Dhathri Purohith <dhathri.puroh...@nutanix.com>
>
> 1. Add support for DHCP domain search option (119)
> Domain search list is encoded according to the specifications in RFC
> 1035,
> section 4.1.4.
>
> 2. Fix the data type for DHCP option tftp_server (66)
> Currently, DHCP option is of type ipv4. But, according to RFC 2132,
> option 66 can be a hostname i.e, we should also accept string type.
> In order to be backward compatible, a new type called "host_id" is
> introduced, which accepts both ipv4 address and string. Type for DHCP
> option 66 is changed to "host_id" instead of ipv4.
> OVN northd code that updates the OVN southbound database is enhanced to
> consider the change in the type and code for DHCP option, so that the
> change in datatype is reflected.
>
> Signed-off-by: Dhathri Purohith <dhathri.puroh...@nutanix.com>
>
Hi Ankur and Dhathri,
Thanks for the patch. Overall the patch looks good to me.
The compilation is failing with the below error when configured with
"--enable-Werror --enable-sparse"
****
t -Wbool-compare -Wshift-negative-value -Wduplicated-cond -Wshadow
-Wmultistatement-macros -Wcast-align=strict -Werror -Werror -g -O2 -MT
lib/actions.lo -MD -MP -MF $depbase.Tpo -c -o lib/actions.lo
../lib/actions.c &&\
mv -f $depbase.Tpo $depbase.Plo
../ovn-sb.ovsschema:4: The checksum "712869723 21916" was calculated from
the schema file and does not match cksum field in the schema file - you
should probably update the version number and the checksum in the schema
file with the value listed here.
make[1]: *** [Makefile:3168: ovn-sb.ovsschema.stamp] Error 1
make[1]: *** Waiting for unfinished jobs....
libtool: compile: env REAL_CC=gcc "CHECK=sparse -Wsparse-error -I
/home/nusiddiq/workspace_cpp/openvswitch/ovn_split/ovs/include/sparse -m64
-I /usr/local/include " cgcc -target=x86_64 -DHAVE_CONFIG_H -I. -I.. -I
../include -I ../include -I ../ovn -I ./include -I
/home/nusiddiq/workspace_cpp/openvswitch/ovn_split/ovs/include -I
/home/nusiddiq/workspace_cpp/openvswitch/ovn_split/ovs/_gcc/include -I
/home/nusiddiq/workspace_cpp/openvswitch/ovn_split/ovs/lib -I
/home/nusiddiq/workspace_cpp/openvswitch/ovn_split/ovs/_gcc/lib -I
/home/nusiddiq/workspace_cpp/openvswitch/ovn_split/ovs -I
/home/nusiddiq/workspace_cpp/openvswitch/ovn_split/ovs/_gcc -I ../lib -I
./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith
-Wformat -Wformat-security -Wswitch-enum -Wunused-parameter
-Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition
-Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing
-Wswitch-bool -Wlogical-not-parentheses -Wsizeof-array-argument
-Wbool-compare -Wshift-negative-value -Wduplicated-cond -Wshadow
-Wmultistatement-macros -Wcast-align=strict -Werror -Werror -g -O2 -MT
lib/actions.lo -MD -MP -MF lib/.deps/actions.Tpo -c ../lib/actions.c -o
lib/actions.o
../lib/actions.c:2373:54: error: restricted ovs_be16 degrades to integer
make[1]: *** [Makefile:2016: lib/actions.lo] Error 1
*****
Looks like the patch needs rebase as there is schema checksum error.
This patch does 2 things. Can you please split into 2 patch series.
Thanks
Numan
> ---
> lib/actions.c | 112
> +++++++++++++++++++++++++++++++++++++++++++++++++++-
> lib/ovn-l7.h | 5 ++-
> northd/ovn-northd.c | 8 +++-
> ovn-sb.ovsschema | 3 +-
> ovn-sb.xml | 24 +++++++++++
> tests/ovn.at | 9 +++++
> tests/test-ovn.c | 3 +-
> 7 files changed, 159 insertions(+), 5 deletions(-)
>
> diff --git a/lib/actions.c b/lib/actions.c
> index 021a376..02068b1 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -1943,7 +1943,12 @@ parse_gen_opt(struct action_context *ctx, struct
> ovnact_gen_option *o,
> return;
> }
>
> - if (!strcmp(o->option->type, "str")) {
> + if (!strcmp(o->option->type, "host_id")) {
> + return;
> + }
> +
> + if (!strcmp(o->option->type, "str") ||
> + !strcmp(o->option->type, "domains")) {
> if (o->value.type != EXPR_C_STRING) {
> lexer_error(ctx->lexer, "%s option %s requires string value.",
> opts_type, o->option->name);
> @@ -2270,6 +2275,111 @@ encode_put_dhcpv4_option(const struct
> ovnact_gen_option *o,
> } else if (!strcmp(o->option->type, "str")) {
> opt_header[1] = strlen(c->string);
> ofpbuf_put(ofpacts, c->string, opt_header[1]);
> + } else if (!strcmp(o->option->type, "host_id")) {
> + if (o->value.type == EXPR_C_STRING) {
> + opt_header[1] = strlen(c->string);
> + ofpbuf_put(ofpacts, c->string, opt_header[1]);
> + } else {
> + opt_header[1] = sizeof(ovs_be32);
> + ofpbuf_put(ofpacts, &c->value.ipv4, sizeof(ovs_be32));
> + }
> + } else if (!strcmp(o->option->type, "domains")) {
> + /* Please refer to RFC 1035, section 4.1.4 for the format of
> encoding
> + * domain names. Below is an example for encoding a search list
> + * consisting of the "abc.com" and "xyz.abc.com".
> + *
> + *
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> + * |119|14 | 3 |'a'|'b'|'c'| 3 |'c'|'o'|'m'| 0
> |'x'|'y'|'z'|xC0|x00|
> + *
> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
> + *
> + * The encoding of "abc.com" ends with 0 to mark the end of the
> + * domain name as required by RFC 1035.
> + *
> + * The encoding of "xyz" (for "xyz.abc.com") ends with the
> two-octet
> + * compression pointer C000 (hex), which points to offset 0 where
> + * another validly encoded domain name can be found to complete
> + * the name ("abc.com").
> + */
> + typedef struct namemap_node {
> + struct hmap_node node;
> + uint16_t offset;
> + char *name;
> + } namemap_node;
> + struct hmap namemap;
> + hmap_init(&namemap);
> + namemap_node *ref;
> + /* Encoding adds 2 bytes (one for length and one for delimiter)
> for
> + * every domain name that is unique. If all the domain names are
> unique
> + * (which probably never happens in real world), then encoded
> string
> + * could be longer than the original string. Just to be on the
> safer
> + * side, allocate the (approx.) worst case length here.
> + */
> + uint8_t *dns_encoded = xzalloc(2 * strlen(c->string));
> + uint8_t encode_offset = 0;
> + char *domain_list = xstrdup(c->string), *dom_ptr = NULL;
> + for (char *domain = strtok_r(domain_list, ",", &dom_ptr);
> + domain != NULL;
> + domain = strtok_r(NULL, ",", &dom_ptr)) {
> + if (strlen(domain) > DOMAIN_NAME_MAX_LEN) {
> + VLOG_WARN("Domain names longer than 255 characters are
> not"
> + "supported");
> + goto out;
> + }
> + char *label_ptr = NULL, *label;
> + for (label = strtok_r(domain, ".", &label_ptr);
> + label != NULL;
> + label = strtok_r(NULL, ".", &label_ptr)) {
> + ref = NULL;
> + /* Check if we have already encoded this label and
> + * fill in the reference, if yes. */
> + uint32_t label_hash = hash_string(label, 0);
> + HMAP_FOR_EACH_IN_BUCKET (ref, node, label_hash, &namemap)
> {
> + if (!strcmp(label, ref->name)) {
> + uint16_t temp = (0xc0 | htons(ref->offset));
> + memcpy(dns_encoded + encode_offset, &temp,
> + sizeof(temp));
> + encode_offset += sizeof(temp);
> + break;
> + }
> + }
> + /* Break, since we have already filled the offset for this
> + * domain. */
> + if (ref != NULL) {
> + break;
> + } else {
> + /* The label was not encoded before, encode it now
> and add
> + * the offset to the namemap map. */
> + ref = xzalloc(sizeof *ref);
> + ref->name = xstrdup(label);
> + ref->offset = encode_offset;
> + hmap_insert(&namemap, &ref->node, label_hash);
> +
> + uint8_t len = strlen(label);
> + memcpy(dns_encoded + encode_offset, &len,
> sizeof(uint8_t));
> + encode_offset += sizeof(uint8_t);
> + memcpy(dns_encoded + encode_offset, label, len);
> + encode_offset += len;
> + }
> + }
> + /* Add the end marker (0 byte) to determine the end of the
> + * domain. */
> + if (label == NULL) {
> + uint8_t end = 0;
> + memcpy(dns_encoded + encode_offset, &end,
> sizeof(uint8_t));
> + encode_offset += sizeof(uint8_t);
> + }
> + }
> + opt_header[1] = encode_offset;
> + ofpbuf_put(ofpacts, dns_encoded, encode_offset);
> +
> + out:
> + HMAP_FOR_EACH_POP (ref, node, &namemap) {
> + free(ref->name);
> + free(ref);
> + }
> + hmap_destroy(&namemap);
> + free(domain_list);
> + free(dns_encoded);
> }
> }
>
> diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h
> index cc63d82..f34bc0d 100644
> --- a/lib/ovn-l7.h
> +++ b/lib/ovn-l7.h
> @@ -34,6 +34,7 @@ struct gen_opts_map {
> size_t code;
> };
>
> +#define DOMAIN_NAME_MAX_LEN 255
> #define DHCP_BROADCAST_FLAG 0x8000
>
> #define DHCP_OPTION(NAME, CODE, TYPE) \
> @@ -57,7 +58,7 @@ struct gen_opts_map {
> #define DHCP_OPT_NIS_SERVER DHCP_OPTION("nis_server", 41, "ipv4")
> #define DHCP_OPT_NTP_SERVER DHCP_OPTION("ntp_server", 42, "ipv4")
> #define DHCP_OPT_SERVER_ID DHCP_OPTION("server_id", 54, "ipv4")
> -#define DHCP_OPT_TFTP_SERVER DHCP_OPTION("tftp_server", 66, "ipv4")
> +#define DHCP_OPT_TFTP_SERVER DHCP_OPTION("tftp_server", 66, "host_id")
>
> #define DHCP_OPT_CLASSLESS_STATIC_ROUTE \
> DHCP_OPTION("classless_static_route", 121, "static_routes")
> @@ -81,6 +82,8 @@ struct gen_opts_map {
> #define DHCP_OPT_PATH_PREFIX DHCP_OPTION("path_prefix", 210, "str")
> #define DHCP_OPT_TFTP_SERVER_ADDRESS \
> DHCP_OPTION("tftp_server_address", 150, "ipv4")
> +#define DHCP_OPT_DOMAIN_SEARCH_LIST \
> + DHCP_OPTION("domain_search_list", 119, "domains")
>
> #define DHCP_OPT_ARP_CACHE_TIMEOUT \
> DHCP_OPTION("arp_cache_timeout", 35, "uint32")
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 3324d20..cc8a7e6 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -11356,6 +11356,7 @@ static struct gen_opts_map supported_dhcp_opts[] =
> {
> DHCP_OPT_DOMAIN_NAME,
> DHCP_OPT_ARP_CACHE_TIMEOUT,
> DHCP_OPT_TCP_KEEPALIVE_INTERVAL,
> + DHCP_OPT_DOMAIN_SEARCH_LIST,
> };
>
> static struct gen_opts_map supported_dhcpv6_opts[] = {
> @@ -11380,7 +11381,12 @@ check_and_add_supported_dhcp_opts_to_sb_db(struct
> northd_context *ctx)
> struct gen_opts_map *dhcp_opt =
> dhcp_opts_find(&dhcp_opts_to_add, opt_row->name);
> if (dhcp_opt) {
> - hmap_remove(&dhcp_opts_to_add, &dhcp_opt->hmap_node);
> + if (!strcmp(dhcp_opt->type, opt_row->type) &&
> + dhcp_opt->code == opt_row->code) {
> + hmap_remove(&dhcp_opts_to_add, &dhcp_opt->hmap_node);
> + } else {
> + sbrec_dhcp_options_delete(opt_row);
> + }
> } else {
> sbrec_dhcp_options_delete(opt_row);
> }
> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
> index d89f8db..d1c1e51 100644
> --- a/ovn-sb.ovsschema
> +++ b/ovn-sb.ovsschema
> @@ -214,7 +214,8 @@
> "type": {"key": {
> "type": "string",
> "enum": ["set", ["bool", "uint8", "uint16",
> "uint32",
> - "ipv4", "static_routes",
> "str"]]}}}},
> + "ipv4", "static_routes", "str",
> + "host_id", "domains"]]}}}},
> "isRoot": true},
> "DHCPv6_Options": {
> "columns": {
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index 3aa7cd4..ddd8bff 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -3208,6 +3208,30 @@ tcp.flags = RST;
> Example. "name=host_name", "code=12", "type=str".
> </p>
> </dd>
> +
> + <dt><code>value: host_id</code></dt>
> + <dd>
> + <p>
> + This indicates that the value of the DHCP option is a host_id.
> + It can either be a host_name or an IP address.
> + </p>
> +
> + <p>
> + Example. "name=tftp_server", "code=66", "type=host_id".
> + </p>
> + </dd>
> +
> + <dt><code>value: domains</code></dt>
> + <dd>
> + <p>
> + This indicates that the value of the DHCP option is a domain
> name
> + or a comma separated list of domain names.
> + </p>
> +
> + <p>
> + Example. "name=domain_search_list", "code=119",
> "type=domains".
> + </p>
> + </dd>
> </dl>
> </column>
> </table>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index f168b14..7e2cd75 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1195,6 +1195,15 @@ reg2[5] =
> put_dhcp_opts(offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.254.0,m
> reg0[15] =
> put_dhcp_opts(offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.255.0,mtu=1400,ip_forward_enable=1,default_ttl=121,dns_server={8.8.8.8,7.7.7.7},classless_static_route={
> 30.0.0.0/24,10.0.0.4,40.0.0.0/16,10.0.0.6,0.0.0.0/0,10.0.0.1
> },ethernet_encap=1,router_discovery=0,tftp_server_address={10.0.0.4,10.0.0.5},arp_cache_timeout=10,tcp_keepalive_interval=10);
> formats as reg0[15] = put_dhcp_opts(offerip = 10.0.0.4, router =
> 10.0.0.1, netmask = 255.255.255.0, mtu = 1400, ip_forward_enable = 1,
> default_ttl = 121, dns_server = {8.8.8.8, 7.7.7.7}, classless_static_route
> = {30.0.0.0/24, 10.0.0.4, 40.0.0.0/16, 10.0.0.6, 0.0.0.0/0, 10.0.0.1},
> ethernet_encap = 1, router_discovery = 0, tftp_server_address = {10.0.0.4,
> 10.0.0.5}, arp_cache_timeout = 10, tcp_keepalive_interval = 10);
> encodes as
> controller(userdata=00.00.00.02.00.00.00.00.00.01.de.10.00.00.00.6f.0a.00.00.04.03.04.0a.00.00.01.01.04.ff.ff.ff.00.1a.02.05.78.13.01.01.17.01.79.06.08.08.08.08.08.07.07.07.07.79.14.18.1e.00.00.0a.00.00.04.10.28.00.0a.00.00.06.00.0a.00.00.01.24.01.01.1f.01.00.96.08.0a.00.00.04.0a.00.00.05.23.04.00.00.00.0a.26.04.00.00.00.0a,pause)
> +reg0[15] =
> put_dhcp_opts(offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.255.0,mtu=1400,ip_forward_enable=1,default_ttl=121,dns_server={8.8.8.8,7.7.7.7},classless_static_route={
> 30.0.0.0/24,10.0.0.4,40.0.0.0/16,10.0.0.6,0.0.0.0/0,10.0.0.1
> },ethernet_encap=1,router_discovery=0,tftp_server=10.0.0.10);
> + formats as reg0[15] = put_dhcp_opts(offerip = 10.0.0.4, router =
> 10.0.0.1, netmask = 255.255.255.0, mtu = 1400, ip_forward_enable = 1,
> default_ttl = 121, dns_server = {8.8.8.8, 7.7.7.7}, classless_static_route
> = {30.0.0.0/24, 10.0.0.4, 40.0.0.0/16, 10.0.0.6, 0.0.0.0/0, 10.0.0.1},
> ethernet_encap = 1, router_discovery = 0, tftp_server = 10.0.0.10);
> + encodes as
> controller(userdata=00.00.00.02.00.00.00.00.00.01.de.10.00.00.00.6f.0a.00.00.04.03.04.0a.00.00.01.01.04.ff.ff.ff.00.1a.02.05.78.13.01.01.17.01.79.06.08.08.08.08.08.07.07.07.07.79.14.18.1e.00.00.0a.00.00.04.10.28.00.0a.00.00.06.00.0a.00.00.01.24.01.01.1f.01.00.42.04.0a.00.00.0a,pause)
> +reg0[15] =
> put_dhcp_opts(offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.255.0,mtu=1400,ip_forward_enable=1,default_ttl=121,dns_server={8.8.8.8,7.7.7.7},classless_static_route={
> 30.0.0.0/24,10.0.0.4,40.0.0.0/16,10.0.0.6,0.0.0.0/0,10.0.0.1
> },ethernet_encap=1,router_discovery=0,tftp_server="tftp_server_test");
> + formats as reg0[15] = put_dhcp_opts(offerip = 10.0.0.4, router =
> 10.0.0.1, netmask = 255.255.255.0, mtu = 1400, ip_forward_enable = 1,
> default_ttl = 121, dns_server = {8.8.8.8, 7.7.7.7}, classless_static_route
> = {30.0.0.0/24, 10.0.0.4, 40.0.0.0/16, 10.0.0.6, 0.0.0.0/0, 10.0.0.1},
> ethernet_encap = 1, router_discovery = 0, tftp_server = "tftp_server_test");
> + encodes as
> controller(userdata=00.00.00.02.00.00.00.00.00.01.de.10.00.00.00.6f.0a.00.00.04.03.04.0a.00.00.01.01.04.ff.ff.ff.00.1a.02.05.78.13.01.01.17.01.79.06.08.08.08.08.08.07.07.07.07.79.14.18.1e.00.00.0a.00.00.04.10.28.00.0a.00.00.06.00.0a.00.00.01.24.01.01.1f.01.00.42.10.74.66.74.70.5f.73.65.72.76.65.72.5f.74.65.73.74,pause)
> +reg2[5] =
> put_dhcp_opts(offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.254.0,mtu=1400,domain_name="
> ovn.org",wpad="https://example.org",bootfile_name=";
> https://127.0.0.1/boot.ipxe",path_prefix="/tftpboot",domain_search_list=";
> ovn.org,abc.ovn.org");
> + formats as reg2[5] = put_dhcp_opts(offerip = 10.0.0.4, router =
> 10.0.0.1, netmask = 255.255.254.0, mtu = 1400, domain_name = "ovn.org",
> wpad = "https://example.org";, bootfile_name = "https://127.0.0.1/boot.ipxe";,
> path_prefix = "/tftpboot", domain_search_list = "ovn.org,abc.ovn.org");
> + encodes as
> controller(userdata=00.00.00.02.00.00.00.00.00.01.de.10.00.00.00.25.0a.00.00.04.03.04.0a.00.00.01.01.04.ff.ff.fe.00.1a.02.05.78.0f.07.6f.76.6e.2e.6f.72.67.fc.13.68.74.74.70.73.3a.2f.2f.65.78.61.6d.70.6c.65.2e.6f.72.67.43.1b.68.74.74.70.73.3a.2f.2f.31.32.37.2e.30.2e.30.2e.31.2f.62.6f.6f.74.2e.69.70.78.65.d2.09.2f.74.66.74.70.62.6f.6f.74.77.0f.03.6f.76.6e.03.6f.72.67.00.03.61.62.63.c0.00,pause)
>
> reg1[0..1] = put_dhcp_opts(offerip = 1.2.3.4, router = 10.0.0.1);
> Cannot use 2-bit field reg1[0..1] where 1-bit field is required.
> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> index a77d2f1..b43f67f 100644
> --- a/tests/test-ovn.c
> +++ b/tests/test-ovn.c
> @@ -174,7 +174,7 @@ create_gen_opts(struct hmap *dhcp_opts, struct hmap
> *dhcpv6_opts,
> dhcp_opt_add(dhcp_opts, "nis_server", 41, "ipv4");
> dhcp_opt_add(dhcp_opts, "ntp_server", 42, "ipv4");
> dhcp_opt_add(dhcp_opts, "server_id", 54, "ipv4");
> - dhcp_opt_add(dhcp_opts, "tftp_server", 66, "ipv4");
> + dhcp_opt_add(dhcp_opts, "tftp_server", 66, "host_id");
> dhcp_opt_add(dhcp_opts, "classless_static_route", 121,
> "static_routes");
> dhcp_opt_add(dhcp_opts, "ip_forward_enable", 19, "bool");
> dhcp_opt_add(dhcp_opts, "router_discovery", 31, "bool");
> @@ -189,6 +189,7 @@ create_gen_opts(struct hmap *dhcp_opts, struct hmap
> *dhcpv6_opts,
> dhcp_opt_add(dhcp_opts, "tftp_server_address", 150, "ipv4");
> dhcp_opt_add(dhcp_opts, "arp_cache_timeout", 35, "uint32");
> dhcp_opt_add(dhcp_opts, "tcp_keepalive_interval", 38, "uint32");
> + dhcp_opt_add(dhcp_opts, "domain_search_list", 119, "domains");
>
> /* DHCPv6 options. */
> hmap_init(dhcpv6_opts);
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
