* [PATCH v3] dhcp, dhcpv6: Add hostname and client fqdn ops @ 2024-11-27 15:33 Enrique Llorente 2024-11-27 23:01 ` Stefano Brivio 0 siblings, 1 reply; 3+ messages in thread From: Enrique Llorente @ 2024-11-27 15:33 UTC (permalink / raw) To: passt-dev; +Cc: Enrique Llorente Both DHCPv4 and DHCPv6 has the capability to pass the hostname to clients, the DHCPv4 uses option 12 (hostname) while the DHCPv6 uses option 39 (client fqdn), for some virt deployments like kubevirt is expected to have the VirtualMachine name as the guest hostname. This change add the following arguments: - -H --hostname NAME to configure the hostname DHCPv4 option(12) - --fqdn NAME to configure client fqdn option for both DHCPv4(81) and DHCPv6(39) Signed-off-by: Enrique Llorente <ellorent@redhat.com> --- conf.c | 23 ++++++++++++++++++++--- dhcp.c | 15 ++++++++++++++- dhcpv6.c | 44 +++++++++++++++++++++++++++++++++++++++++++- passt.h | 4 ++++ test/lib/setup | 10 +++++----- test/passt.mbuto | 6 ++++-- test/passt/dhcp | 17 ++++++++++++++++- util.c | 21 +++++++++++++++++++++ util.h | 2 ++ 9 files changed, 129 insertions(+), 13 deletions(-) diff --git a/conf.c b/conf.c index 86566db..e9cf491 100644 --- a/conf.c +++ b/conf.c @@ -849,7 +849,9 @@ static void usage(const char *name, FILE *f, int status) " --freebind Bind to any address for forwarding\n" " --no-map-gw Don't map gateway address to host\n" " -4, --ipv4-only Enable IPv4 operation only\n" - " -6, --ipv6-only Enable IPv6 operation only\n"); + " -6, --ipv6-only Enable IPv6 operation only\n" + " -H, --hostname NAME Hostname to configure client with\n" + " --fqdn NAME The FQDN to configure client with\n"); if (strstr(name, "pasta")) goto pasta_opts; @@ -1266,6 +1268,7 @@ void conf(struct ctx *c, int argc, char **argv) {"outbound", required_argument, NULL, 'o' }, {"dns", required_argument, NULL, 'D' }, {"search", required_argument, NULL, 'S' }, + {"hostname", required_argument, NULL, 'H' }, {"no-tcp", no_argument, &c->no_tcp, 1 }, {"no-udp", no_argument, &c->no_udp, 1 }, {"no-icmp", no_argument, &c->no_icmp, 1 }, @@ -1305,6 +1308,7 @@ void conf(struct ctx *c, int argc, char **argv) {"map-guest-addr", required_argument, NULL, 22 }, {"host-lo-to-ns-lo", no_argument, NULL, 23 }, {"dns-host", required_argument, NULL, 24 }, + {"fqdn", required_argument, NULL, 25 }, { 0 }, }; const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt"; @@ -1327,9 +1331,9 @@ void conf(struct ctx *c, int argc, char **argv) if (c->mode == MODE_PASTA) { c->no_dhcp_dns = c->no_dhcp_dns_search = 1; fwd_default = FWD_AUTO; - optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:"; + optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:H:46t:u:T:U:"; } else { - optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:"; + optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:H:461t:u:"; } c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET; @@ -1682,6 +1686,19 @@ void conf(struct ctx *c, int argc, char **argv) c->one_off = true; break; + case 'H': + ret = snprintf(c->hostname.n, sizeof(c->hostname.n), + "%s", optarg); + if (ret <= 0 || ret >= (int)sizeof(c->hostname.n)) + die("Invalid hostname: %s", optarg); + break; + case 25: + ret = snprintf(c->fqdn.n, sizeof(c->fqdn.n), + "%s", optarg); + if (ret <= 0 || ret >= (int)sizeof(c->fqdn.n)) + die("Invalid fqdn: %s", optarg); + break; + case 't': case 'u': case 'T': diff --git a/dhcp.c b/dhcp.c index a06f143..e05bfc6 100644 --- a/dhcp.c +++ b/dhcp.c @@ -275,7 +275,7 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len) */ int dhcp(const struct ctx *c, const struct pool *p) { - size_t mlen, dlen, offset = 0, opt_len, opt_off = 0; + size_t mlen, dlen, offset = 0, opt_len, opt_off = 0, hostname_len, fqdn_len = 0; char macstr[ETH_ADDRSTRLEN]; const struct ethhdr *eh; const struct iphdr *iph; @@ -375,6 +375,19 @@ int dhcp(const struct ctx *c, const struct pool *p) opts[6].slen += sizeof(uint32_t); } + hostname_len = strlen(c->hostname.n); + if ( hostname_len > 0 ) { + opts[12].slen = hostname_len; + memcpy(opts[12].s, &c->hostname.n, hostname_len); + } + + fqdn_len = strlen(c->fqdn.n); + if ( fqdn_len > 0 ) { + opts[81].slen = fqdn_len+3; + memcpy(opts[81].s+3, &c->fqdn.n, fqdn_len); + } + + if (!c->no_dhcp_dns_search) opt_set_dns_search(c, sizeof(m->o)); diff --git a/dhcpv6.c b/dhcpv6.c index 0523bba..88c0978 100644 --- a/dhcpv6.c +++ b/dhcpv6.c @@ -48,6 +48,7 @@ struct opt_hdr { # define STATUS_NOTONLINK htons_constant(4) # define OPT_DNS_SERVERS htons_constant(23) # define OPT_DNS_SEARCH htons_constant(24) +# define OPT_CLIENT_FQDN htons_constant(39) #define STR_NOTONLINK "Prefix not appropriate for link." uint16_t l; @@ -163,6 +164,18 @@ struct opt_dns_search { char list[MAXDNSRCH * NS_MAXDNAME]; } __attribute__((packed)); +/** + * struct opt_client_fqdn - Client FQDN option (RFC 4704) + * @hdr: Option header + * @flags: Flags as stated at RFC 4704 (always zero for us) + * @fqdn: Client fqdn + */ +struct opt_client_fqdn{ + struct opt_hdr hdr; + uint8_t flags; + char domain_name[253 /*RFC 1035*/ ]; +} __attribute__((packed)); + /** * struct msg_hdr - DHCPv6 client/server message header * @type: DHCP message type @@ -193,6 +206,7 @@ struct msg_hdr { * @client_id: Client Identifier, variable length * @dns_servers: DNS Recursive Name Server, here just for storage size * @dns_search: Domain Search List, here just for storage size + * @client_fqdn: Client FQDN, variable length */ static struct resp_t { struct msg_hdr hdr; @@ -203,10 +217,10 @@ static struct resp_t { struct opt_client_id client_id; struct opt_dns_servers dns_servers; struct opt_dns_search dns_search; + struct opt_client_fqdn client_fqdn; } __attribute__((__packed__)) resp = { { 0 }, SERVER_ID, - { { OPT_IA_NA, OPT_SIZE_CONV(sizeof(struct opt_ia_na) + sizeof(struct opt_ia_addr) - sizeof(struct opt_hdr)) }, @@ -228,6 +242,10 @@ static struct resp_t { { { OPT_DNS_SEARCH, 0, }, { 0 }, }, + + { { OPT_CLIENT_FQDN, 0, }, + 0, { 0 }, + }, }; static const struct opt_status_code sc_not_on_link = { @@ -411,6 +429,29 @@ search: return offset; } +/** + * dhcpv6_client_fqdn_fill() - Fill in client FQDN option + * @c: Execution context + * @buf: Response message buffer where options will be appended + * @offset: Offset in message buffer for new options + * + * Return: updated length of response message buffer. + */ +static size_t dhcpv6_client_fqdn_fill(const struct ctx *c, char *buf, int offset) +{ + uint16_t fqdn_len = strlen(c->fqdn.n); + if (fqdn_len > 0) { + struct opt_client_fqdn *o = (struct opt_client_fqdn *)(buf + offset); + size_t encoded_fqdn_len = encode_domain_name(c->fqdn.n, fqdn_len, o->domain_name); + size_t opt_len = encoded_fqdn_len + 1; + o->hdr.t = OPT_CLIENT_FQDN; + o->hdr.l = htons(opt_len); + o->flags = 0x00; + offset += sizeof(struct opt_hdr) + opt_len; + } + return offset; +} + /** * dhcpv6() - Check if this is a DHCPv6 message, reply as needed * @c: Execution context @@ -544,6 +585,7 @@ int dhcpv6(struct ctx *c, const struct pool *p, n = offsetof(struct resp_t, client_id) + sizeof(struct opt_hdr) + ntohs(client_id->l); n = dhcpv6_dns_fill(c, (char *)&resp, n); + n = dhcpv6_client_fqdn_fill(c, (char*)&resp, n); resp.hdr.xid = mh->xid; diff --git a/passt.h b/passt.h index 72c7f72..566d21d 100644 --- a/passt.h +++ b/passt.h @@ -205,6 +205,8 @@ struct ip6_ctx { * @ifi4: Index of template interface for IPv4, 0 if IPv4 disabled * @ip: IPv4 configuration * @dns_search: DNS search list + * @hostname: Client hostname + * @fqdn: Client FQDN * @ifi6: Index of template interface for IPv6, 0 if IPv6 disabled * @ip6: IPv6 configuration * @pasta_ifn: Name of namespace interface for pasta @@ -262,6 +264,8 @@ struct ctx { struct ip4_ctx ip4; struct fqdn dns_search[MAXDNSRCH]; + struct fqdn hostname; + struct fqdn fqdn; unsigned int ifi6; struct ip6_ctx ip6; diff --git a/test/lib/setup b/test/lib/setup index 5338393..a3fe578 100755 --- a/test/lib/setup +++ b/test/lib/setup @@ -49,7 +49,7 @@ setup_passt() { context_run passt "make clean" context_run passt "make valgrind" - context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -P ${STATESETUP}/passt.pid" + context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -H hostname1 --fqdn fqdn1.passt.top -P ${STATESETUP}/passt.pid" # pidfile isn't created until passt is listening wait_for [ -f "${STATESETUP}/passt.pid" ] @@ -146,11 +146,11 @@ setup_passt_in_ns() { if [ ${VALGRIND} -eq 1 ]; then context_run passt "make clean" context_run passt "make valgrind" - context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}" + context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H hostname1 --fqdn fqdn1.passt.top -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}" else context_run passt "make clean" context_run passt "make" - context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}" + context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H hostname1 --fqdn fqdn1.passt.top -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}" fi wait_for [ -f "${STATESETUP}/passt.pid" ] @@ -215,7 +215,7 @@ setup_two_guests() { [ ${DEBUG} -eq 1 ] && __opts="${__opts} -d" [ ${TRACE} -eq 1 ] && __opts="${__opts} --trace" - context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} -t 10001 -u 10001" + context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} --fqdn fqdn1.passt.top -H hostname1 -t 10001 -u 10001" wait_for [ -f "${STATESETUP}/passt_1.pid" ] __opts= @@ -223,7 +223,7 @@ setup_two_guests() { [ ${DEBUG} -eq 1 ] && __opts="${__opts} -d" [ ${TRACE} -eq 1 ] && __opts="${__opts} --trace" - context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} -t 10004 -u 10004" + context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} --hostname hostname2 --fqdn fqdn2 -t 10004 -u 10004" wait_for [ -f "${STATESETUP}/passt_2.pid" ] GUEST_1_CID=94557 diff --git a/test/passt.mbuto b/test/passt.mbuto index 138d365..1e07693 100755 --- a/test/passt.mbuto +++ b/test/passt.mbuto @@ -13,7 +13,7 @@ PROGS="${PROGS:-ash,dash,bash ip mount ls insmod mkdir ln cat chmod lsmod modprobe find grep mknod mv rm umount jq iperf3 dhclient hostname sed tr chown sipcalc cut socat dd strace ping tail killall sleep sysctl - nproc tcp_rr tcp_crr udp_rr which tee seq bc sshd ssh-keygen cmp}" + nproc tcp_rr tcp_crr udp_rr which tee seq bc sshd ssh-keygen cmp env}" # OpenSSH 9.8 introduced split binaries, with sshd being the daemon, and # sshd-session the per-session program. We need the latter as well, and the path @@ -41,6 +41,7 @@ FIXUP="${FIXUP}"' #!/bin/sh LOG=/var/log/dhclient-script.log echo \${reason} \${interface} >> \$LOG +env >> \$LOG set >> \$LOG [ -n "\${new_interface_mtu}" ] && ip link set dev \${interface} mtu \${new_interface_mtu} @@ -54,7 +55,8 @@ set >> \$LOG [ -n "\${new_ip6_address}" ] && ip addr add \${new_ip6_address}/\${new_ip6_prefixlen} dev \${interface} [ -n "\${new_dhcp6_name_servers}" ] && for d in \${new_dhcp6_name_servers}; do echo "nameserver \${d}%\${interface}" >> /etc/resolv.conf; done [ -n "\${new_dhcp6_domain_search}" ] && (printf "search"; for d in \${new_dhcp6_domain_search}; do printf " %s" "\${d}"; done; printf "\n") >> /etc/resolv.conf -[ -n "\${new_host_name}" ] && hostname "\${new_host_name}" +[ -n "\${new_host_name}" ] && echo "\${new_host_name}" > /tmp/new_host_name +[ -n "\${new_fqdn_fqdn}" ] && echo "\${new_fqdn_fqdn}" > /tmp/new_fqdn_fqdn exit 0 EOF chmod 755 /sbin/dhclient-script diff --git a/test/passt/dhcp b/test/passt/dhcp index 9925ab9..f947fd1 100644 --- a/test/passt/dhcp +++ b/test/passt/dhcp @@ -11,7 +11,7 @@ # Copyright (c) 2021 Red Hat GmbH # Author: Stefano Brivio <sbrivio@redhat.com> -gtools ip jq dhclient sed tr +gtools ip jq dhclient sed tr hostname htools ip jq sed tr head test Interface name @@ -22,6 +22,7 @@ check [ -n "__IFNAME__" ] test DHCP: address guest /sbin/dhclient -4 __IFNAME__ +guest cat /var/log/dhclient-script.log gout ADDR ip -j -4 addr show|jq -rM '.[] | select(.ifname == "__IFNAME__").addr_info[0].local' hout HOST_ADDR ip -j -4 addr show|jq -rM '.[] | select(.ifname == "__HOST_IFNAME__").addr_info[0].local' check [ "__ADDR__" = "__HOST_ADDR__" ] @@ -47,10 +48,20 @@ gout SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^searc hout HOST_SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/' check [ "__SEARCH__" = "__HOST_SEARCH__" ] +test DHCP: Hostname +gout NEW_HOST_NAME cat /tmp/new_host_name +check [ "__NEW_HOST_NAME__" = "hostname1" ] + +test DHCP: Client FQDN +gout NEW_FQDN_FQDN cat /tmp/new_fqdn_fqdn +check [ "__NEW_FQDN_FQDN__" = "fqdn1.passt.top" ] + test DHCPv6: address +guest rm /tmp/new_fqdn_fqdn guest /sbin/dhclient -6 __IFNAME__ # Wait for DAD to complete guest while ip -j -6 addr show tentative | jq -e '.[].addr_info'; do sleep 0.1; done +guest cat /var/log/dhclient-script.log gout ADDR6 ip -j -6 addr show|jq -rM '[.[] | select(.ifname == "__IFNAME__").addr_info[] | select(.prefixlen == 128).local] | .[0]' hout HOST_ADDR6 ip -j -6 addr show|jq -rM '[.[] | select(.ifname == "__HOST_IFNAME6__").addr_info[] | select(.scope == "global" and .deprecated != true).local] | .[0]' check [ "__ADDR6__" = "__HOST_ADDR6__" ] @@ -70,3 +81,7 @@ test DHCPv6: search list gout SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/' hout HOST_SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/' check [ "__SEARCH6__" = "__HOST_SEARCH6__" ] + +test DHCPv6: Hostname +gout NEW_FQDN_FQDN cat /tmp/new_fqdn_fqdn +check [ "__NEW_FQDN_FQDN__" = "fqdn1.passt.top" ] diff --git a/util.c b/util.c index 55cae3f..b38aec7 100644 --- a/util.c +++ b/util.c @@ -837,3 +837,24 @@ void raw_random(void *buf, size_t buflen) if (random_read < buflen) die("Unexpected EOF on random data source"); } +/** + * encode_domain_name() - Encoding domain names according to RFC 3315 sec 8 + * @domain_name: Input domain name to encode + * @len: Domain name length + * @buf: Buffer to fill in with encoded domain name + * + * Returns: encoded domain name length + */ + +size_t encode_domain_name(const char* domain_name, size_t len, char* buf){ + buf[0] = strcspn(domain_name, "."); + char* p = buf + 1; + for (size_t i = 0; i < len; i++) { + if (domain_name[i] == '.') { + p[i] = strcspn(domain_name + i + 1, "."); + } else { + p[i] = domain_name[i]; + } + } + return len + 2; +} diff --git a/util.h b/util.h index 90428c4..8001ebb 100644 --- a/util.h +++ b/util.h @@ -305,4 +305,6 @@ static inline int wrap_accept4(int sockfd, struct sockaddr *addr, #define accept4(s, addr, addrlen, flags) \ wrap_accept4((s), (addr), (addrlen), (flags)) +size_t encode_domain_name(const char* domain_name, size_t len, char* buf); + #endif /* UTIL_H */ -- @@ -305,4 +305,6 @@ static inline int wrap_accept4(int sockfd, struct sockaddr *addr, #define accept4(s, addr, addrlen, flags) \ wrap_accept4((s), (addr), (addrlen), (flags)) +size_t encode_domain_name(const char* domain_name, size_t len, char* buf); + #endif /* UTIL_H */ -- 2.47.0 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3] dhcp, dhcpv6: Add hostname and client fqdn ops 2024-11-27 15:33 [PATCH v3] dhcp, dhcpv6: Add hostname and client fqdn ops Enrique Llorente @ 2024-11-27 23:01 ` Stefano Brivio 2024-12-02 11:47 ` Enrique Llorente Pastora 0 siblings, 1 reply; 3+ messages in thread From: Stefano Brivio @ 2024-11-27 23:01 UTC (permalink / raw) To: Enrique Llorente; +Cc: passt-dev On Wed, 27 Nov 2024 16:33:39 +0100 Enrique Llorente <ellorent@redhat.com> wrote: > Both DHCPv4 and DHCPv6 has the capability to pass the hostname to > clients, the DHCPv4 uses option 12 (hostname) while the DHCPv6 uses option 39 > (client fqdn), for some virt deployments like kubevirt is expected to > have the VirtualMachine name as the guest hostname. > > This change add the following arguments: > - -H --hostname NAME to configure the hostname DHCPv4 option(12) > - --fqdn NAME to configure client fqdn option for both DHCPv4(81) and > DHCPv6(39) > > Signed-off-by: Enrique Llorente <ellorent@redhat.com> This looks pretty good to me. A few comments below (some are details, some are serious issues): > --- > conf.c | 23 ++++++++++++++++++++--- > dhcp.c | 15 ++++++++++++++- > dhcpv6.c | 44 +++++++++++++++++++++++++++++++++++++++++++- > passt.h | 4 ++++ > test/lib/setup | 10 +++++----- > test/passt.mbuto | 6 ++++-- > test/passt/dhcp | 17 ++++++++++++++++- > util.c | 21 +++++++++++++++++++++ > util.h | 2 ++ > 9 files changed, 129 insertions(+), 13 deletions(-) > > diff --git a/conf.c b/conf.c > index 86566db..e9cf491 100644 > --- a/conf.c > +++ b/conf.c > @@ -849,7 +849,9 @@ static void usage(const char *name, FILE *f, int status) > " --freebind Bind to any address for forwarding\n" > " --no-map-gw Don't map gateway address to host\n" > " -4, --ipv4-only Enable IPv4 operation only\n" > - " -6, --ipv6-only Enable IPv6 operation only\n"); > + " -6, --ipv6-only Enable IPv6 operation only\n" > + " -H, --hostname NAME Hostname to configure client with\n" > + " --fqdn NAME The FQDN to configure client with\n"); A few comments here: - just "FQDN" instead of "The FQDN" makes it consistent with the description for -H - I would move those a bit up, right after the DNS options (-S), because that's where users are more likely to look for this stuff - now that you mention we're _configuring the client with_ those (I had in mind that we were just sending those via DHCP / DHCPv6... even though it's pretty much same thing), I wonder if we shouldn't also use the NAME from --hostname for the sethostname() call we have in pasta_spawn_cmd(). That is, if there's a "Hostname to configure the client with", it would be totally unexpected that we ignore that when setting the hostname with pasta - these need matching updates in the man page > > if (strstr(name, "pasta")) > goto pasta_opts; > @@ -1266,6 +1268,7 @@ void conf(struct ctx *c, int argc, char **argv) > {"outbound", required_argument, NULL, 'o' }, > {"dns", required_argument, NULL, 'D' }, > {"search", required_argument, NULL, 'S' }, > + {"hostname", required_argument, NULL, 'H' }, > {"no-tcp", no_argument, &c->no_tcp, 1 }, > {"no-udp", no_argument, &c->no_udp, 1 }, > {"no-icmp", no_argument, &c->no_icmp, 1 }, > @@ -1305,6 +1308,7 @@ void conf(struct ctx *c, int argc, char **argv) > {"map-guest-addr", required_argument, NULL, 22 }, > {"host-lo-to-ns-lo", no_argument, NULL, 23 }, > {"dns-host", required_argument, NULL, 24 }, > + {"fqdn", required_argument, NULL, 25 }, > { 0 }, > }; > const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt"; > @@ -1327,9 +1331,9 @@ void conf(struct ctx *c, int argc, char **argv) > if (c->mode == MODE_PASTA) { > c->no_dhcp_dns = c->no_dhcp_dns_search = 1; > fwd_default = FWD_AUTO; > - optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:"; > + optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:H:46t:u:T:U:"; > } else { > - optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:"; > + optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:H:461t:u:"; > } > > c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET; > @@ -1682,6 +1686,19 @@ void conf(struct ctx *c, int argc, char **argv) > > c->one_off = true; > break; > + case 'H': You moved 'H' in optstring to be just after 'S', now this should also go after case 'S'. > + ret = snprintf(c->hostname.n, sizeof(c->hostname.n), > + "%s", optarg); > + if (ret <= 0 || ret >= (int)sizeof(c->hostname.n)) So, we're accepting 1024 characters here, but the DHCP option can only be 255 characters long: $ unshare -rUn # echo $$ 369109 ...then: $ make clean; make valgrind $ gdb --args ./pasta -f --config-net -H hfoiewhfiohewifoewhfiowhfoiewhfowehfiewohfowhfieofhow.lll.38428.jjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjj.kkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkk -p hostname.pcap 369109 ...back to the namespace: # dhclient and: DHCP: offer to discover from b2:33:a6:a2:bf:8c Program received signal SIGSEGV, Segmentation fault. __memcpy_avx_unaligned_erms () at ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:523 523 ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S: No such file or directory. (gdb) (gdb) bt #0 __memcpy_avx_unaligned_erms () at ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:523 #1 0x00005555555620dd in memcpy (__len=<optimized out>, __src=0x55555559d6ec <opts+5772>, __dest=0x55555662f33c <pkt_buf+828>) at /usr/include/x86_64-linux-gnu/bits/string_fortified.h:29 #2 fill_one (offset=<synthetic pointer>, o=11, m=0x55555662f02a <pkt_buf+42>) at dhcp.c:138 #3 fill (m=<optimized out>) at dhcp.c:175 #4 dhcp (c=c@entry=0x7ffffff6ed60, p=p@entry=0x7ffffff6eb40) at dhcp.c:417 #5 0x0000555555570c94 in tap4_handler (c=c@entry=0x7ffffff6ed60, in=in@entry=0x5555574a8ee0 <pool_tap4_storage>, now=now@entry=0x7ffffff6ec50) at tap.c:681 #6 0x0000555555571971 in tap_handler (now=0x7ffffff6ec50, c=0x7ffffff6ed60) at tap.c:958 #7 tap_pasta_input (now=0x7ffffff6ec50, c=0x7ffffff6ed60) at tap.c:1133 #8 tap_handler_pasta (c=c@entry=0x7ffffff6ed60, events=events@entry=1, now=now@entry=0x7ffffff6ec50) at tap.c:1149 #9 0x0000555555558eaa in main (argc=<optimized out>, argv=<optimized out>) at passt.c:318 ...I think c->hostname should also be 253 characters long, as you now have for 'domain_name' in the DHCPv6 option. > + die("Invalid hostname: %s", optarg); > + break; > + case 25: This should be after all the numbered cases (now that would be case 27 after case 26), for consistency. > + ret = snprintf(c->fqdn.n, sizeof(c->fqdn.n), > + "%s", optarg); > + if (ret <= 0 || ret >= (int)sizeof(c->fqdn.n)) And here, RFC 4702 mandates option concatenation. This can still be 1024 characters (excluding the terminator), but it will crash in the same way if you handle it like this. More on option concatenation below. By the way, after introducing those snprintf() calls, I added a function, snprintf_check(), to take care of these checks in a more concise way. I think both options are fine: you can use it because it's convenient, or not use it for consistency (and one day I'll just do a mass conversion of those). > + die("Invalid fqdn: %s", optarg); > + break; > + > case 't': > case 'u': > case 'T': > diff --git a/dhcp.c b/dhcp.c > index a06f143..e05bfc6 100644 > --- a/dhcp.c > +++ b/dhcp.c > @@ -275,7 +275,7 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len) > */ > int dhcp(const struct ctx *c, const struct pool *p) > { > - size_t mlen, dlen, offset = 0, opt_len, opt_off = 0; > + size_t mlen, dlen, offset = 0, opt_len, opt_off = 0, hostname_len, fqdn_len = 0; These shouldn't exceed 80 columns (there's no reason here). You can add them on a separate line, because they're logically somewhat distinct. Why is fqdn_len initialised? There's no need to, it's always set anyway. > char macstr[ETH_ADDRSTRLEN]; > const struct ethhdr *eh; > const struct iphdr *iph; > @@ -375,6 +375,19 @@ int dhcp(const struct ctx *c, const struct pool *p) > opts[6].slen += sizeof(uint32_t); > } > > + hostname_len = strlen(c->hostname.n); > + if ( hostname_len > 0 ) { Coding style, for consistency with the rest: if (hostname_len > 0) { > + opts[12].slen = hostname_len; > + memcpy(opts[12].s, &c->hostname.n, hostname_len); > + } > + > + fqdn_len = strlen(c->fqdn.n); > + if ( fqdn_len > 0 ) { Same here. > + opts[81].slen = fqdn_len+3; Spaces around '+', and missing check on length. If the option is longer than 255 bytes (excluding type and length bytes), RFC 4702 mandates that we use option concatenation for it (as specified by RFC 3396). That is, fill() should be ready to concatenate, in the response, multiple parts of the same option (option code is repeated, first option encoding has 255 bytes as length, the next one has the remaining length). Option overloading (using 'sname' and 'file') is not mandatory, though, so we can set a 308-byte upper bound, which means we can change struct opts to have slightly bigger buffers (say, 308 bytes) for the options we want to send ('s'). An alternative could be to limit what can be passed to --fqdn to 252 bytes (because of "Flags", "RCODE1", and "RCODE2"). At that point, even if support for option concatenation is mandatory, we know that we'll never need it, so we can avoid implementing it. The whole thing would be slightly buggy, though, because an FQDN could be, for sure, 253 bytes. > + memcpy(opts[81].s+3, &c->fqdn.n, fqdn_len); Just like for option 39 in DHCPv6, we need to use the "canonical" (RFC 1035) encoding of the domain name. Unlike DHCPv6 option 39, we need set the 'E' bit in "Flags" to 1. > + } > + > + Excess newline. > if (!c->no_dhcp_dns_search) > opt_set_dns_search(c, sizeof(m->o)); > > diff --git a/dhcpv6.c b/dhcpv6.c > index 0523bba..88c0978 100644 > --- a/dhcpv6.c > +++ b/dhcpv6.c > @@ -48,6 +48,7 @@ struct opt_hdr { > # define STATUS_NOTONLINK htons_constant(4) > # define OPT_DNS_SERVERS htons_constant(23) > # define OPT_DNS_SEARCH htons_constant(24) > +# define OPT_CLIENT_FQDN htons_constant(39) This should be aligned to the other values with a tab, not spaces. > #define STR_NOTONLINK "Prefix not appropriate for link." > > uint16_t l; > @@ -163,6 +164,18 @@ struct opt_dns_search { > char list[MAXDNSRCH * NS_MAXDNAME]; > } __attribute__((packed)); > > +/** > + * struct opt_client_fqdn - Client FQDN option (RFC 4704) > + * @hdr: Option header > + * @flags: Flags as stated at RFC 4704 (always zero for us) "described by RFC 4704" > + * @fqdn: Client fqdn ...now you called this called 'domain_name'. > + */ > +struct opt_client_fqdn{ > + struct opt_hdr hdr; > + uint8_t flags; > + char domain_name[253 /*RFC 1035*/ ]; For consistency with coding style elsewhere: /* RFC 1035 */ > +} __attribute__((packed)); > + > /** > * struct msg_hdr - DHCPv6 client/server message header > * @type: DHCP message type > @@ -193,6 +206,7 @@ struct msg_hdr { > * @client_id: Client Identifier, variable length > * @dns_servers: DNS Recursive Name Server, here just for storage size > * @dns_search: Domain Search List, here just for storage size > + * @client_fqdn: Client FQDN, variable length > */ > static struct resp_t { > struct msg_hdr hdr; > @@ -203,10 +217,10 @@ static struct resp_t { > struct opt_client_id client_id; > struct opt_dns_servers dns_servers; > struct opt_dns_search dns_search; > + struct opt_client_fqdn client_fqdn; > } __attribute__((__packed__)) resp = { > { 0 }, > SERVER_ID, > - Unrelated change. > { { OPT_IA_NA, OPT_SIZE_CONV(sizeof(struct opt_ia_na) + > sizeof(struct opt_ia_addr) - > sizeof(struct opt_hdr)) }, > @@ -228,6 +242,10 @@ static struct resp_t { > { { OPT_DNS_SEARCH, 0, }, > { 0 }, > }, > + > + { { OPT_CLIENT_FQDN, 0, }, > + 0, { 0 }, > + }, > }; > > static const struct opt_status_code sc_not_on_link = { > @@ -411,6 +429,29 @@ search: > return offset; > } > > +/** > + * dhcpv6_client_fqdn_fill() - Fill in client FQDN option > + * @c: Execution context > + * @buf: Response message buffer where options will be appended > + * @offset: Offset in message buffer for new options > + * > + * Return: updated length of response message buffer. > + */ > +static size_t dhcpv6_client_fqdn_fill(const struct ctx *c, char *buf, int offset) > +{ > + uint16_t fqdn_len = strlen(c->fqdn.n); We usually add an empty line between declarations and code, for readability. > + if (fqdn_len > 0) { An early return might be more convenient. If not, excess whitespace between ) and {. > + struct opt_client_fqdn *o = (struct opt_client_fqdn *)(buf + offset); > + size_t encoded_fqdn_len = encode_domain_name(c->fqdn.n, fqdn_len, o->domain_name); These need to be wrapped to 80 columns. > + size_t opt_len = encoded_fqdn_len + 1; Same here, empty line would be nice. > + o->hdr.t = OPT_CLIENT_FQDN; > + o->hdr.l = htons(opt_len); > + o->flags = 0x00; > + offset += sizeof(struct opt_hdr) + opt_len; > + } > + return offset; > +} > + > /** > * dhcpv6() - Check if this is a DHCPv6 message, reply as needed > * @c: Execution context > @@ -544,6 +585,7 @@ int dhcpv6(struct ctx *c, const struct pool *p, > n = offsetof(struct resp_t, client_id) + > sizeof(struct opt_hdr) + ntohs(client_id->l); > n = dhcpv6_dns_fill(c, (char *)&resp, n); > + n = dhcpv6_client_fqdn_fill(c, (char*)&resp, n); > > resp.hdr.xid = mh->xid; > > diff --git a/passt.h b/passt.h > index 72c7f72..566d21d 100644 > --- a/passt.h > +++ b/passt.h > @@ -205,6 +205,8 @@ struct ip6_ctx { > * @ifi4: Index of template interface for IPv4, 0 if IPv4 disabled > * @ip: IPv4 configuration > * @dns_search: DNS search list > + * @hostname: Client hostname > + * @fqdn: Client FQDN I would call it "Guest" (or specify we send those to it) because otherwise it's not clear from just reading this that those are not the options the client sends. "Client hostname" seems to imply we store there the option 12 sent by a DHCP client. > * @ifi6: Index of template interface for IPv6, 0 if IPv6 disabled > * @ip6: IPv6 configuration > * @pasta_ifn: Name of namespace interface for pasta > @@ -262,6 +264,8 @@ struct ctx { > struct ip4_ctx ip4; > > struct fqdn dns_search[MAXDNSRCH]; > + struct fqdn hostname; > + struct fqdn fqdn; > > unsigned int ifi6; > struct ip6_ctx ip6; > diff --git a/test/lib/setup b/test/lib/setup > index 5338393..a3fe578 100755 > --- a/test/lib/setup > +++ b/test/lib/setup > @@ -49,7 +49,7 @@ setup_passt() { > > context_run passt "make clean" > context_run passt "make valgrind" > - context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -P ${STATESETUP}/passt.pid" > + context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -H hostname1 --fqdn fqdn1.passt.top -P ${STATESETUP}/passt.pid" Very minor: I'll actually run those tests on passt.top, so maybe we shouldn't use that as domain, because if we actually decide to change these tests and do something with it, the behaviour might depend on where tests run. I'd rather use passt.test: https://en.wikipedia.org/wiki/.test > > # pidfile isn't created until passt is listening > wait_for [ -f "${STATESETUP}/passt.pid" ] > @@ -146,11 +146,11 @@ setup_passt_in_ns() { > if [ ${VALGRIND} -eq 1 ]; then > context_run passt "make clean" > context_run passt "make valgrind" > - context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}" > + context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H hostname1 --fqdn fqdn1.passt.top -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}" > else > context_run passt "make clean" > context_run passt "make" > - context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}" > + context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H hostname1 --fqdn fqdn1.passt.top -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}" > fi > wait_for [ -f "${STATESETUP}/passt.pid" ] > > @@ -215,7 +215,7 @@ setup_two_guests() { > [ ${DEBUG} -eq 1 ] && __opts="${__opts} -d" > [ ${TRACE} -eq 1 ] && __opts="${__opts} --trace" > > - context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} -t 10001 -u 10001" > + context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} --fqdn fqdn1.passt.top -H hostname1 -t 10001 -u 10001" > wait_for [ -f "${STATESETUP}/passt_1.pid" ] > > __opts= > @@ -223,7 +223,7 @@ setup_two_guests() { > [ ${DEBUG} -eq 1 ] && __opts="${__opts} -d" > [ ${TRACE} -eq 1 ] && __opts="${__opts} --trace" > > - context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} -t 10004 -u 10004" > + context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} --hostname hostname2 --fqdn fqdn2 -t 10004 -u 10004" > wait_for [ -f "${STATESETUP}/passt_2.pid" ] > > GUEST_1_CID=94557 > diff --git a/test/passt.mbuto b/test/passt.mbuto > index 138d365..1e07693 100755 > --- a/test/passt.mbuto > +++ b/test/passt.mbuto > @@ -13,7 +13,7 @@ > PROGS="${PROGS:-ash,dash,bash ip mount ls insmod mkdir ln cat chmod lsmod > modprobe find grep mknod mv rm umount jq iperf3 dhclient hostname > sed tr chown sipcalc cut socat dd strace ping tail killall sleep sysctl > - nproc tcp_rr tcp_crr udp_rr which tee seq bc sshd ssh-keygen cmp}" > + nproc tcp_rr tcp_crr udp_rr which tee seq bc sshd ssh-keygen cmp env}" > > # OpenSSH 9.8 introduced split binaries, with sshd being the daemon, and > # sshd-session the per-session program. We need the latter as well, and the path > @@ -41,6 +41,7 @@ FIXUP="${FIXUP}"' > #!/bin/sh > LOG=/var/log/dhclient-script.log > echo \${reason} \${interface} >> \$LOG > +env >> \$LOG > set >> \$LOG > > [ -n "\${new_interface_mtu}" ] && ip link set dev \${interface} mtu \${new_interface_mtu} > @@ -54,7 +55,8 @@ set >> \$LOG > [ -n "\${new_ip6_address}" ] && ip addr add \${new_ip6_address}/\${new_ip6_prefixlen} dev \${interface} > [ -n "\${new_dhcp6_name_servers}" ] && for d in \${new_dhcp6_name_servers}; do echo "nameserver \${d}%\${interface}" >> /etc/resolv.conf; done > [ -n "\${new_dhcp6_domain_search}" ] && (printf "search"; for d in \${new_dhcp6_domain_search}; do printf " %s" "\${d}"; done; printf "\n") >> /etc/resolv.conf > -[ -n "\${new_host_name}" ] && hostname "\${new_host_name}" > +[ -n "\${new_host_name}" ] && echo "\${new_host_name}" > /tmp/new_host_name > +[ -n "\${new_fqdn_fqdn}" ] && echo "\${new_fqdn_fqdn}" > /tmp/new_fqdn_fqdn > exit 0 > EOF > chmod 755 /sbin/dhclient-script > diff --git a/test/passt/dhcp b/test/passt/dhcp > index 9925ab9..f947fd1 100644 > --- a/test/passt/dhcp > +++ b/test/passt/dhcp > @@ -11,7 +11,7 @@ > # Copyright (c) 2021 Red Hat GmbH > # Author: Stefano Brivio <sbrivio@redhat.com> > > -gtools ip jq dhclient sed tr > +gtools ip jq dhclient sed tr hostname > htools ip jq sed tr head > > test Interface name > @@ -22,6 +22,7 @@ check [ -n "__IFNAME__" ] > > test DHCP: address > guest /sbin/dhclient -4 __IFNAME__ > +guest cat /var/log/dhclient-script.log This is a left-over from debugging, I guess? > gout ADDR ip -j -4 addr show|jq -rM '.[] | select(.ifname == "__IFNAME__").addr_info[0].local' > hout HOST_ADDR ip -j -4 addr show|jq -rM '.[] | select(.ifname == "__HOST_IFNAME__").addr_info[0].local' > check [ "__ADDR__" = "__HOST_ADDR__" ] > @@ -47,10 +48,20 @@ gout SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^searc > hout HOST_SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/' > check [ "__SEARCH__" = "__HOST_SEARCH__" ] > > +test DHCP: Hostname > +gout NEW_HOST_NAME cat /tmp/new_host_name > +check [ "__NEW_HOST_NAME__" = "hostname1" ] > + > +test DHCP: Client FQDN > +gout NEW_FQDN_FQDN cat /tmp/new_fqdn_fqdn > +check [ "__NEW_FQDN_FQDN__" = "fqdn1.passt.top" ] > + > test DHCPv6: address > +guest rm /tmp/new_fqdn_fqdn > guest /sbin/dhclient -6 __IFNAME__ > # Wait for DAD to complete > guest while ip -j -6 addr show tentative | jq -e '.[].addr_info'; do sleep 0.1; done > +guest cat /var/log/dhclient-script.log > gout ADDR6 ip -j -6 addr show|jq -rM '[.[] | select(.ifname == "__IFNAME__").addr_info[] | select(.prefixlen == 128).local] | .[0]' > hout HOST_ADDR6 ip -j -6 addr show|jq -rM '[.[] | select(.ifname == "__HOST_IFNAME6__").addr_info[] | select(.scope == "global" and .deprecated != true).local] | .[0]' > check [ "__ADDR6__" = "__HOST_ADDR6__" ] > @@ -70,3 +81,7 @@ test DHCPv6: search list > gout SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/' > hout HOST_SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/' > check [ "__SEARCH6__" = "__HOST_SEARCH6__" ] > + > +test DHCPv6: Hostname > +gout NEW_FQDN_FQDN cat /tmp/new_fqdn_fqdn > +check [ "__NEW_FQDN_FQDN__" = "fqdn1.passt.top" ] > diff --git a/util.c b/util.c > index 55cae3f..b38aec7 100644 > --- a/util.c > +++ b/util.c > @@ -837,3 +837,24 @@ void raw_random(void *buf, size_t buflen) > if (random_read < buflen) > die("Unexpected EOF on random data source"); > } > +/** > + * encode_domain_name() - Encoding domain names according to RFC 3315 sec 8 "Encode", "name" (it's just one). I'd rather say RFC 1035, section 3.1, because that's where the encoding is actually specified. > + * @domain_name: Input domain name to encode > + * @len: Domain name length > + * @buf: Buffer to fill in with encoded domain name > + * > + * Returns: encoded domain name length "Return" (imperative mood, C is an imperative language). > + */ > + Excess newline. > +size_t encode_domain_name(const char* domain_name, size_t len, char* buf){ char *domain_name, etc. > + buf[0] = strcspn(domain_name, "."); If 'p' were to start from 'buf', instead, would we still need this? > + char* p = buf + 1; We don't mix declarations and code (C90 style, same as Linux kernel). > + for (size_t i = 0; i < len; i++) { ...and we don't declare variables in for loop initialisers. > + if (domain_name[i] == '.') { > + p[i] = strcspn(domain_name + i + 1, "."); This logic is taken from dhcpv6_dns_fill(). Wouldn't it make sense to use this new helper there instead of duplicating it? > + } else { > + p[i] = domain_name[i]; > + } No need for curly brackets, these statements are all single lines. > + } > + return len + 2; > +} > diff --git a/util.h b/util.h > index 90428c4..8001ebb 100644 > --- a/util.h > +++ b/util.h > @@ -305,4 +305,6 @@ static inline int wrap_accept4(int sockfd, struct sockaddr *addr, > #define accept4(s, addr, addrlen, flags) \ > wrap_accept4((s), (addr), (addrlen), (flags)) > > +size_t encode_domain_name(const char* domain_name, size_t len, char* buf); > + > #endif /* UTIL_H */ The rest looks good to me. Thanks, -- Stefano ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] dhcp, dhcpv6: Add hostname and client fqdn ops 2024-11-27 23:01 ` Stefano Brivio @ 2024-12-02 11:47 ` Enrique Llorente Pastora 0 siblings, 0 replies; 3+ messages in thread From: Enrique Llorente Pastora @ 2024-12-02 11:47 UTC (permalink / raw) To: Stefano Brivio; +Cc: passt-dev On Thu, Nov 28, 2024 at 12:01 AM Stefano Brivio <sbrivio@redhat.com> wrote: > > On Wed, 27 Nov 2024 16:33:39 +0100 > Enrique Llorente <ellorent@redhat.com> wrote: > > > Both DHCPv4 and DHCPv6 has the capability to pass the hostname to > > clients, the DHCPv4 uses option 12 (hostname) while the DHCPv6 uses option 39 > > (client fqdn), for some virt deployments like kubevirt is expected to > > have the VirtualMachine name as the guest hostname. > > > > This change add the following arguments: > > - -H --hostname NAME to configure the hostname DHCPv4 option(12) > > - --fqdn NAME to configure client fqdn option for both DHCPv4(81) and > > DHCPv6(39) > > > > Signed-off-by: Enrique Llorente <ellorent@redhat.com> > > This looks pretty good to me. A few comments below (some are details, > some are serious issues): > > > --- > > conf.c | 23 ++++++++++++++++++++--- > > dhcp.c | 15 ++++++++++++++- > > dhcpv6.c | 44 +++++++++++++++++++++++++++++++++++++++++++- > > passt.h | 4 ++++ > > test/lib/setup | 10 +++++----- > > test/passt.mbuto | 6 ++++-- > > test/passt/dhcp | 17 ++++++++++++++++- > > util.c | 21 +++++++++++++++++++++ > > util.h | 2 ++ > > 9 files changed, 129 insertions(+), 13 deletions(-) > > > > diff --git a/conf.c b/conf.c > > index 86566db..e9cf491 100644 > > --- a/conf.c > > +++ b/conf.c > > @@ -849,7 +849,9 @@ static void usage(const char *name, FILE *f, int status) > > " --freebind Bind to any address for forwarding\n" > > " --no-map-gw Don't map gateway address to host\n" > > " -4, --ipv4-only Enable IPv4 operation only\n" > > - " -6, --ipv6-only Enable IPv6 operation only\n"); > > + " -6, --ipv6-only Enable IPv6 operation only\n" > > + " -H, --hostname NAME Hostname to configure client with\n" > > + " --fqdn NAME The FQDN to configure client with\n"); > > A few comments here: > > - just "FQDN" instead of "The FQDN" makes it consistent with the > description for -H > Done > - I would move those a bit up, right after the DNS options (-S), > because that's where users are more likely to look for this stuff > Done > - now that you mention we're _configuring the client with_ those (I had > in mind that we were just sending those via DHCP / DHCPv6... even > though it's pretty much same thing), I wonder if we shouldn't also > use the NAME from --hostname for the sethostname() call we have in > pasta_spawn_cmd(). > > That is, if there's a "Hostname to configure the client with", it > would be totally unexpected that we ignore that when setting the > hostname with pasta > TODO: Test not working. > - these need matching updates in the man page > Done > > > > if (strstr(name, "pasta")) > > goto pasta_opts; > > @@ -1266,6 +1268,7 @@ void conf(struct ctx *c, int argc, char **argv) > > {"outbound", required_argument, NULL, 'o' }, > > {"dns", required_argument, NULL, 'D' }, > > {"search", required_argument, NULL, 'S' }, > > + {"hostname", required_argument, NULL, 'H' }, > > {"no-tcp", no_argument, &c->no_tcp, 1 }, > > {"no-udp", no_argument, &c->no_udp, 1 }, > > {"no-icmp", no_argument, &c->no_icmp, 1 }, > > @@ -1305,6 +1308,7 @@ void conf(struct ctx *c, int argc, char **argv) > > {"map-guest-addr", required_argument, NULL, 22 }, > > {"host-lo-to-ns-lo", no_argument, NULL, 23 }, > > {"dns-host", required_argument, NULL, 24 }, > > + {"fqdn", required_argument, NULL, 25 }, > > { 0 }, > > }; > > const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt"; > > @@ -1327,9 +1331,9 @@ void conf(struct ctx *c, int argc, char **argv) > > if (c->mode == MODE_PASTA) { > > c->no_dhcp_dns = c->no_dhcp_dns_search = 1; > > fwd_default = FWD_AUTO; > > - optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:"; > > + optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:H:46t:u:T:U:"; > > } else { > > - optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:"; > > + optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:H:461t:u:"; > > } > > > > c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET; > > @@ -1682,6 +1686,19 @@ void conf(struct ctx *c, int argc, char **argv) > > > > c->one_off = true; > > break; > > + case 'H': > > You moved 'H' in optstring to be just after 'S', now this should also > go after case 'S'. > Done > > + ret = snprintf(c->hostname.n, sizeof(c->hostname.n), > > + "%s", optarg); > > + if (ret <= 0 || ret >= (int)sizeof(c->hostname.n)) > > So, we're accepting 1024 characters here, but the DHCP option can only > be 255 characters long: > > $ unshare -rUn > # echo $$ > 369109 > > ...then: > > $ make clean; make valgrind > $ gdb --args ./pasta -f --config-net -H hfoiewhfiohewifoewhfiowhfoiewhfowehfiewohfowhfieofhow.lll.38428.jjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjj.kkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkk -p hostname.pcap 369109 > > ...back to the namespace: > > # dhclient > > and: > > DHCP: offer to discover > from b2:33:a6:a2:bf:8c > > Program received signal SIGSEGV, Segmentation fault. > __memcpy_avx_unaligned_erms () at ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:523 > 523 ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S: No such file or directory. > (gdb) > (gdb) bt > #0 __memcpy_avx_unaligned_erms () at ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:523 > #1 0x00005555555620dd in memcpy (__len=<optimized out>, __src=0x55555559d6ec <opts+5772>, __dest=0x55555662f33c <pkt_buf+828>) at /usr/include/x86_64-linux-gnu/bits/string_fortified.h:29 > #2 fill_one (offset=<synthetic pointer>, o=11, m=0x55555662f02a <pkt_buf+42>) at dhcp.c:138 > #3 fill (m=<optimized out>) at dhcp.c:175 > #4 dhcp (c=c@entry=0x7ffffff6ed60, p=p@entry=0x7ffffff6eb40) at dhcp.c:417 > #5 0x0000555555570c94 in tap4_handler (c=c@entry=0x7ffffff6ed60, in=in@entry=0x5555574a8ee0 <pool_tap4_storage>, now=now@entry=0x7ffffff6ec50) at tap.c:681 > #6 0x0000555555571971 in tap_handler (now=0x7ffffff6ec50, c=0x7ffffff6ed60) at tap.c:958 > #7 tap_pasta_input (now=0x7ffffff6ec50, c=0x7ffffff6ed60) at tap.c:1133 > #8 tap_handler_pasta (c=c@entry=0x7ffffff6ed60, events=events@entry=1, now=now@entry=0x7ffffff6ec50) at tap.c:1149 > #9 0x0000555555558eaa in main (argc=<optimized out>, argv=<optimized out>) at passt.c:318 > > ...I think c->hostname should also be 253 characters long, as you now > have for 'domain_name' in the DHCPv6 option. > Done > > + die("Invalid hostname: %s", optarg); > > + break; > > + case 25: > > This should be after all the numbered cases (now that would be case 27 > after case 26), for consistency. > Done > > + ret = snprintf(c->fqdn.n, sizeof(c->fqdn.n), > > + "%s", optarg); > > + if (ret <= 0 || ret >= (int)sizeof(c->fqdn.n)) > > And here, RFC 4702 mandates option concatenation. This can still be > 1024 characters (excluding the terminator), but it will crash in the > same way if you handle it like this. More on option concatenation below. > > By the way, after introducing those snprintf() calls, I added a > function, snprintf_check(), to take care of these checks in a more > concise way. > > I think both options are fine: you can use it because it's convenient, > or not use it for consistency (and one day I'll just do a mass > conversion of those). > TODO > > + die("Invalid fqdn: %s", optarg); > > + break; > > + > > case 't': > > case 'u': > > case 'T': > > diff --git a/dhcp.c b/dhcp.c > > index a06f143..e05bfc6 100644 > > --- a/dhcp.c > > +++ b/dhcp.c > > @@ -275,7 +275,7 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len) > > */ > > int dhcp(const struct ctx *c, const struct pool *p) > > { > > - size_t mlen, dlen, offset = 0, opt_len, opt_off = 0; > > + size_t mlen, dlen, offset = 0, opt_len, opt_off = 0, hostname_len, fqdn_len = 0; > > These shouldn't exceed 80 columns (there's no reason here). You can add > them on a separate line, because they're logically somewhat distinct. > > Why is fqdn_len initialised? There's no need to, it's always set anyway. > Done > > char macstr[ETH_ADDRSTRLEN]; > > const struct ethhdr *eh; > > const struct iphdr *iph; > > @@ -375,6 +375,19 @@ int dhcp(const struct ctx *c, const struct pool *p) > > opts[6].slen += sizeof(uint32_t); > > } > > > > + hostname_len = strlen(c->hostname.n); > > + if ( hostname_len > 0 ) { > > Coding style, for consistency with the rest: > > if (hostname_len > 0) { > Done > > + opts[12].slen = hostname_len; > > + memcpy(opts[12].s, &c->hostname.n, hostname_len); > > + } > > + > > + fqdn_len = strlen(c->fqdn.n); > > + if ( fqdn_len > 0 ) { > > Same here. > Done > > + opts[81].slen = fqdn_len+3; > > Spaces around '+', and missing check on length. > Done > If the option is longer than 255 bytes (excluding type and length > bytes), RFC 4702 mandates that we use option concatenation for it (as > specified by RFC 3396). > > That is, fill() should be ready to concatenate, in the response, > multiple parts of the same option (option code is repeated, first > option encoding has 255 bytes as length, the next one has the remaining > length). > > Option overloading (using 'sname' and 'file') is not mandatory, though, > so we can set a 308-byte upper bound, which means we can change struct > opts to have slightly bigger buffers (say, 308 bytes) for the options > we want to send ('s'). > > An alternative could be to limit what can be passed to --fqdn to 252 > bytes (because of "Flags", "RCODE1", and "RCODE2"). At that point, even > if support for option concatenation is mandatory, we know that we'll > never need it, so we can avoid implementing it. The whole thing would > be slightly buggy, though, because an FQDN could be, for sure, 253 > bytes. > TODO > > + memcpy(opts[81].s+3, &c->fqdn.n, fqdn_len); > > Just like for option 39 in DHCPv6, we need to use the "canonical" (RFC > 1035) encoding of the domain name. > Done > Unlike DHCPv6 option 39, we need set the 'E' bit in "Flags" to 1. > > > + } > > + > > + > Done > Excess newline. > Done > > if (!c->no_dhcp_dns_search) > > opt_set_dns_search(c, sizeof(m->o)); > > > > diff --git a/dhcpv6.c b/dhcpv6.c > > index 0523bba..88c0978 100644 > > --- a/dhcpv6.c > > +++ b/dhcpv6.c > > @@ -48,6 +48,7 @@ struct opt_hdr { > > # define STATUS_NOTONLINK htons_constant(4) > > # define OPT_DNS_SERVERS htons_constant(23) > > # define OPT_DNS_SEARCH htons_constant(24) > > +# define OPT_CLIENT_FQDN htons_constant(39) > > This should be aligned to the other values with a tab, not spaces. > Done > > #define STR_NOTONLINK "Prefix not appropriate for link." > > > > uint16_t l; > > @@ -163,6 +164,18 @@ struct opt_dns_search { > > char list[MAXDNSRCH * NS_MAXDNAME]; > > } __attribute__((packed)); > > > > +/** > > + * struct opt_client_fqdn - Client FQDN option (RFC 4704) > > + * @hdr: Option header > > + * @flags: Flags as stated at RFC 4704 (always zero for us) > > "described by RFC 4704" > Done > > + * @fqdn: Client fqdn > > ...now you called this called 'domain_name'. > Done > > + */ > > +struct opt_client_fqdn{ > > + struct opt_hdr hdr; > > + uint8_t flags; > > + char domain_name[253 /*RFC 1035*/ ]; > > For consistency with coding style elsewhere: /* RFC 1035 */ > Done > > +} __attribute__((packed)); > > + > > /** > > * struct msg_hdr - DHCPv6 client/server message header > > * @type: DHCP message type > > @@ -193,6 +206,7 @@ struct msg_hdr { > > * @client_id: Client Identifier, variable length > > * @dns_servers: DNS Recursive Name Server, here just for storage size > > * @dns_search: Domain Search List, here just for storage size > > + * @client_fqdn: Client FQDN, variable length > > */ > > static struct resp_t { > > struct msg_hdr hdr; > > @@ -203,10 +217,10 @@ static struct resp_t { > > struct opt_client_id client_id; > > struct opt_dns_servers dns_servers; > > struct opt_dns_search dns_search; > > + struct opt_client_fqdn client_fqdn; > > } __attribute__((__packed__)) resp = { > > { 0 }, > > SERVER_ID, > > - > > Unrelated change. > Done > > { { OPT_IA_NA, OPT_SIZE_CONV(sizeof(struct opt_ia_na) + > > sizeof(struct opt_ia_addr) - > > sizeof(struct opt_hdr)) }, > > @@ -228,6 +242,10 @@ static struct resp_t { > > { { OPT_DNS_SEARCH, 0, }, > > { 0 }, > > }, > > + > > + { { OPT_CLIENT_FQDN, 0, }, > > + 0, { 0 }, > > + }, > > }; > > > > static const struct opt_status_code sc_not_on_link = { > > @@ -411,6 +429,29 @@ search: > > return offset; > > } > > > > +/** > > + * dhcpv6_client_fqdn_fill() - Fill in client FQDN option > > + * @c: Execution context > > + * @buf: Response message buffer where options will be appended > > + * @offset: Offset in message buffer for new options > > + * > > + * Return: updated length of response message buffer. > > + */ > > +static size_t dhcpv6_client_fqdn_fill(const struct ctx *c, char *buf, int offset) > > +{ > > + uint16_t fqdn_len = strlen(c->fqdn.n); > > We usually add an empty line between declarations and code, for > readability. > Done > > + if (fqdn_len > 0) { > > An early return might be more convenient. If not, excess whitespace > between ) and {. > Done > > + struct opt_client_fqdn *o = (struct opt_client_fqdn *)(buf + offset); > > + size_t encoded_fqdn_len = encode_domain_name(c->fqdn.n, fqdn_len, o->domain_name); > > These need to be wrapped to 80 columns. > Done > > + size_t opt_len = encoded_fqdn_len + 1; > > Same here, empty line would be nice. > Done > > + o->hdr.t = OPT_CLIENT_FQDN; > > + o->hdr.l = htons(opt_len); > > + o->flags = 0x00; > > + offset += sizeof(struct opt_hdr) + opt_len; > > + } > > + return offset; > > +} > > + > > /** > > * dhcpv6() - Check if this is a DHCPv6 message, reply as needed > > * @c: Execution context > > @@ -544,6 +585,7 @@ int dhcpv6(struct ctx *c, const struct pool *p, > > n = offsetof(struct resp_t, client_id) + > > sizeof(struct opt_hdr) + ntohs(client_id->l); > > n = dhcpv6_dns_fill(c, (char *)&resp, n); > > + n = dhcpv6_client_fqdn_fill(c, (char*)&resp, n); > > > > resp.hdr.xid = mh->xid; > > > > diff --git a/passt.h b/passt.h > > index 72c7f72..566d21d 100644 > > --- a/passt.h > > +++ b/passt.h > > @@ -205,6 +205,8 @@ struct ip6_ctx { > > * @ifi4: Index of template interface for IPv4, 0 if IPv4 disabled > > * @ip: IPv4 configuration > > * @dns_search: DNS search list > > + * @hostname: Client hostname > > + * @fqdn: Client FQDN > > I would call it "Guest" (or specify we send those to it) because > otherwise it's not clear from just reading this that those are not the > options the client sends. > > "Client hostname" seems to imply we store there the option 12 sent by a > DHCP client. > Done > > * @ifi6: Index of template interface for IPv6, 0 if IPv6 disabled > > * @ip6: IPv6 configuration > > * @pasta_ifn: Name of namespace interface for pasta > > @@ -262,6 +264,8 @@ struct ctx { > > struct ip4_ctx ip4; > > > > struct fqdn dns_search[MAXDNSRCH]; > > + struct fqdn hostname; > > + struct fqdn fqdn; > > > > unsigned int ifi6; > > struct ip6_ctx ip6; > > diff --git a/test/lib/setup b/test/lib/setup > > index 5338393..a3fe578 100755 > > --- a/test/lib/setup > > +++ b/test/lib/setup > > @@ -49,7 +49,7 @@ setup_passt() { > > > > context_run passt "make clean" > > context_run passt "make valgrind" > > - context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -P ${STATESETUP}/passt.pid" > > + context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -H hostname1 --fqdn fqdn1.passt.top -P ${STATESETUP}/passt.pid" > > Very minor: I'll actually run those tests on passt.top, so maybe we > shouldn't use that as domain, because if we actually decide to change > these tests and do something with it, the behaviour might depend on > where tests run. I'd rather use passt.test: > > https://en.wikipedia.org/wiki/.test > Done > > > > # pidfile isn't created until passt is listening > > wait_for [ -f "${STATESETUP}/passt.pid" ] > > @@ -146,11 +146,11 @@ setup_passt_in_ns() { > > if [ ${VALGRIND} -eq 1 ]; then > > context_run passt "make clean" > > context_run passt "make valgrind" > > - context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}" > > + context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H hostname1 --fqdn fqdn1.passt.top -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}" > > else > > context_run passt "make clean" > > context_run passt "make" > > - context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}" > > + context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H hostname1 --fqdn fqdn1.passt.top -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}" > > fi > > wait_for [ -f "${STATESETUP}/passt.pid" ] > > > > @@ -215,7 +215,7 @@ setup_two_guests() { > > [ ${DEBUG} -eq 1 ] && __opts="${__opts} -d" > > [ ${TRACE} -eq 1 ] && __opts="${__opts} --trace" > > > > - context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} -t 10001 -u 10001" > > + context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} --fqdn fqdn1.passt.top -H hostname1 -t 10001 -u 10001" > > wait_for [ -f "${STATESETUP}/passt_1.pid" ] > > > > __opts= > > @@ -223,7 +223,7 @@ setup_two_guests() { > > [ ${DEBUG} -eq 1 ] && __opts="${__opts} -d" > > [ ${TRACE} -eq 1 ] && __opts="${__opts} --trace" > > > > - context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} -t 10004 -u 10004" > > + context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} --hostname hostname2 --fqdn fqdn2 -t 10004 -u 10004" > > wait_for [ -f "${STATESETUP}/passt_2.pid" ] > > > > GUEST_1_CID=94557 > > diff --git a/test/passt.mbuto b/test/passt.mbuto > > index 138d365..1e07693 100755 > > --- a/test/passt.mbuto > > +++ b/test/passt.mbuto > > @@ -13,7 +13,7 @@ > > PROGS="${PROGS:-ash,dash,bash ip mount ls insmod mkdir ln cat chmod lsmod > > modprobe find grep mknod mv rm umount jq iperf3 dhclient hostname > > sed tr chown sipcalc cut socat dd strace ping tail killall sleep sysctl > > - nproc tcp_rr tcp_crr udp_rr which tee seq bc sshd ssh-keygen cmp}" > > + nproc tcp_rr tcp_crr udp_rr which tee seq bc sshd ssh-keygen cmp env}" > > > > # OpenSSH 9.8 introduced split binaries, with sshd being the daemon, and > > # sshd-session the per-session program. We need the latter as well, and the path > > @@ -41,6 +41,7 @@ FIXUP="${FIXUP}"' > > #!/bin/sh > > LOG=/var/log/dhclient-script.log > > echo \${reason} \${interface} >> \$LOG > > +env >> \$LOG > > set >> \$LOG > > > > [ -n "\${new_interface_mtu}" ] && ip link set dev \${interface} mtu \${new_interface_mtu} > > @@ -54,7 +55,8 @@ set >> \$LOG > > [ -n "\${new_ip6_address}" ] && ip addr add \${new_ip6_address}/\${new_ip6_prefixlen} dev \${interface} > > [ -n "\${new_dhcp6_name_servers}" ] && for d in \${new_dhcp6_name_servers}; do echo "nameserver \${d}%\${interface}" >> /etc/resolv.conf; done > > [ -n "\${new_dhcp6_domain_search}" ] && (printf "search"; for d in \${new_dhcp6_domain_search}; do printf " %s" "\${d}"; done; printf "\n") >> /etc/resolv.conf > > -[ -n "\${new_host_name}" ] && hostname "\${new_host_name}" > > +[ -n "\${new_host_name}" ] && echo "\${new_host_name}" > /tmp/new_host_name > > +[ -n "\${new_fqdn_fqdn}" ] && echo "\${new_fqdn_fqdn}" > /tmp/new_fqdn_fqdn > > exit 0 > > EOF > > chmod 755 /sbin/dhclient-script > > diff --git a/test/passt/dhcp b/test/passt/dhcp > > index 9925ab9..f947fd1 100644 > > --- a/test/passt/dhcp > > +++ b/test/passt/dhcp > > @@ -11,7 +11,7 @@ > > # Copyright (c) 2021 Red Hat GmbH > > # Author: Stefano Brivio <sbrivio@redhat.com> > > > > -gtools ip jq dhclient sed tr > > +gtools ip jq dhclient sed tr hostname > > htools ip jq sed tr head > > > > test Interface name > > @@ -22,6 +22,7 @@ check [ -n "__IFNAME__" ] > > > > test DHCP: address > > guest /sbin/dhclient -4 __IFNAME__ > > +guest cat /var/log/dhclient-script.log > > This is a left-over from debugging, I guess? > Done > > gout ADDR ip -j -4 addr show|jq -rM '.[] | select(.ifname == "__IFNAME__").addr_info[0].local' > > hout HOST_ADDR ip -j -4 addr show|jq -rM '.[] | select(.ifname == "__HOST_IFNAME__").addr_info[0].local' > > check [ "__ADDR__" = "__HOST_ADDR__" ] > > @@ -47,10 +48,20 @@ gout SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^searc > > hout HOST_SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/' > > check [ "__SEARCH__" = "__HOST_SEARCH__" ] > > > > +test DHCP: Hostname > > +gout NEW_HOST_NAME cat /tmp/new_host_name > > +check [ "__NEW_HOST_NAME__" = "hostname1" ] > > + > > +test DHCP: Client FQDN > > +gout NEW_FQDN_FQDN cat /tmp/new_fqdn_fqdn > > +check [ "__NEW_FQDN_FQDN__" = "fqdn1.passt.top" ] > > + > > test DHCPv6: address > > +guest rm /tmp/new_fqdn_fqdn > > guest /sbin/dhclient -6 __IFNAME__ > > # Wait for DAD to complete > > guest while ip -j -6 addr show tentative | jq -e '.[].addr_info'; do sleep 0.1; done > > +guest cat /var/log/dhclient-script.log > > gout ADDR6 ip -j -6 addr show|jq -rM '[.[] | select(.ifname == "__IFNAME__").addr_info[] | select(.prefixlen == 128).local] | .[0]' > > hout HOST_ADDR6 ip -j -6 addr show|jq -rM '[.[] | select(.ifname == "__HOST_IFNAME6__").addr_info[] | select(.scope == "global" and .deprecated != true).local] | .[0]' > > check [ "__ADDR6__" = "__HOST_ADDR6__" ] > > @@ -70,3 +81,7 @@ test DHCPv6: search list > > gout SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/' > > hout HOST_SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/' > > check [ "__SEARCH6__" = "__HOST_SEARCH6__" ] > > + > > +test DHCPv6: Hostname > > +gout NEW_FQDN_FQDN cat /tmp/new_fqdn_fqdn > > +check [ "__NEW_FQDN_FQDN__" = "fqdn1.passt.top" ] > > diff --git a/util.c b/util.c > > index 55cae3f..b38aec7 100644 > > --- a/util.c > > +++ b/util.c > > @@ -837,3 +837,24 @@ void raw_random(void *buf, size_t buflen) > > if (random_read < buflen) > > die("Unexpected EOF on random data source"); > > } > > +/** > > + * encode_domain_name() - Encoding domain names according to RFC 3315 sec 8 > > "Encode", "name" (it's just one). I'd rather say RFC 1035, section 3.1, > because that's where the encoding is actually specified. > Done > > + * @domain_name: Input domain name to encode > > + * @len: Domain name length > > + * @buf: Buffer to fill in with encoded domain name > > + * > > + * Returns: encoded domain name length > > "Return" (imperative mood, C is an imperative language). > > > + */ > > + > > Excess newline. > Done > > +size_t encode_domain_name(const char* domain_name, size_t len, char* buf){ > > char *domain_name, etc. > Done > > + buf[0] = strcspn(domain_name, "."); > > If 'p' were to start from 'buf', instead, would we still need this? > If we want to have all the strcspn under the for loop then we will need to do the following since we don't have the skip the dot at the beginning so no "+1". if (i == 0) { p[i] = strcspn(domain_name, "."); } else if (domain_name[i] == '.') { p[i] = strcspn(domain_name + i + 1, "."); } else { p[i] = domain_name[i]; } Don't know if it's really worth it. > > + char* p = buf + 1; > > We don't mix declarations and code (C90 style, same as Linux kernel). > Done > > + for (size_t i = 0; i < len; i++) { > > ...and we don't declare variables in for loop initialisers. > Done > > + if (domain_name[i] == '.') { > > + p[i] = strcspn(domain_name + i + 1, "."); > > This logic is taken from dhcpv6_dns_fill(). Wouldn't it make sense to > use this new helper there instead of duplicating it? > Done > > + } else { > > + p[i] = domain_name[i]; > > + } > > No need for curly brackets, these statements are all single lines. > > > + } > > + return len + 2; > > +} > > diff --git a/util.h b/util.h > > index 90428c4..8001ebb 100644 > > --- a/util.h > > +++ b/util.h > > @@ -305,4 +305,6 @@ static inline int wrap_accept4(int sockfd, struct sockaddr *addr, > > #define accept4(s, addr, addrlen, flags) \ > > wrap_accept4((s), (addr), (addrlen), (flags)) > > > > +size_t encode_domain_name(const char* domain_name, size_t len, char* buf); > > + > > #endif /* UTIL_H */ > > The rest looks good to me. Thanks, > > -- > Stefano > -- Quique Llorente CNV networking Senior Software Engineer Red Hat EMEA ellorent@redhat.com @RedHat Red Hat Red Hat ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-12-02 11:47 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-11-27 15:33 [PATCH v3] dhcp, dhcpv6: Add hostname and client fqdn ops Enrique Llorente 2024-11-27 23:01 ` Stefano Brivio 2024-12-02 11:47 ` Enrique Llorente Pastora
Code repositories for project(s) associated with this public inbox https://passt.top/passt This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for IMAP folder(s).