| Message ID | 1592588578-84413-1-git-send-email-svc.mail.git@nutanix.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v7,ovn] Add support for DHCP domain search option (119) | expand |
Bleep bloop. Greetings Ankur Sharma, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Ankur Sharma <ankur.sharma@nutanix.com>
ERROR: Use ovs_strlcpy() in place of strcpy()
#77 FILE: lib/actions.c:2358:
strcpy(suffix, domain);
ERROR: Use ovs_strlcpy() in place of strcpy()
#105 FILE: lib/actions.c:2386:
strcpy(suffix, domain);
Lines checked: 307, Warnings: 1, Errors: 2
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
On Fri, Jun 19, 2020 at 11:12 PM Ankur Sharma <svc.mail.git@nutanix.com> wrote: > From: Dhathri Purohith <dhathri.purohith@nutanix.com> > > Domain search list is encoded according to the specifications in RFC 1035, > section 4.1.4. > > Signed-off-by: Dhathri Purohith <dhathri.purohith@nutanix.com> > Signed-off-by: Ankur Sharma <ankur.sharma@nutanix.com> > I have asked a question in v5 of this patch about the encoding. Could you please reply to that ? Thanks Numan > --- > lib/actions.c | 90 > ++++++++++++++++++++++++++++++++++++++++++++++++++++- > lib/ovn-l7.h | 3 ++ > northd/ovn-northd.c | 1 + > ovn-nb.xml | 13 ++++++++ > ovn-sb.ovsschema | 6 ++-- > ovn-sb.xml | 11 +++++++ > tests/ovn.at | 36 +++++++++++++++++++++ > tests/test-ovn.c | 1 + > 8 files changed, 157 insertions(+), 4 deletions(-) > > diff --git a/lib/actions.c b/lib/actions.c > index 9baa90f..ad7ae78 100644 > --- a/lib/actions.c > +++ b/lib/actions.c > @@ -1982,7 +1982,8 @@ parse_gen_opt(struct action_context *ctx, struct > ovnact_gen_option *o, > return; > } > > - if (!strcmp(o->option->type, "str")) { > + 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); > @@ -2317,6 +2318,93 @@ encode_put_dhcpv4_option(const struct > ovnact_gen_option *o, > 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"). > + * > + * 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)); > + uint16_t encode_offset = 0; > + struct shash label_offset_map; > + shash_init(&label_offset_map); > + char *domain_list = xstrdup(c->string), *dom_ptr = NULL; > + char *suffix = xzalloc(strlen(domain_list)); > + 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; > + } > + strcpy(suffix, domain); > + char *label; > + for (label = strtok_r(domain, ".", &domain); > + label != NULL; > + label = strtok_r(NULL, ".", &domain)) { > + /* Check if we have already encoded this suffix. > + * If yes, fill in the reference and break. */ > + uint16_t *get_offset; > + get_offset = shash_find_data(&label_offset_map, suffix); > + if (get_offset != NULL) { > + ovs_be16 temp = htons(0xc000) | htons(*get_offset); > + memcpy(dns_encoded + encode_offset, &temp, > + sizeof(temp)); > + encode_offset += sizeof(temp); > + break; > + } else { > + /* The suffix was not encoded before, encode it now > + * and add the offset to the label_offset_map. */ > + uint16_t *set_offset = xzalloc(sizeof(uint16_t)); > + *set_offset = encode_offset; > + shash_add_once(&label_offset_map, suffix, set_offset); > + > + 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; > + } > + strcpy(suffix, domain); > + } > + /* 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: > + free(suffix); > + free(domain_list); > + free(dns_encoded); > + struct shash_node *node, *next; > + SHASH_FOR_EACH_SAFE (node, next, &label_offset_map) { > + shash_delete(&label_offset_map, node); > + } > + shash_destroy(&label_offset_map); > } > } > > diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h > index 22a2153..cea97b9 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) \ > @@ -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 6a9b097..22891c1 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -11345,6 +11345,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[] = { > diff --git a/ovn-nb.xml b/ovn-nb.xml > index 8368d51..80e843c 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -2950,6 +2950,19 @@ > </p> > </column> > </group> > + > + <group title=" DHCP Options of type domains"> > + <p> > + These options accept string value which is a comma separated > + list of domain names. The domain names are encoded based on RFC > 1035. > + </p> > + > + <column name="options" key="domain_search_list"> > + <p> > + The DHCPv4 option code for this option is 119. > + </p> > + </column> > + </group> > </group> > > <group title="DHCPv6 options"> > diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema > index 2ec729b..99c5de8 100644 > --- a/ovn-sb.ovsschema > +++ b/ovn-sb.ovsschema > @@ -1,7 +1,7 @@ > { > "name": "OVN_Southbound", > - "version": "2.8.1", > - "cksum": "236203406 21905", > + "version": "2.8.2", > + "cksum": "464326363 21916", > "tables": { > "SB_Global": { > "columns": { > @@ -218,7 +218,7 @@ > "type": "string", > "enum": ["set", ["bool", "uint8", "uint16", > "uint32", > "ipv4", "static_routes", "str", > - "host_id"]]}}}}, > + "host_id", "domains"]]}}}}, > "isRoot": true}, > "DHCPv6_Options": { > "columns": { > diff --git a/ovn-sb.xml b/ovn-sb.xml > index 208fb93..a6da80b 100644 > --- a/ovn-sb.xml > +++ b/ovn-sb.xml > @@ -3232,6 +3232,17 @@ tcp.flags = RST; > </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 7622b74..6093991 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -1218,6 +1218,9 @@ reg0[15] = > put_dhcp_opts(offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.255.0, > 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. > @@ -1235,6 +1238,8 @@ reg1[0] = put_dhcp_opts(offerip="xyzzy"); > DHCPv4 option offerip requires numeric value. > reg1[0] = put_dhcp_opts(offerip=1.2.3.4, domain_name=1.2.3.4); > DHCPv4 option domain_name requires string value. > +reg1[0] = put_dhcp_opts(offerip=1.2.3.4, domain_search_list=1.2.3.4); > + DHCPv4 option domain_search_list requires string value. > > # nd_ns > nd_ns { nd.target = xxreg0; output; }; > @@ -5614,6 +5619,37 @@ AT_CHECK([cat 2.packets | cut -c -48], [0], > [expout]) > cat 2.expected | cut -c 53- > expout > AT_CHECK([cat 2.packets | cut -c 53-], [0], [expout]) > > +reset_pcap_file hv1-vif1 hv1/vif1 > +reset_pcap_file hv1-vif2 hv1/vif2 > +rm -f 1.expected > +rm -f 2.expected > + > +# Set domain search list option for ls1 > +echo "------ Set domain search list --------" > +ovn-nbctl dhcp-options-set-options $d1 server_id=10.0.0.1 \ > +server_mac=ff:10:00:00:00:01 lease_time=3600 router=10.0.0.1 \ > +domain_search_list=\"test1.com,test2.com\" > +echo "------------------------------------------" > + > +# Send DHCPREQUEST in the SELECTING/INIT-REBOOT state with the offered IP > +# address in the Requested IP Address option. > +offer_ip=`ip_to_hex 10 0 0 6` > +server_ip=`ip_to_hex 10 0 0 1` > +ciaddr=`ip_to_hex 0 0 0 0` > +request_ip=$offer_ip > > +expected_dhcp_opts=771305746573743103636f6d00057465737432c006330400000e100104ffffff0003040a00000136040a000001 > +test_dhcp 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 > ff1000000001 $server_ip 05 $expected_dhcp_opts > + > +# NXT_RESUMEs should be 12. > +OVS_WAIT_UNTIL([test 12 = `cat ofctl_monitor*.log | grep -c NXT_RESUME`]) > + > +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets > +cat 2.expected | cut -c -48 > expout > +AT_CHECK([cat 2.packets | cut -c -48], [0], [expout]) > +# Skipping the IPv4 checksum. > +cat 2.expected | cut -c 53- > expout > +AT_CHECK([cat 2.packets | cut -c 53-], [0], [expout]) > + > OVN_CLEANUP([hv1]) > > AT_CLEANUP > diff --git a/tests/test-ovn.c b/tests/test-ovn.c > index 4391694..b43f67f 100644 > --- a/tests/test-ovn.c > +++ b/tests/test-ovn.c > @@ -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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Mon, Jun 22, 2020 at 12:09 PM Numan Siddique <numans@ovn.org> wrote: > > > On Fri, Jun 19, 2020 at 11:12 PM Ankur Sharma <svc.mail.git@nutanix.com> > wrote: > >> From: Dhathri Purohith <dhathri.purohith@nutanix.com> >> >> Domain search list is encoded according to the specifications in RFC 1035, >> section 4.1.4. >> >> Signed-off-by: Dhathri Purohith <dhathri.purohith@nutanix.com> >> Signed-off-by: Ankur Sharma <ankur.sharma@nutanix.com> >> > > I have asked a question in v5 of this patch about the encoding. > Could you please reply to that ? > > Thanks > Numan > > Hi Dhatri, I've few minor comments. Otherwise the patch LGTM. I tested locally and it seems to work fine. I compared the encoding between what this patch is doing and dnsmasq. I found a little difference. For example: for the domain_search_list - test.org,ovn.org,abc.ovn.org, def.ovn.org,ghi.ovn.org Your patch encodes it as : 77 22 04 74 65 73 74 03 6f 72 67 00 03 6f 76 6e c0 05 03 61 62 63 c0 0a 03 64 65 66 c0 0a 03 67 68 69 c0 0a But dnsmasq does encoding a little differently. 77 2e 04 74 65 73 74 03 6f 72 67 00 03 6f 76 6e c0 05 03 61 62 63 03 6f 76 6e c0 05 03 64 65 66 03 6f 76 6e c0 05 03 67 68 69 03 6f 76 6e c0 05 Can you please check and verify once ? I'm fine with your patch. But I just want to be sure. --- >> lib/actions.c | 90 >> ++++++++++++++++++++++++++++++++++++++++++++++++++++- >> lib/ovn-l7.h | 3 ++ >> northd/ovn-northd.c | 1 + >> ovn-nb.xml | 13 ++++++++ >> ovn-sb.ovsschema | 6 ++-- >> ovn-sb.xml | 11 +++++++ >> tests/ovn.at | 36 +++++++++++++++++++++ >> tests/test-ovn.c | 1 + >> 8 files changed, 157 insertions(+), 4 deletions(-) >> >> diff --git a/lib/actions.c b/lib/actions.c >> index 9baa90f..ad7ae78 100644 >> --- a/lib/actions.c >> +++ b/lib/actions.c >> @@ -1982,7 +1982,8 @@ parse_gen_opt(struct action_context *ctx, struct >> ovnact_gen_option *o, >> return; >> } >> >> - if (!strcmp(o->option->type, "str")) { >> + 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); >> @@ -2317,6 +2318,93 @@ encode_put_dhcpv4_option(const struct >> ovnact_gen_option *o, >> 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"). >> + * >> + * 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)); >> + uint16_t encode_offset = 0; >> + struct shash label_offset_map; >> + shash_init(&label_offset_map); >> > You can init shash during declaration as: struct shash label_offset_map = SHASH_INITIALIZER(&label_offset_map); > + char *domain_list = xstrdup(c->string), *dom_ptr = NULL; >> + char *suffix = xzalloc(strlen(domain_list)); >> + 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; >> + } >> + strcpy(suffix, domain); >> > checkpatch complains to use ovs_strlcpy. Please use that instead of strcpy. > + char *label; >> + for (label = strtok_r(domain, ".", &domain); >> + label != NULL; >> + label = strtok_r(NULL, ".", &domain)) { >> + /* Check if we have already encoded this suffix. >> + * If yes, fill in the reference and break. */ >> + uint16_t *get_offset; >> + get_offset = shash_find_data(&label_offset_map, suffix); >> + if (get_offset != NULL) { >> + ovs_be16 temp = htons(0xc000) | htons(*get_offset); >> + memcpy(dns_encoded + encode_offset, &temp, >> + sizeof(temp)); >> + encode_offset += sizeof(temp); >> + break; >> + } else { >> + /* The suffix was not encoded before, encode it now >> + * and add the offset to the label_offset_map. */ >> + uint16_t *set_offset = xzalloc(sizeof(uint16_t)); >> + *set_offset = encode_offset; >> + shash_add_once(&label_offset_map, suffix, >> set_offset); >> + >> + 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; >> + } >> + strcpy(suffix, domain); >> > Same here. > + } >> + /* 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: >> + free(suffix); >> + free(domain_list); >> + free(dns_encoded); >> + struct shash_node *node, *next; >> + SHASH_FOR_EACH_SAFE (node, next, &label_offset_map) { >> + shash_delete(&label_offset_map, node); >> + } >> + shash_destroy(&label_offset_map); >> > I think there is a memory leak here since shash_delete doesn't free the shash data which you allocated for 'set_offset'. I think instead of using SHASH_FOR_EACH_SAFE and then calling shash_destroy(), you can just do shash_destroy_free_data(). } >> } >> >> diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h >> index 22a2153..cea97b9 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) \ >> @@ -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 6a9b097..22891c1 100644 >> --- a/northd/ovn-northd.c >> +++ b/northd/ovn-northd.c >> @@ -11345,6 +11345,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[] = { >> diff --git a/ovn-nb.xml b/ovn-nb.xml >> index 8368d51..80e843c 100644 >> --- a/ovn-nb.xml >> +++ b/ovn-nb.xml >> @@ -2950,6 +2950,19 @@ >> </p> >> </column> >> </group> >> + >> + <group title=" DHCP Options of type domains"> >> + <p> >> + These options accept string value which is a comma separated >> + list of domain names. The domain names are encoded based on >> RFC 1035. >> + </p> >> + >> + <column name="options" key="domain_search_list"> >> + <p> >> + The DHCPv4 option code for this option is 119. >> + </p> >> + </column> >> + </group> >> </group> >> >> <group title="DHCPv6 options"> >> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema >> index 2ec729b..99c5de8 100644 >> --- a/ovn-sb.ovsschema >> +++ b/ovn-sb.ovsschema >> @@ -1,7 +1,7 @@ >> { >> "name": "OVN_Southbound", >> - "version": "2.8.1", >> - "cksum": "236203406 21905", >> + "version": "2.8.2", >> + "cksum": "464326363 21916", >> "tables": { >> "SB_Global": { >> "columns": { >> @@ -218,7 +218,7 @@ >> "type": "string", >> "enum": ["set", ["bool", "uint8", "uint16", >> "uint32", >> "ipv4", "static_routes", "str", >> - "host_id"]]}}}}, >> + "host_id", "domains"]]}}}}, >> "isRoot": true}, >> "DHCPv6_Options": { >> "columns": { >> diff --git a/ovn-sb.xml b/ovn-sb.xml >> index 208fb93..a6da80b 100644 >> --- a/ovn-sb.xml >> +++ b/ovn-sb.xml >> @@ -3232,6 +3232,17 @@ tcp.flags = RST; >> </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 7622b74..6093991 100644 >> --- a/tests/ovn.at >> +++ b/tests/ovn.at >> @@ -1218,6 +1218,9 @@ reg0[15] = >> put_dhcp_opts(offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.255.0, >> 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) >> > Can you add a few more cases here ? Like the above example I gave ? Thanks Numan > >> 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. >> @@ -1235,6 +1238,8 @@ reg1[0] = put_dhcp_opts(offerip="xyzzy"); >> DHCPv4 option offerip requires numeric value. >> reg1[0] = put_dhcp_opts(offerip=1.2.3.4, domain_name=1.2.3.4); >> DHCPv4 option domain_name requires string value. >> +reg1[0] = put_dhcp_opts(offerip=1.2.3.4, domain_search_list=1.2.3.4); >> + DHCPv4 option domain_search_list requires string value. >> >> # nd_ns >> nd_ns { nd.target = xxreg0; output; }; >> @@ -5614,6 +5619,37 @@ AT_CHECK([cat 2.packets | cut -c -48], [0], >> [expout]) >> cat 2.expected | cut -c 53- > expout >> AT_CHECK([cat 2.packets | cut -c 53-], [0], [expout]) >> >> +reset_pcap_file hv1-vif1 hv1/vif1 >> +reset_pcap_file hv1-vif2 hv1/vif2 >> +rm -f 1.expected >> +rm -f 2.expected >> + >> +# Set domain search list option for ls1 >> +echo "------ Set domain search list --------" >> +ovn-nbctl dhcp-options-set-options $d1 server_id=10.0.0.1 \ >> +server_mac=ff:10:00:00:00:01 lease_time=3600 router=10.0.0.1 \ >> +domain_search_list=\"test1.com,test2.com\" >> +echo "------------------------------------------" >> + >> +# Send DHCPREQUEST in the SELECTING/INIT-REBOOT state with the offered IP >> +# address in the Requested IP Address option. >> +offer_ip=`ip_to_hex 10 0 0 6` >> +server_ip=`ip_to_hex 10 0 0 1` >> +ciaddr=`ip_to_hex 0 0 0 0` >> +request_ip=$offer_ip >> >> +expected_dhcp_opts=771305746573743103636f6d00057465737432c006330400000e100104ffffff0003040a00000136040a000001 >> +test_dhcp 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 >> ff1000000001 $server_ip 05 $expected_dhcp_opts >> + >> +# NXT_RESUMEs should be 12. >> +OVS_WAIT_UNTIL([test 12 = `cat ofctl_monitor*.log | grep -c NXT_RESUME`]) >> + >> +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets >> +cat 2.expected | cut -c -48 > expout >> +AT_CHECK([cat 2.packets | cut -c -48], [0], [expout]) >> +# Skipping the IPv4 checksum. >> +cat 2.expected | cut -c 53- > expout >> +AT_CHECK([cat 2.packets | cut -c 53-], [0], [expout]) >> + >> OVN_CLEANUP([hv1]) >> >> AT_CLEANUP >> diff --git a/tests/test-ovn.c b/tests/test-ovn.c >> index 4391694..b43f67f 100644 >> --- a/tests/test-ovn.c >> +++ b/tests/test-ovn.c >> @@ -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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >>
On 6/23/20 5:13 AM, Numan Siddique wrote: > On Mon, Jun 22, 2020 at 12:09 PM Numan Siddique <numans@ovn.org> wrote: > >> >> >> On Fri, Jun 19, 2020 at 11:12 PM Ankur Sharma <svc.mail.git@nutanix.com> >> wrote: >> >>> From: Dhathri Purohith <dhathri.purohith@nutanix.com> >>> >>> Domain search list is encoded according to the specifications in RFC 1035, >>> section 4.1.4. >>> >>> Signed-off-by: Dhathri Purohith <dhathri.purohith@nutanix.com> >>> Signed-off-by: Ankur Sharma <ankur.sharma@nutanix.com> >>> >> >> I have asked a question in v5 of this patch about the encoding. >> Could you please reply to that ? >> >> Thanks >> Numan >> >> > > Hi Dhatri, > > I've few minor comments. Otherwise the patch LGTM. > > I tested locally and it seems to work fine. I compared the encoding between > what this patch is doing > and dnsmasq. I found a little difference. > > For example: for the domain_search_list - test.org,ovn.org,abc.ovn.org, > def.ovn.org,ghi.ovn.org > > Your patch encodes it as : > 77 22 04 74 65 73 74 03 6f 72 67 00 03 6f 76 6e c0 05 03 61 62 63 c0 0a 03 > 64 65 66 c0 0a 03 67 68 69 c0 0a > > But dnsmasq does encoding a little differently. > 77 2e 04 74 65 73 74 03 6f 72 67 00 03 6f 76 6e c0 05 03 61 62 63 03 6f 76 > 6e c0 05 03 64 65 66 03 6f 76 6e c0 05 03 67 68 69 03 6f 76 6e c0 05 > > Can you please check and verify once ? > Hi Numan, they both appear to be correct to me. dnsmasq uses fewer domain compression pointers than this patch does. dnsmasq compresses every occurrence of ".org" after the first into "c0 05". However, it repeats the explicit use of "ovn" throughout. This patch takes it a step further by encoding ".org" into "c0 05" but also encoding "ovn.org" into "c0 0a" (which itself compresses the ".org" into "c0 05"). This results in a smaller message that still decodes to the same value. > I'm fine with your patch. But I just want to be sure. > > > --- >>> lib/actions.c | 90 >>> ++++++++++++++++++++++++++++++++++++++++++++++++++++- >>> lib/ovn-l7.h | 3 ++ >>> northd/ovn-northd.c | 1 + >>> ovn-nb.xml | 13 ++++++++ >>> ovn-sb.ovsschema | 6 ++-- >>> ovn-sb.xml | 11 +++++++ >>> tests/ovn.at | 36 +++++++++++++++++++++ >>> tests/test-ovn.c | 1 + >>> 8 files changed, 157 insertions(+), 4 deletions(-) >>> >>> diff --git a/lib/actions.c b/lib/actions.c >>> index 9baa90f..ad7ae78 100644 >>> --- a/lib/actions.c >>> +++ b/lib/actions.c >>> @@ -1982,7 +1982,8 @@ parse_gen_opt(struct action_context *ctx, struct >>> ovnact_gen_option *o, >>> return; >>> } >>> >>> - if (!strcmp(o->option->type, "str")) { >>> + 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); >>> @@ -2317,6 +2318,93 @@ encode_put_dhcpv4_option(const struct >>> ovnact_gen_option *o, >>> 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"). >>> + * >>> + * 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)); >>> + uint16_t encode_offset = 0; >>> + struct shash label_offset_map; >>> + shash_init(&label_offset_map); >>> >> > You can init shash during declaration as: > struct shash label_offset_map = SHASH_INITIALIZER(&label_offset_map); > > > >> + char *domain_list = xstrdup(c->string), *dom_ptr = NULL; >>> + char *suffix = xzalloc(strlen(domain_list)); >>> + 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; >>> + } >>> + strcpy(suffix, domain); >>> >> > checkpatch complains to use ovs_strlcpy. Please use that instead of strcpy. > > >> + char *label; >>> + for (label = strtok_r(domain, ".", &domain); >>> + label != NULL; >>> + label = strtok_r(NULL, ".", &domain)) { >>> + /* Check if we have already encoded this suffix. >>> + * If yes, fill in the reference and break. */ >>> + uint16_t *get_offset; >>> + get_offset = shash_find_data(&label_offset_map, suffix); >>> + if (get_offset != NULL) { >>> + ovs_be16 temp = htons(0xc000) | htons(*get_offset); >>> + memcpy(dns_encoded + encode_offset, &temp, >>> + sizeof(temp)); >>> + encode_offset += sizeof(temp); >>> + break; >>> + } else { >>> + /* The suffix was not encoded before, encode it now >>> + * and add the offset to the label_offset_map. */ >>> + uint16_t *set_offset = xzalloc(sizeof(uint16_t)); >>> + *set_offset = encode_offset; >>> + shash_add_once(&label_offset_map, suffix, >>> set_offset); >>> + >>> + 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; >>> + } >>> + strcpy(suffix, domain); >>> >> > Same here. > > > >> + } >>> + /* 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: >>> + free(suffix); >>> + free(domain_list); >>> + free(dns_encoded); >>> + struct shash_node *node, *next; >>> + SHASH_FOR_EACH_SAFE (node, next, &label_offset_map) { >>> + shash_delete(&label_offset_map, node); >>> + } >>> + shash_destroy(&label_offset_map); >>> >> > I think there is a memory leak here since shash_delete doesn't free the > shash data which you allocated for 'set_offset'. > > I think instead of using SHASH_FOR_EACH_SAFE and then > calling shash_destroy(), > you can just do shash_destroy_free_data(). > > > } >>> } >>> >>> diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h >>> index 22a2153..cea97b9 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) \ >>> @@ -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 6a9b097..22891c1 100644 >>> --- a/northd/ovn-northd.c >>> +++ b/northd/ovn-northd.c >>> @@ -11345,6 +11345,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[] = { >>> diff --git a/ovn-nb.xml b/ovn-nb.xml >>> index 8368d51..80e843c 100644 >>> --- a/ovn-nb.xml >>> +++ b/ovn-nb.xml >>> @@ -2950,6 +2950,19 @@ >>> </p> >>> </column> >>> </group> >>> + >>> + <group title=" DHCP Options of type domains"> >>> + <p> >>> + These options accept string value which is a comma separated >>> + list of domain names. The domain names are encoded based on >>> RFC 1035. >>> + </p> >>> + >>> + <column name="options" key="domain_search_list"> >>> + <p> >>> + The DHCPv4 option code for this option is 119. >>> + </p> >>> + </column> >>> + </group> >>> </group> >>> >>> <group title="DHCPv6 options"> >>> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema >>> index 2ec729b..99c5de8 100644 >>> --- a/ovn-sb.ovsschema >>> +++ b/ovn-sb.ovsschema >>> @@ -1,7 +1,7 @@ >>> { >>> "name": "OVN_Southbound", >>> - "version": "2.8.1", >>> - "cksum": "236203406 21905", >>> + "version": "2.8.2", >>> + "cksum": "464326363 21916", >>> "tables": { >>> "SB_Global": { >>> "columns": { >>> @@ -218,7 +218,7 @@ >>> "type": "string", >>> "enum": ["set", ["bool", "uint8", "uint16", >>> "uint32", >>> "ipv4", "static_routes", "str", >>> - "host_id"]]}}}}, >>> + "host_id", "domains"]]}}}}, >>> "isRoot": true}, >>> "DHCPv6_Options": { >>> "columns": { >>> diff --git a/ovn-sb.xml b/ovn-sb.xml >>> index 208fb93..a6da80b 100644 >>> --- a/ovn-sb.xml >>> +++ b/ovn-sb.xml >>> @@ -3232,6 +3232,17 @@ tcp.flags = RST; >>> </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 7622b74..6093991 100644 >>> --- a/tests/ovn.at >>> +++ b/tests/ovn.at >>> @@ -1218,6 +1218,9 @@ reg0[15] = >>> put_dhcp_opts(offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.255.0, >>> 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) >>> >> > Can you add a few more cases here ? Like the above example I gave ? > > Thanks > Numan > > >> >>> 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. >>> @@ -1235,6 +1238,8 @@ reg1[0] = put_dhcp_opts(offerip="xyzzy"); >>> DHCPv4 option offerip requires numeric value. >>> reg1[0] = put_dhcp_opts(offerip=1.2.3.4, domain_name=1.2.3.4); >>> DHCPv4 option domain_name requires string value. >>> +reg1[0] = put_dhcp_opts(offerip=1.2.3.4, domain_search_list=1.2.3.4); >>> + DHCPv4 option domain_search_list requires string value. >>> >>> # nd_ns >>> nd_ns { nd.target = xxreg0; output; }; >>> @@ -5614,6 +5619,37 @@ AT_CHECK([cat 2.packets | cut -c -48], [0], >>> [expout]) >>> cat 2.expected | cut -c 53- > expout >>> AT_CHECK([cat 2.packets | cut -c 53-], [0], [expout]) >>> >>> +reset_pcap_file hv1-vif1 hv1/vif1 >>> +reset_pcap_file hv1-vif2 hv1/vif2 >>> +rm -f 1.expected >>> +rm -f 2.expected >>> + >>> +# Set domain search list option for ls1 >>> +echo "------ Set domain search list --------" >>> +ovn-nbctl dhcp-options-set-options $d1 server_id=10.0.0.1 \ >>> +server_mac=ff:10:00:00:00:01 lease_time=3600 router=10.0.0.1 \ >>> +domain_search_list=\"test1.com,test2.com\" >>> +echo "------------------------------------------" >>> + >>> +# Send DHCPREQUEST in the SELECTING/INIT-REBOOT state with the offered IP >>> +# address in the Requested IP Address option. >>> +offer_ip=`ip_to_hex 10 0 0 6` >>> +server_ip=`ip_to_hex 10 0 0 1` >>> +ciaddr=`ip_to_hex 0 0 0 0` >>> +request_ip=$offer_ip >>> >>> +expected_dhcp_opts=771305746573743103636f6d00057465737432c006330400000e100104ffffff0003040a00000136040a000001 >>> +test_dhcp 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 >>> ff1000000001 $server_ip 05 $expected_dhcp_opts >>> + >>> +# NXT_RESUMEs should be 12. >>> +OVS_WAIT_UNTIL([test 12 = `cat ofctl_monitor*.log | grep -c NXT_RESUME`]) >>> + >>> +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets >>> +cat 2.expected | cut -c -48 > expout >>> +AT_CHECK([cat 2.packets | cut -c -48], [0], [expout]) >>> +# Skipping the IPv4 checksum. >>> +cat 2.expected | cut -c 53- > expout >>> +AT_CHECK([cat 2.packets | cut -c 53-], [0], [expout]) >>> + >>> OVN_CLEANUP([hv1]) >>> >>> AT_CLEANUP >>> diff --git a/tests/test-ovn.c b/tests/test-ovn.c >>> index 4391694..b43f67f 100644 >>> --- a/tests/test-ovn.c >>> +++ b/tests/test-ovn.c >>> @@ -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 >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >>> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Tue, Jun 23, 2020 at 7:36 PM Mark Michelson <mmichels@redhat.com> wrote: > On 6/23/20 5:13 AM, Numan Siddique wrote: > > On Mon, Jun 22, 2020 at 12:09 PM Numan Siddique <numans@ovn.org> wrote: > > > >> > >> > >> On Fri, Jun 19, 2020 at 11:12 PM Ankur Sharma <svc.mail.git@nutanix.com > > > >> wrote: > >> > >>> From: Dhathri Purohith <dhathri.purohith@nutanix.com> > >>> > >>> Domain search list is encoded according to the specifications in RFC > 1035, > >>> section 4.1.4. > >>> > >>> Signed-off-by: Dhathri Purohith <dhathri.purohith@nutanix.com> > >>> Signed-off-by: Ankur Sharma <ankur.sharma@nutanix.com> > >>> > >> > >> I have asked a question in v5 of this patch about the encoding. > >> Could you please reply to that ? > >> > >> Thanks > >> Numan > >> > >> > > > > Hi Dhatri, > > > > I've few minor comments. Otherwise the patch LGTM. > > > > I tested locally and it seems to work fine. I compared the encoding > between > > what this patch is doing > > and dnsmasq. I found a little difference. > > > > For example: for the domain_search_list - test.org,ovn.org,abc.ovn.org, > > def.ovn.org,ghi.ovn.org > > > > Your patch encodes it as : > > 77 22 04 74 65 73 74 03 6f 72 67 00 03 6f 76 6e c0 05 03 61 62 63 c0 0a > 03 > > 64 65 66 c0 0a 03 67 68 69 c0 0a > > > But dnsmasq does encoding a little differently. > > 77 2e 04 74 65 73 74 03 6f 72 67 00 03 6f 76 6e c0 05 03 61 62 63 03 6f > 76 > > 6e c0 05 03 64 65 66 03 6f 76 6e c0 05 03 67 68 69 03 6f 76 6e c0 05 > > > > Can you please check and verify once ? > > > Hi Numan, they both appear to be correct to me. dnsmasq uses fewer > domain compression pointers than this patch does. dnsmasq compresses > every occurrence of ".org" after the first into "c0 05". However, it > repeats the explicit use of "ovn" throughout. > > This patch takes it a step further by encoding ".org" into "c0 05" but > also encoding "ovn.org" into "c0 0a" (which itself compresses the ".org" > into "c0 05"). This results in a smaller message that still decodes to > the same value. > > Thanks Mark for verifying it further. I was just making sure that the present patch is correct taking dnsmasq as reference. Thanks Numan > > I'm fine with your patch. But I just want to be sure. > > > > > > --- > >>> lib/actions.c | 90 > >>> ++++++++++++++++++++++++++++++++++++++++++++++++++++- > >>> lib/ovn-l7.h | 3 ++ > >>> northd/ovn-northd.c | 1 + > >>> ovn-nb.xml | 13 ++++++++ > >>> ovn-sb.ovsschema | 6 ++-- > >>> ovn-sb.xml | 11 +++++++ > >>> tests/ovn.at | 36 +++++++++++++++++++++ > >>> tests/test-ovn.c | 1 + > >>> 8 files changed, 157 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/lib/actions.c b/lib/actions.c > >>> index 9baa90f..ad7ae78 100644 > >>> --- a/lib/actions.c > >>> +++ b/lib/actions.c > >>> @@ -1982,7 +1982,8 @@ parse_gen_opt(struct action_context *ctx, struct > >>> ovnact_gen_option *o, > >>> return; > >>> } > >>> > >>> - if (!strcmp(o->option->type, "str")) { > >>> + 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); > >>> @@ -2317,6 +2318,93 @@ encode_put_dhcpv4_option(const struct > >>> ovnact_gen_option *o, > >>> 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"). > >>> + * > >>> + * 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)); > >>> + uint16_t encode_offset = 0; > >>> + struct shash label_offset_map; > >>> + shash_init(&label_offset_map); > >>> > >> > > You can init shash during declaration as: > > struct shash label_offset_map = SHASH_INITIALIZER(&label_offset_map); > > > > > > > >> + char *domain_list = xstrdup(c->string), *dom_ptr = NULL; > >>> + char *suffix = xzalloc(strlen(domain_list)); > >>> + 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; > >>> + } > >>> + strcpy(suffix, domain); > >>> > >> > > checkpatch complains to use ovs_strlcpy. Please use that instead of > strcpy. > > > > > >> + char *label; > >>> + for (label = strtok_r(domain, ".", &domain); > >>> + label != NULL; > >>> + label = strtok_r(NULL, ".", &domain)) { > >>> + /* Check if we have already encoded this suffix. > >>> + * If yes, fill in the reference and break. */ > >>> + uint16_t *get_offset; > >>> + get_offset = shash_find_data(&label_offset_map, > suffix); > >>> + if (get_offset != NULL) { > >>> + ovs_be16 temp = htons(0xc000) | > htons(*get_offset); > >>> + memcpy(dns_encoded + encode_offset, &temp, > >>> + sizeof(temp)); > >>> + encode_offset += sizeof(temp); > >>> + break; > >>> + } else { > >>> + /* The suffix was not encoded before, encode it > now > >>> + * and add the offset to the label_offset_map. */ > >>> + uint16_t *set_offset = xzalloc(sizeof(uint16_t)); > >>> + *set_offset = encode_offset; > >>> + shash_add_once(&label_offset_map, suffix, > >>> set_offset); > >>> + > >>> + 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; > >>> + } > >>> + strcpy(suffix, domain); > >>> > >> > > Same here. > > > > > > > >> + } > >>> + /* 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: > >>> + free(suffix); > >>> + free(domain_list); > >>> + free(dns_encoded); > >>> + struct shash_node *node, *next; > >>> + SHASH_FOR_EACH_SAFE (node, next, &label_offset_map) { > >>> + shash_delete(&label_offset_map, node); > >>> + } > >>> + shash_destroy(&label_offset_map); > >>> > >> > > I think there is a memory leak here since shash_delete doesn't free the > > shash data which you allocated for 'set_offset'. > > > > I think instead of using SHASH_FOR_EACH_SAFE and then > > calling shash_destroy(), > > you can just do shash_destroy_free_data(). > > > > > > } > >>> } > >>> > >>> diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h > >>> index 22a2153..cea97b9 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) \ > >>> @@ -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 6a9b097..22891c1 100644 > >>> --- a/northd/ovn-northd.c > >>> +++ b/northd/ovn-northd.c > >>> @@ -11345,6 +11345,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[] = { > >>> diff --git a/ovn-nb.xml b/ovn-nb.xml > >>> index 8368d51..80e843c 100644 > >>> --- a/ovn-nb.xml > >>> +++ b/ovn-nb.xml > >>> @@ -2950,6 +2950,19 @@ > >>> </p> > >>> </column> > >>> </group> > >>> + > >>> + <group title=" DHCP Options of type domains"> > >>> + <p> > >>> + These options accept string value which is a comma separated > >>> + list of domain names. The domain names are encoded based on > >>> RFC 1035. > >>> + </p> > >>> + > >>> + <column name="options" key="domain_search_list"> > >>> + <p> > >>> + The DHCPv4 option code for this option is 119. > >>> + </p> > >>> + </column> > >>> + </group> > >>> </group> > >>> > >>> <group title="DHCPv6 options"> > >>> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema > >>> index 2ec729b..99c5de8 100644 > >>> --- a/ovn-sb.ovsschema > >>> +++ b/ovn-sb.ovsschema > >>> @@ -1,7 +1,7 @@ > >>> { > >>> "name": "OVN_Southbound", > >>> - "version": "2.8.1", > >>> - "cksum": "236203406 21905", > >>> + "version": "2.8.2", > >>> + "cksum": "464326363 21916", > >>> "tables": { > >>> "SB_Global": { > >>> "columns": { > >>> @@ -218,7 +218,7 @@ > >>> "type": "string", > >>> "enum": ["set", ["bool", "uint8", "uint16", > >>> "uint32", > >>> "ipv4", "static_routes", > "str", > >>> - "host_id"]]}}}}, > >>> + "host_id", "domains"]]}}}}, > >>> "isRoot": true}, > >>> "DHCPv6_Options": { > >>> "columns": { > >>> diff --git a/ovn-sb.xml b/ovn-sb.xml > >>> index 208fb93..a6da80b 100644 > >>> --- a/ovn-sb.xml > >>> +++ b/ovn-sb.xml > >>> @@ -3232,6 +3232,17 @@ tcp.flags = RST; > >>> </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 7622b74..6093991 100644 > >>> --- a/tests/ovn.at > >>> +++ b/tests/ovn.at > >>> @@ -1218,6 +1218,9 @@ reg0[15] = > >>> put_dhcp_opts(offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.255.0, > >>> 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) > >>> > >> > > Can you add a few more cases here ? Like the above example I gave ? > > > > Thanks > > Numan > > > > > >> > >>> 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. > >>> @@ -1235,6 +1238,8 @@ reg1[0] = put_dhcp_opts(offerip="xyzzy"); > >>> DHCPv4 option offerip requires numeric value. > >>> reg1[0] = put_dhcp_opts(offerip=1.2.3.4, domain_name=1.2.3.4); > >>> DHCPv4 option domain_name requires string value. > >>> +reg1[0] = put_dhcp_opts(offerip=1.2.3.4, domain_search_list=1.2.3.4); > >>> + DHCPv4 option domain_search_list requires string value. > >>> > >>> # nd_ns > >>> nd_ns { nd.target = xxreg0; output; }; > >>> @@ -5614,6 +5619,37 @@ AT_CHECK([cat 2.packets | cut -c -48], [0], > >>> [expout]) > >>> cat 2.expected | cut -c 53- > expout > >>> AT_CHECK([cat 2.packets | cut -c 53-], [0], [expout]) > >>> > >>> +reset_pcap_file hv1-vif1 hv1/vif1 > >>> +reset_pcap_file hv1-vif2 hv1/vif2 > >>> +rm -f 1.expected > >>> +rm -f 2.expected > >>> + > >>> +# Set domain search list option for ls1 > >>> +echo "------ Set domain search list --------" > >>> +ovn-nbctl dhcp-options-set-options $d1 server_id=10.0.0.1 \ > >>> +server_mac=ff:10:00:00:00:01 lease_time=3600 router=10.0.0.1 \ > >>> +domain_search_list=\"test1.com,test2.com\" > >>> +echo "------------------------------------------" > >>> + > >>> +# Send DHCPREQUEST in the SELECTING/INIT-REBOOT state with the > offered IP > >>> +# address in the Requested IP Address option. > >>> +offer_ip=`ip_to_hex 10 0 0 6` > >>> +server_ip=`ip_to_hex 10 0 0 1` > >>> +ciaddr=`ip_to_hex 0 0 0 0` > >>> +request_ip=$offer_ip > >>> > >>> > +expected_dhcp_opts=771305746573743103636f6d00057465737432c006330400000e100104ffffff0003040a00000136040a000001 > >>> +test_dhcp 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 > >>> ff1000000001 $server_ip 05 $expected_dhcp_opts > >>> + > >>> +# NXT_RESUMEs should be 12. > >>> +OVS_WAIT_UNTIL([test 12 = `cat ofctl_monitor*.log | grep -c > NXT_RESUME`]) > >>> + > >>> +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > > 2.packets > >>> +cat 2.expected | cut -c -48 > expout > >>> +AT_CHECK([cat 2.packets | cut -c -48], [0], [expout]) > >>> +# Skipping the IPv4 checksum. > >>> +cat 2.expected | cut -c 53- > expout > >>> +AT_CHECK([cat 2.packets | cut -c 53-], [0], [expout]) > >>> + > >>> OVN_CLEANUP([hv1]) > >>> > >>> AT_CLEANUP > >>> diff --git a/tests/test-ovn.c b/tests/test-ovn.c > >>> index 4391694..b43f67f 100644 > >>> --- a/tests/test-ovn.c > >>> +++ b/tests/test-ovn.c > >>> @@ -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 > >>> dev@openvswitch.org > >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >>> > >>> > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
Thanks for confirming the correctness of the encoding Mark. Will address the rest of the review comments and update the patch. Thanks, Dhathri From: Numan Siddique <numans@ovn.org> Date: Tuesday, June 23, 2020 at 7:33 AM To: Mark Michelson <mmichels@redhat.com> Cc: "svc.mail.git" <svc.mail.git@nutanix.com>, ovs-dev <ovs-dev@openvswitch.org>, Dhathri Purohith <dhathri.purohith@nutanix.com> Subject: Re: [ovs-dev] [PATCH v7 ovn] Add support for DHCP domain search option (119) On Tue, Jun 23, 2020 at 7:36 PM Mark Michelson <mmichels@redhat.com<mailto:mmichels@redhat.com>> wrote: On 6/23/20 5:13 AM, Numan Siddique wrote: > On Mon, Jun 22, 2020 at 12:09 PM Numan Siddique <numans@ovn.org<mailto:numans@ovn.org>> wrote: > >> >> >> On Fri, Jun 19, 2020 at 11:12 PM Ankur Sharma <svc.mail.git@nutanix.com<mailto:svc.mail.git@nutanix.com>> >> wrote: >> >>> From: Dhathri Purohith <dhathri.purohith@nutanix.com<mailto:dhathri.purohith@nutanix.com>> >>> >>> Domain search list is encoded according to the specifications in RFC 1035, >>> section 4.1.4. >>> >>> Signed-off-by: Dhathri Purohith <dhathri.purohith@nutanix.com<mailto:dhathri.purohith@nutanix.com>> >>> Signed-off-by: Ankur Sharma <ankur.sharma@nutanix.com<mailto:ankur.sharma@nutanix.com>> >>> >> >> I have asked a question in v5 of this patch about the encoding. >> Could you please reply to that ? >> >> Thanks >> Numan >> >> > > Hi Dhatri, > > I've few minor comments. Otherwise the patch LGTM. > > I tested locally and it seems to work fine. I compared the encoding between > what this patch is doing > and dnsmasq. I found a little difference. > > For example: for the domain_search_list - test.org [test.org]<https://urldefense.proofpoint.com/v2/url?u=http-3A__test.org&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=E6H3a-1CsxSndIs34d6OgjJd9wazm9lKsk974vFul3E&e=>,ovn.org [ovn.org]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.org&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=4F-Kh0fA52dZU35XPtUyphwquItwevusYr5K--o-DxQ&e=>,abc.ovn.org [abc.ovn.org]<https://urldefense.proofpoint.com/v2/url?u=http-3A__abc.ovn.org&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=7q3hlfdvr-hOcN9s1CWD3id1kwPf3ytmQ_mlK1qLtOE&e=>, > def.ovn.org [def.ovn.org]<https://urldefense.proofpoint.com/v2/url?u=http-3A__def.ovn.org&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=vx5nw7FYwVcEbuSUKiy32nKMgXU0HUoaKo-Ten58Yhk&e=>,ghi.ovn.org [ghi.ovn.org]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ghi.ovn.org&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=pGvXMxho_eWU6Z6BYVKwUCHhVEPwwkFfkkOYQuaS0Eo&e=> > > Your patch encodes it as : > 77 22 04 74 65 73 74 03 6f 72 67 00 03 6f 76 6e c0 05 03 61 62 63 c0 0a 03 > 64 65 66 c0 0a 03 67 68 69 c0 0a > > But dnsmasq does encoding a little differently. > 77 2e 04 74 65 73 74 03 6f 72 67 00 03 6f 76 6e c0 05 03 61 62 63 03 6f 76 > 6e c0 05 03 64 65 66 03 6f 76 6e c0 05 03 67 68 69 03 6f 76 6e c0 05 > > Can you please check and verify once ? > Hi Numan, they both appear to be correct to me. dnsmasq uses fewer domain compression pointers than this patch does. dnsmasq compresses every occurrence of ".org" after the first into "c0 05". However, it repeats the explicit use of "ovn" throughout. This patch takes it a step further by encoding ".org" into "c0 05" but also encoding "ovn.org [ovn.org]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.org&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=4F-Kh0fA52dZU35XPtUyphwquItwevusYr5K--o-DxQ&e=>" into "c0 0a" (which itself compresses the ".org" into "c0 05"). This results in a smaller message that still decodes to the same value. Thanks Mark for verifying it further. I was just making sure that the present patch is correct taking dnsmasq as reference. Thanks Numan > I'm fine with your patch. But I just want to be sure. > > > --- >>> lib/actions.c | 90 >>> ++++++++++++++++++++++++++++++++++++++++++++++++++++- >>> lib/ovn-l7.h | 3 ++ >>> northd/ovn-northd.c | 1 + >>> ovn-nb.xml | 13 ++++++++ >>> ovn-sb.ovsschema | 6 ++-- >>> ovn-sb.xml | 11 +++++++ >>> tests/ovn.at [ovn.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=USGZ4IKQ9s0-Twx9pi-r04faUbIOvuCLhBZPRFaUgVk&e=> | 36 +++++++++++++++++++++ >>> tests/test-ovn.c | 1 + >>> 8 files changed, 157 insertions(+), 4 deletions(-) >>> >>> diff --git a/lib/actions.c b/lib/actions.c >>> index 9baa90f..ad7ae78 100644 >>> --- a/lib/actions.c >>> +++ b/lib/actions.c >>> @@ -1982,7 +1982,8 @@ parse_gen_opt(struct action_context *ctx, struct >>> ovnact_gen_option *o, >>> return; >>> } >>> >>> - if (!strcmp(o->option->type, "str")) { >>> + 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); >>> @@ -2317,6 +2318,93 @@ encode_put_dhcpv4_option(const struct >>> ovnact_gen_option *o, >>> 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 [abc.com]<https://urldefense.proofpoint.com/v2/url?u=http-3A__abc.com&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=k9RlSREUufW_ffPfyx8MMjB16egctPcMAFc0jD9DP1w&e=>" and "xyz.abc.com [xyz.abc.com]<https://urldefense.proofpoint.com/v2/url?u=http-3A__xyz.abc.com&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=9SElXIVG_arTMr9XVEh2J5HWNktk644qiVJkqUM6W6I&e=>". >>> + * >>> + * >>> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+ >>> + * |119|14 | 3 |'a'|'b'|'c'| 3 |'c'|'o'|'m'| 0 >>> |'x'|'y'|'z'|xC0|x00| >>> + * >>> +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+ >>> + * >>> + * The encoding of "abc.com [abc.com]<https://urldefense.proofpoint.com/v2/url?u=http-3A__abc.com&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=k9RlSREUufW_ffPfyx8MMjB16egctPcMAFc0jD9DP1w&e=>" ends with 0 to mark the end of the >>> + * domain name as required by RFC 1035. >>> + * >>> + * The encoding of "xyz" (for "xyz.abc.com [xyz.abc.com]<https://urldefense.proofpoint.com/v2/url?u=http-3A__xyz.abc.com&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=9SElXIVG_arTMr9XVEh2J5HWNktk644qiVJkqUM6W6I&e=>") 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 [abc.com]<https://urldefense.proofpoint.com/v2/url?u=http-3A__abc.com&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=k9RlSREUufW_ffPfyx8MMjB16egctPcMAFc0jD9DP1w&e=>"). >>> + * >>> + * 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)); >>> + uint16_t encode_offset = 0; >>> + struct shash label_offset_map; >>> + shash_init(&label_offset_map); >>> >> > You can init shash during declaration as: > struct shash label_offset_map = SHASH_INITIALIZER(&label_offset_map); > > > >> + char *domain_list = xstrdup(c->string), *dom_ptr = NULL; >>> + char *suffix = xzalloc(strlen(domain_list)); >>> + 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; >>> + } >>> + strcpy(suffix, domain); >>> >> > checkpatch complains to use ovs_strlcpy. Please use that instead of strcpy. > > >> + char *label; >>> + for (label = strtok_r(domain, ".", &domain); >>> + label != NULL; >>> + label = strtok_r(NULL, ".", &domain)) { >>> + /* Check if we have already encoded this suffix. >>> + * If yes, fill in the reference and break. */ >>> + uint16_t *get_offset; >>> + get_offset = shash_find_data(&label_offset_map, suffix); >>> + if (get_offset != NULL) { >>> + ovs_be16 temp = htons(0xc000) | htons(*get_offset); >>> + memcpy(dns_encoded + encode_offset, &temp, >>> + sizeof(temp)); >>> + encode_offset += sizeof(temp); >>> + break; >>> + } else { >>> + /* The suffix was not encoded before, encode it now >>> + * and add the offset to the label_offset_map. */ >>> + uint16_t *set_offset = xzalloc(sizeof(uint16_t)); >>> + *set_offset = encode_offset; >>> + shash_add_once(&label_offset_map, suffix, >>> set_offset); >>> + >>> + 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; >>> + } >>> + strcpy(suffix, domain); >>> >> > Same here. > > > >> + } >>> + /* 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: >>> + free(suffix); >>> + free(domain_list); >>> + free(dns_encoded); >>> + struct shash_node *node, *next; >>> + SHASH_FOR_EACH_SAFE (node, next, &label_offset_map) { >>> + shash_delete(&label_offset_map, node); >>> + } >>> + shash_destroy(&label_offset_map); >>> >> > I think there is a memory leak here since shash_delete doesn't free the > shash data which you allocated for 'set_offset'. > > I think instead of using SHASH_FOR_EACH_SAFE and then > calling shash_destroy(), > you can just do shash_destroy_free_data(). > > > } >>> } >>> >>> diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h >>> index 22a2153..cea97b9 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) \ >>> @@ -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 6a9b097..22891c1 100644 >>> --- a/northd/ovn-northd.c >>> +++ b/northd/ovn-northd.c >>> @@ -11345,6 +11345,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[] = { >>> diff --git a/ovn-nb.xml b/ovn-nb.xml >>> index 8368d51..80e843c 100644 >>> --- a/ovn-nb.xml >>> +++ b/ovn-nb.xml >>> @@ -2950,6 +2950,19 @@ >>> </p> >>> </column> >>> </group> >>> + >>> + <group title=" DHCP Options of type domains"> >>> + <p> >>> + These options accept string value which is a comma separated >>> + list of domain names. The domain names are encoded based on >>> RFC 1035. >>> + </p> >>> + >>> + <column name="options" key="domain_search_list"> >>> + <p> >>> + The DHCPv4 option code for this option is 119. >>> + </p> >>> + </column> >>> + </group> >>> </group> >>> >>> <group title="DHCPv6 options"> >>> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema >>> index 2ec729b..99c5de8 100644 >>> --- a/ovn-sb.ovsschema >>> +++ b/ovn-sb.ovsschema >>> @@ -1,7 +1,7 @@ >>> { >>> "name": "OVN_Southbound", >>> - "version": "2.8.1", >>> - "cksum": "236203406 21905", >>> + "version": "2.8.2", >>> + "cksum": "464326363 21916", >>> "tables": { >>> "SB_Global": { >>> "columns": { >>> @@ -218,7 +218,7 @@ >>> "type": "string", >>> "enum": ["set", ["bool", "uint8", "uint16", >>> "uint32", >>> "ipv4", "static_routes", "str", >>> - "host_id"]]}}}}, >>> + "host_id", "domains"]]}}}}, >>> "isRoot": true}, >>> "DHCPv6_Options": { >>> "columns": { >>> diff --git a/ovn-sb.xml b/ovn-sb.xml >>> index 208fb93..a6da80b 100644 >>> --- a/ovn-sb.xml >>> +++ b/ovn-sb.xml >>> @@ -3232,6 +3232,17 @@ tcp.flags = RST; >>> </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 [ovn.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=USGZ4IKQ9s0-Twx9pi-r04faUbIOvuCLhBZPRFaUgVk&e=> b/tests/ovn.at [ovn.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=USGZ4IKQ9s0-Twx9pi-r04faUbIOvuCLhBZPRFaUgVk&e=> >>> index 7622b74..6093991 100644 >>> --- a/tests/ovn.at [ovn.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=USGZ4IKQ9s0-Twx9pi-r04faUbIOvuCLhBZPRFaUgVk&e=> >>> +++ b/tests/ovn.at [ovn.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=USGZ4IKQ9s0-Twx9pi-r04faUbIOvuCLhBZPRFaUgVk&e=> >>> @@ -1218,6 +1218,9 @@ reg0[15] = >>> put_dhcp_opts(offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.255.0, >>> 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 [30.0.0.0]<https://urldefense.proofpoint.com/v2/url?u=http-3A__30.0.0.0_24-2C10.0.0.4-2C40.0.0.0_16-2C10.0.0.6-2C0.0.0.0_0-2C10.0.0.1&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=yrStzJV0XFK5WQ1FsZ1sLTdwYfvz5QEgTOZiObFKT58&e=> >>> },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 [30.0.0.0]<https://urldefense.proofpoint.com/v2/url?u=http-3A__30.0.0.0_24&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=op7Kuh1cBOFxUQZwTCPiaBh8zrHevcVbrMjTIXs-LrQ&e=>, 10.0.0.4, 40.0.0.0/16 [40.0.0.0]<https://urldefense.proofpoint.com/v2/url?u=http-3A__40.0.0.0_16&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=zL8RlP1dqzSZOoxzp3fVoTeVt5qaZYlH7iw31pDPZXI&e=>, 10.0.0.6, 0.0.0.0/0 [0.0.0.0]<https://urldefense.proofpoint.com/v2/url?u=http-3A__0.0.0.0_0&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=yt9Qm2XXNLzTZzqoc7Vlncp7owiUvPwYpLa86i4Ij08&e=>, 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 [ovn.org]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.org&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=4F-Kh0fA52dZU35XPtUyphwquItwevusYr5K--o-DxQ&e=>",wpad="https://example.org [example.org]<https://urldefense.proofpoint.com/v2/url?u=https-3A__example.org&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=f-DgUnVu-5YNOk_I0PfKrsTKG5f9_o1GT3M7isPMl2I&e=>",bootfile_name=" >>> https://127.0.0.1/boot.ipxe [127.0.0.1]<https://urldefense.proofpoint.com/v2/url?u=https-3A__127.0.0.1_boot.ipxe&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=-zZ5XsHdvqCj9TMOMLYspyLpHYhZwpJIuHujt71tkmk&e=>",path_prefix="/tftpboot",domain_search_list=" >>> ovn.org [ovn.org]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.org&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=4F-Kh0fA52dZU35XPtUyphwquItwevusYr5K--o-DxQ&e=>,abc.ovn.org [abc.ovn.org]<https://urldefense.proofpoint.com/v2/url?u=http-3A__abc.ovn.org&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=7q3hlfdvr-hOcN9s1CWD3id1kwPf3ytmQ_mlK1qLtOE&e=>"); >>> + 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 [ovn.org]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.org&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=4F-Kh0fA52dZU35XPtUyphwquItwevusYr5K--o-DxQ&e=>", >>> wpad = "https://example.org [example.org]<https://urldefense.proofpoint.com/v2/url?u=https-3A__example.org&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=f-DgUnVu-5YNOk_I0PfKrsTKG5f9_o1GT3M7isPMl2I&e=>", bootfile_name = " >>> https://127.0.0.1/boot.ipxe [127.0.0.1]<https://urldefense.proofpoint.com/v2/url?u=https-3A__127.0.0.1_boot.ipxe&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=-zZ5XsHdvqCj9TMOMLYspyLpHYhZwpJIuHujt71tkmk&e=>", path_prefix = "/tftpboot", >>> domain_search_list = "ovn.org [ovn.org]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.org&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=4F-Kh0fA52dZU35XPtUyphwquItwevusYr5K--o-DxQ&e=>,abc.ovn.org [abc.ovn.org]<https://urldefense.proofpoint.com/v2/url?u=http-3A__abc.ovn.org&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=7q3hlfdvr-hOcN9s1CWD3id1kwPf3ytmQ_mlK1qLtOE&e=>"); >>> + 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) >>> >> > Can you add a few more cases here ? Like the above example I gave ? > > Thanks > Numan > > >> >>> 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. >>> @@ -1235,6 +1238,8 @@ reg1[0] = put_dhcp_opts(offerip="xyzzy"); >>> DHCPv4 option offerip requires numeric value. >>> reg1[0] = put_dhcp_opts(offerip=1.2.3.4, domain_name=1.2.3.4); >>> DHCPv4 option domain_name requires string value. >>> +reg1[0] = put_dhcp_opts(offerip=1.2.3.4, domain_search_list=1.2.3.4); >>> + DHCPv4 option domain_search_list requires string value. >>> >>> # nd_ns >>> nd_ns { nd.target = xxreg0; output; }; >>> @@ -5614,6 +5619,37 @@ AT_CHECK([cat 2.packets | cut -c -48], [0], >>> [expout]) >>> cat 2.expected | cut -c 53- > expout >>> AT_CHECK([cat 2.packets | cut -c 53-], [0], [expout]) >>> >>> +reset_pcap_file hv1-vif1 hv1/vif1 >>> +reset_pcap_file hv1-vif2 hv1/vif2 >>> +rm -f 1.expected >>> +rm -f 2.expected >>> + >>> +# Set domain search list option for ls1 >>> +echo "------ Set domain search list --------" >>> +ovn-nbctl dhcp-options-set-options $d1 server_id=10.0.0.1 \ >>> +server_mac=ff:10:00:00:00:01 lease_time=3600 router=10.0.0.1 \ >>> +domain_search_list=\"test1.com [test1.com]<https://urldefense.proofpoint.com/v2/url?u=http-3A__test1.com&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=Nda86WfFsTSiDqmEBMnnOb5oK8sBnldRW4LjqSt5OMI&e=>,test2.com [test2.com]<https://urldefense.proofpoint.com/v2/url?u=http-3A__test2.com&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=8KalKuMp_mGuW4wxno9y7w_OD3OrDR-bTJsna6Pjc-E&e=>\" >>> +echo "------------------------------------------" >>> + >>> +# Send DHCPREQUEST in the SELECTING/INIT-REBOOT state with the offered IP >>> +# address in the Requested IP Address option. >>> +offer_ip=`ip_to_hex 10 0 0 6` >>> +server_ip=`ip_to_hex 10 0 0 1` >>> +ciaddr=`ip_to_hex 0 0 0 0` >>> +request_ip=$offer_ip >>> >>> +expected_dhcp_opts=771305746573743103636f6d00057465737432c006330400000e100104ffffff0003040a00000136040a000001 >>> +test_dhcp 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 >>> ff1000000001 $server_ip 05 $expected_dhcp_opts >>> + >>> +# NXT_RESUMEs should be 12. >>> +OVS_WAIT_UNTIL([test 12 = `cat ofctl_monitor*.log | grep -c NXT_RESUME`]) >>> + >>> +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in [ovs-pcap.in]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovs-2Dpcap.in&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=amWND7a1cgSvZh5wTu_ig_Ad7vkGwTHVZhDHTMLnn80&e=>" hv1/vif2-tx.pcap > 2.packets >>> +cat 2.expected | cut -c -48 > expout >>> +AT_CHECK([cat 2.packets | cut -c -48], [0], [expout]) >>> +# Skipping the IPv4 checksum. >>> +cat 2.expected | cut -c 53- > expout >>> +AT_CHECK([cat 2.packets | cut -c 53-], [0], [expout]) >>> + >>> OVN_CLEANUP([hv1]) >>> >>> AT_CLEANUP >>> diff --git a/tests/test-ovn.c b/tests/test-ovn.c >>> index 4391694..b43f67f 100644 >>> --- a/tests/test-ovn.c >>> +++ b/tests/test-ovn.c >>> @@ -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 >>> dev@openvswitch.org<mailto:dev@openvswitch.org> >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev [mail.openvswitch.org]<https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=rwkIHGPmlnDVOvusT4qwmexfFeeqIIY_7f0H8Mm3dtY&e=> >>> >>> > _______________________________________________ > dev mailing list > dev@openvswitch.org<mailto:dev@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev [mail.openvswitch.org]<https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwMFaQ&c=s883GpUCOChKOHiocYtGcg&r=dftLCwr11LPmmIAnjdakaot5N4f-Q2Qfwcs7XdgQZXE&m=6SLkwkTQcOAVG-AUBtO73GHZBCTA2MxZj4MGbOxG9S4&s=rwkIHGPmlnDVOvusT4qwmexfFeeqIIY_7f0H8Mm3dtY&e=> >
diff --git a/lib/actions.c b/lib/actions.c index 9baa90f..ad7ae78 100644 --- a/lib/actions.c +++ b/lib/actions.c @@ -1982,7 +1982,8 @@ parse_gen_opt(struct action_context *ctx, struct ovnact_gen_option *o, return; } - if (!strcmp(o->option->type, "str")) { + 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); @@ -2317,6 +2318,93 @@ encode_put_dhcpv4_option(const struct ovnact_gen_option *o, 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"). + * + * 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)); + uint16_t encode_offset = 0; + struct shash label_offset_map; + shash_init(&label_offset_map); + char *domain_list = xstrdup(c->string), *dom_ptr = NULL; + char *suffix = xzalloc(strlen(domain_list)); + 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; + } + strcpy(suffix, domain); + char *label; + for (label = strtok_r(domain, ".", &domain); + label != NULL; + label = strtok_r(NULL, ".", &domain)) { + /* Check if we have already encoded this suffix. + * If yes, fill in the reference and break. */ + uint16_t *get_offset; + get_offset = shash_find_data(&label_offset_map, suffix); + if (get_offset != NULL) { + ovs_be16 temp = htons(0xc000) | htons(*get_offset); + memcpy(dns_encoded + encode_offset, &temp, + sizeof(temp)); + encode_offset += sizeof(temp); + break; + } else { + /* The suffix was not encoded before, encode it now + * and add the offset to the label_offset_map. */ + uint16_t *set_offset = xzalloc(sizeof(uint16_t)); + *set_offset = encode_offset; + shash_add_once(&label_offset_map, suffix, set_offset); + + 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; + } + strcpy(suffix, domain); + } + /* 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: + free(suffix); + free(domain_list); + free(dns_encoded); + struct shash_node *node, *next; + SHASH_FOR_EACH_SAFE (node, next, &label_offset_map) { + shash_delete(&label_offset_map, node); + } + shash_destroy(&label_offset_map); } } diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h index 22a2153..cea97b9 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) \ @@ -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 6a9b097..22891c1 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -11345,6 +11345,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[] = { diff --git a/ovn-nb.xml b/ovn-nb.xml index 8368d51..80e843c 100644 --- a/ovn-nb.xml +++ b/ovn-nb.xml @@ -2950,6 +2950,19 @@ </p> </column> </group> + + <group title=" DHCP Options of type domains"> + <p> + These options accept string value which is a comma separated + list of domain names. The domain names are encoded based on RFC 1035. + </p> + + <column name="options" key="domain_search_list"> + <p> + The DHCPv4 option code for this option is 119. + </p> + </column> + </group> </group> <group title="DHCPv6 options"> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema index 2ec729b..99c5de8 100644 --- a/ovn-sb.ovsschema +++ b/ovn-sb.ovsschema @@ -1,7 +1,7 @@ { "name": "OVN_Southbound", - "version": "2.8.1", - "cksum": "236203406 21905", + "version": "2.8.2", + "cksum": "464326363 21916", "tables": { "SB_Global": { "columns": { @@ -218,7 +218,7 @@ "type": "string", "enum": ["set", ["bool", "uint8", "uint16", "uint32", "ipv4", "static_routes", "str", - "host_id"]]}}}}, + "host_id", "domains"]]}}}}, "isRoot": true}, "DHCPv6_Options": { "columns": { diff --git a/ovn-sb.xml b/ovn-sb.xml index 208fb93..a6da80b 100644 --- a/ovn-sb.xml +++ b/ovn-sb.xml @@ -3232,6 +3232,17 @@ tcp.flags = RST; </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 7622b74..6093991 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -1218,6 +1218,9 @@ reg0[15] = put_dhcp_opts(offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.255.0, 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. @@ -1235,6 +1238,8 @@ reg1[0] = put_dhcp_opts(offerip="xyzzy"); DHCPv4 option offerip requires numeric value. reg1[0] = put_dhcp_opts(offerip=1.2.3.4, domain_name=1.2.3.4); DHCPv4 option domain_name requires string value. +reg1[0] = put_dhcp_opts(offerip=1.2.3.4, domain_search_list=1.2.3.4); + DHCPv4 option domain_search_list requires string value. # nd_ns nd_ns { nd.target = xxreg0; output; }; @@ -5614,6 +5619,37 @@ AT_CHECK([cat 2.packets | cut -c -48], [0], [expout]) cat 2.expected | cut -c 53- > expout AT_CHECK([cat 2.packets | cut -c 53-], [0], [expout]) +reset_pcap_file hv1-vif1 hv1/vif1 +reset_pcap_file hv1-vif2 hv1/vif2 +rm -f 1.expected +rm -f 2.expected + +# Set domain search list option for ls1 +echo "------ Set domain search list --------" +ovn-nbctl dhcp-options-set-options $d1 server_id=10.0.0.1 \ +server_mac=ff:10:00:00:00:01 lease_time=3600 router=10.0.0.1 \ +domain_search_list=\"test1.com,test2.com\" +echo "------------------------------------------" + +# Send DHCPREQUEST in the SELECTING/INIT-REBOOT state with the offered IP +# address in the Requested IP Address option. +offer_ip=`ip_to_hex 10 0 0 6` +server_ip=`ip_to_hex 10 0 0 1` +ciaddr=`ip_to_hex 0 0 0 0` +request_ip=$offer_ip +expected_dhcp_opts=771305746573743103636f6d00057465737432c006330400000e100104ffffff0003040a00000136040a000001 +test_dhcp 2 f00000000002 03 0 $ciaddr $offer_ip $request_ip 0 ff1000000001 $server_ip 05 $expected_dhcp_opts + +# NXT_RESUMEs should be 12. +OVS_WAIT_UNTIL([test 12 = `cat ofctl_monitor*.log | grep -c NXT_RESUME`]) + +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap > 2.packets +cat 2.expected | cut -c -48 > expout +AT_CHECK([cat 2.packets | cut -c -48], [0], [expout]) +# Skipping the IPv4 checksum. +cat 2.expected | cut -c 53- > expout +AT_CHECK([cat 2.packets | cut -c 53-], [0], [expout]) + OVN_CLEANUP([hv1]) AT_CLEANUP diff --git a/tests/test-ovn.c b/tests/test-ovn.c index 4391694..b43f67f 100644 --- a/tests/test-ovn.c +++ b/tests/test-ovn.c @@ -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);