From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=Rk0R4PI3; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTPS id 52D605A004E for ; Wed, 08 Jan 2025 01:02:20 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1736294537; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=HS3bkejiHa5X499YZvpxEZMhn3fBZy0aO0iA4TGSQ+I=; b=Rk0R4PI3ykOa6EaC44G22VnKZ4mqLVseZTJEr+t6RHLfaU34TUzK9n4Gxd6zih/EBGlEDC AdgpjWz3wwk3h+pajxoa0RHGy2/4pjf/P6+qRQNHfhQAcKSv9YFGYrVmXt0RxxhVnNZFER XVmstiZfON0lmIRpWlIQPKnFSEQMxvU= Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-248-Xvz5CWgQPhap9jSQFHp6hA-1; Tue, 07 Jan 2025 19:02:14 -0500 X-MC-Unique: Xvz5CWgQPhap9jSQFHp6hA-1 X-Mimecast-MFC-AGG-ID: Xvz5CWgQPhap9jSQFHp6hA Received: by mail-wr1-f69.google.com with SMTP id ffacd0b85a97d-385e2579507so7078051f8f.1 for ; Tue, 07 Jan 2025 16:02:14 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736294532; x=1736899332; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=HS3bkejiHa5X499YZvpxEZMhn3fBZy0aO0iA4TGSQ+I=; b=M7wy75KOaxN/+pGAJGCWT2PBN+lB/4o906KPA87CApkOAlX8Sa/6+QRuGNpDu5DpX6 uUiJxc1a/mLnRY7iZ3YXUi146FN3XKDTNabeTFNza9NejFj44qAo4cfrcz+SZkwxo50q zahR02z/g8OL9fhaG9uAwq289sd0A7z5rB6lIsWcLex/aWm2uXr5vqChA3cg/S0YxrWx nKYgaiffs2w5Ke2Lr4bStfK1LnIdBJ7pRJN3x+pId4MKw8pMQi1V8T8IuLvNJbVI3dOL TyRImbxOebeOH/hy9ZuTFaMTKodlK1qNMc696AT0azvzmGnvn0/Ua+GxaEASZaN/Hp6q 9iww== X-Gm-Message-State: AOJu0YyJosGwPep3zMGMw6tj3m70WCDnJwHSDBHmxbRZgzL6A6EPGgbe 8LqEhbTQLtd9BNBVSvOEkZ9PC/nhaa/GRWRX3z/5TUk89TLnV3LNAKE4/DITMCf2FfAVIgQSdqP Si0RiwgMwNOBY26+hNvz2l/tew3qDiXYaVX6Nn8Uj+TXNAGqhwZu4OdAQKfb/Ln5f33KiQlRS6y 28TbfZ7m8sRe5mZoKgeF8C8WV6qIfValkn X-Gm-Gg: ASbGncsp5Ngj5rGc84JB8/zBgIOzlVGrhpZWyMJ4UlwrfiD3o80naVYSTDNTwbLan1c FdNMzczwq2NNWmjQRH9+IbTzOoQemKqSNUsiYVLQTNsVhdtIq901V63CX3L/nQn5kcmNw8DPUGB FjcU91k2G+1/aho+S1bOtkMUaEdPtGKeoWghpJlfUTmysvxjo2cmtGyHDgjgu9BK2nEkyghE0kB yfO9tKBOdxcXFV/kV/AkVk8H9lADpp4jFjrD8YppguP1ryygo4p/Pxdrr+iC+9S+5W5WaLKCMEP +W8HtWZ0NQ== X-Received: by 2002:a05:6000:2a3:b0:386:4a0d:bb21 with SMTP id ffacd0b85a97d-38a8730a11emr379095f8f.22.1736294531384; Tue, 07 Jan 2025 16:02:11 -0800 (PST) X-Google-Smtp-Source: AGHT+IFo8es2OHKJFbcKWt48zwXt7Bd2i4cd7NF3g8Aj2/08uTbWbntE9VB3x4u1nsXduTuYp6dl6Q== X-Received: by 2002:a05:6000:2a3:b0:386:4a0d:bb21 with SMTP id ffacd0b85a97d-38a8730a11emr379055f8f.22.1736294530479; Tue, 07 Jan 2025 16:02:10 -0800 (PST) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-38a1c8ace0esm50772937f8f.106.2025.01.07.16.02.08 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Jan 2025 16:02:09 -0800 (PST) Date: Wed, 8 Jan 2025 01:02:06 +0100 From: Stefano Brivio To: Enrique Llorente Subject: Re: [PATCH v6] dhcp, dhcpv6: Add hostname and client fqdn ops Message-ID: <20250108010206.2133217f@elisabeth> In-Reply-To: <20250107093617.127934-1-ellorent@redhat.com> References: <20250107093617.127934-1-ellorent@redhat.com> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: LHUirO5LXNIuMh7hYbui2WaupNmIdJwO-AtVrceDTxk_1736294533 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: 4LDG6TFCBEZ6UBTJ5XQ4KWQMZDA42DAR X-Message-ID-Hash: 4LDG6TFCBEZ6UBTJ5XQ4KWQMZDA42DAR X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Functionally speaking, this looks (almost) correct to me now, some comments below: On Tue, 7 Jan 2025 10:36:17 +0100 Enrique Llorente 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 > --- > conf.c | 20 ++++++++++-- > dhcp.c | 63 ++++++++++++++++++++++++++++++++----- > dhcpv6.c | 82 +++++++++++++++++++++++++++++++++++++++--------- > passt.1 | 11 +++++++ > passt.h | 5 +++ > pasta.c | 17 +++++++--- > test/lib/setup | 10 +++--- > test/passt.mbuto | 6 ++-- > test/passt/dhcp | 15 ++++++++- > util.c | 27 ++++++++++++++++ > util.h | 6 ++++ > 11 files changed, 225 insertions(+), 37 deletions(-) > > diff --git a/conf.c b/conf.c > index df2b016..fdbae38 100644 > --- a/conf.c > +++ b/conf.c > @@ -854,7 +854,9 @@ static void usage(const char *name, FILE *f, int status) > FPRINTF(f, " default: use addresses from /etc/resolv.conf\n"); > FPRINTF(f, > " -S, --search LIST Space-separated list, search domains\n" > - " a single, empty option disables the DNS search list\n"); > + " a single, empty option disables the DNS search list\n" > + " -H, --hostname NAME Hostname to configure client with\n" > + " --fqdn NAME FQDN to configure client with\n"); > if (strstr(name, "pasta")) > FPRINTF(f, " default: don't use any search list\n"); > else > @@ -1313,6 +1315,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 }, > @@ -1357,6 +1360,7 @@ void conf(struct ctx *c, int argc, char **argv) > /* vhost-user backend program convention */ > {"print-capabilities", no_argument, NULL, 26 }, > {"socket-path", required_argument, NULL, 's' }, > + {"fqdn", required_argument, NULL, 27 }, > { 0 }, > }; > const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt"; > @@ -1379,9 +1383,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; > @@ -1558,6 +1562,11 @@ void conf(struct ctx *c, int argc, char **argv) > case 26: > vu_print_capabilities(); > break; > + case 27: > + if (snprintf_check(c->fqdn, PASST_MAXDNAME, > + "%s", optarg)) ...again, this should be: if (snprintf_check(c->fqdn, PASST_MAXDNAME, "%s", optarg)) like you did for case 'H' below. Are you sure that your editor is set to show 8-character wide tabs, and that it actually displays spaces and tabs separately? > + die("Invalid FQDN: %s", optarg); > + break; > case 'd': > c->debug = 1; > c->quiet = 0; > @@ -1727,6 +1736,11 @@ void conf(struct ctx *c, int argc, char **argv) > > die("Cannot use DNS search domain %s", optarg); > break; > + case 'H': > + if (snprintf_check(c->hostname, PASST_MAXDNAME, > + "%s", optarg)) > + die("Invalid hostname: %s", optarg); > + break; > case '4': > v4_only = true; > v6_only = false; > diff --git a/dhcp.c b/dhcp.c > index d8515aa..51e6eb3 100644 > --- a/dhcp.c > +++ b/dhcp.c > @@ -63,6 +63,11 @@ static struct opt opts[255]; > > #define OPT_MIN 60 /* RFC 951 */ > > +/* Total option size (excluding end option) is 576 (RFC 2131), minus > + * offset of options (268), minus end option and its length (2). > + */ > +#define OPT_MAX 306 > + > /** > * dhcp_init() - Initialise DHCP options > */ > @@ -122,7 +127,7 @@ struct msg { > uint8_t sname[64]; > uint8_t file[128]; > uint32_t magic; > - uint8_t o[308]; > + uint8_t o[OPT_MAX + 2/* End option and its length */]; Spaces around /* ... */ wouldn't be bad for readability. > } __attribute__((__packed__)); > > /** > @@ -130,15 +135,35 @@ struct msg { > * @m: Message to fill > * @o: Option number > * @offset: Current offset within options field, updated on insertion > + * > + * Return: offset for the next option field > */ > -static void fill_one(struct msg *m, int o, int *offset) > +static int fill_one(struct msg *m, int o, int *offset) > { > + size_t slen; > + > + slen = opts[o].slen; This can be initialised at declaration time. > + > + /* If we can't write minimal content, then just skip */ How do you define "minimal content"? I'd rather be specific: "if we can't write at least one byte". But with this version, things change, so this check should change as well, right? That is, we should actually use slen, instead of checking if we can write a single byte, because we're not truncating options anymore. That is, I guess: if (*offset + 1 /* length of option */ + slen > OPT_MAX) ... ? > + if (*offset + 1 /* length */ + !!slen > OPT_MAX) > + return OPT_MAX; > + > m->o[*offset] = o; > - m->o[*offset + 1] = opts[o].slen; > - memcpy(&m->o[*offset + 2], opts[o].s, opts[o].slen); > + m->o[*offset + 1] = slen; > + > + /* Move to option */ > + *offset += 2; > + > + /* Skip if it do not fit */ "if it doesn't"? > + if (*offset + slen > OPT_MAX) { This check could be made redundant now, by replacing it with the check above. > + return OPT_MAX; > + } > + > + memcpy(&m->o[*offset], opts[o].s, slen); > > opts[o].sent = 1; > - *offset += 2 + opts[o].slen; > + *offset += slen; > + return *offset; > } > > /** > @@ -150,6 +175,7 @@ static void fill_one(struct msg *m, int o, int *offset) > static int fill(struct msg *m) > { > int i, o, offset = 0; > + bool truncated; Extra whitespace at the end of the line ('git show' tells you). > > m->op = BOOTREPLY; > m->secs = 0; > @@ -170,9 +196,16 @@ static int fill(struct msg *m) > fill_one(m, o, &offset); > } > > + truncated = false; You could initialise this at declaration time and make the function a bit easier to follow ("we start with truncated == false, then..."). > for (o = 0; o < 255; o++) { > - if (opts[o].slen != -1 && !opts[o].sent) > - fill_one(m, o, &offset); > + if (opts[o].slen != -1 && !opts[o].sent) { > + if (truncated) { > + debug("DHCP: truncating after option %i", o); > + break; > + } > + if (fill_one(m, o, &offset) == OPT_MAX) > + truncated = false; ...you mean = true? Otherwise it's always false. By the way, one thing occurred to me (sorry for not noticing earlier): "after option x" doesn't say much. The user doesn't necessarily know in which order we have options, so perhaps we should simply print all the option numbers we can't add, instead. Instead of creating yourself a problem with special return values that overlap with non-error return values (OPT_MAX), what if you just return -1 from fill_one() if there's no space, and continue the loop here (instead of breaking it on error), mentioning that you're omitting a given option when you get -1? Please, as usual, if you do so: update function comments. > + } > } > > m->o[offset++] = 255; > @@ -398,6 +431,22 @@ int dhcp(const struct ctx *c, const struct pool *p) > if (!opts[6].slen) > opts[6].slen = -1; > > + opt_len = strlen(c->hostname); > + if (opt_len > 0) { > + opts[12].slen = opt_len; > + memcpy(opts[12].s, &c->hostname, opt_len); > + } > + > + if (c->fqdn[0]) { > + size_t encoded_len; > + opts[81].s[0] = 0x4; /* flags (E) */ > + opts[81].s[1] = 0xff; /* RCODE1 */ > + opts[81].s[2] = 0xff; /* RCODE2 */ > + encoded_len = encode_domain_name((char *) opts[81].s + 3, No need for extra whitespace between (char *) and opts[81].s. > + c->fqdn); > + opts[81].slen = encoded_len + 3; > + } > + > if (!c->no_dhcp_dns_search) > opt_set_dns_search(c, sizeof(m->o)); > > diff --git a/dhcpv6.c b/dhcpv6.c > index 0523bba..70a610a 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; > @@ -58,6 +59,9 @@ struct opt_hdr { > sizeof(struct opt_hdr)) > #define OPT_VSIZE(x) (sizeof(struct opt_##x) - \ > sizeof(struct opt_hdr)) > +#define OPT_MAX_SIZE IPV6_MIN_MTU - (sizeof(struct ipv6hdr) + \ > + sizeof(struct udphdr) + \ > + sizeof(struct msg_hdr)) > > /** > * struct opt_client_id - DHCPv6 Client Identifier option > @@ -163,6 +167,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 described by RFC 4704 (always zero for us) Not anymore (always zero for us). > + * @domain_name: Client FQDN > + */ > +struct opt_client_fqdn { > + struct opt_hdr hdr; > + uint8_t flags; > + char domain_name[PASST_MAXDNAME]; > +} __attribute__((packed)); > + > /** > * struct msg_hdr - DHCPv6 client/server message header > * @type: DHCP message type > @@ -193,6 +209,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,6 +220,7 @@ 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, > @@ -228,6 +246,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 = { > @@ -346,7 +368,6 @@ static size_t dhcpv6_dns_fill(const struct ctx *c, char *buf, int offset) > { > struct opt_dns_servers *srv = NULL; > struct opt_dns_search *srch = NULL; > - char *p = NULL; > int i; > > if (c->no_dhcp_dns) > @@ -373,6 +394,7 @@ search: > return offset; > > for (i = 0; *c->dns_search[i].n; i++) { > + size_t encoded_name_len = 0; No need to initialise this to zero (one wonders why: do you ever use this without assigning it?). > size_t name_len = strlen(c->dns_search[i].n); > > /* We already append separators, don't duplicate if present */ > @@ -388,29 +410,58 @@ search: > offset += sizeof(struct opt_hdr); > srch->hdr.t = OPT_DNS_SEARCH; > srch->hdr.l = 0; > - p = srch->list; > } > > - *p = '.'; > - p = stpncpy(p + 1, c->dns_search[i].n, name_len); > - p++; > - srch->hdr.l += name_len + 2; > - offset += name_len + 2; > + encoded_name_len = encode_domain_name(srch->list, > + c->dns_search[i].n); c->dns_search[i].n should be aligned under the other argument, srch->list, because they are at the same level: they're both arguments of the same function. > + srch->hdr.l += encoded_name_len; > + offset += encoded_name_len; > } > > - if (srch) { > - for (i = 0; i < srch->hdr.l; i++) { > - if (srch->list[i] == '.') { > - srch->list[i] = strcspn(srch->list + i + 1, > - "."); > - } > - } > + if (srch) > srch->hdr.l = htons(srch->hdr.l); > - } > > return offset; > } > > +/** > + * dhcpv6_client_fqdn_fill() - Fill in client FQDN option > + * @c: Execution context This is indented deeper than the others for no apparent reason. > + * @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 pool *p, const struct ctx *c, char *buf, This exceeds 80 columns. > + int offset) This should be aligned under the other arguments, after '('. > +{ > + ssize_t opt_len; > + struct opt_client_fqdn *o; > + struct opt_hdr *req_opt; These should go from longest to shortest. > + > + if ((ssize_t)(OPT_MAX_SIZE - (offset + sizeof(struct opt_hdr) + 2 + strlen(c->fqdn))) <= 0) { This exceeds 80 columns for no particular reason. No need for curly brackets. This duplicates the calculation performed by encode_domain_name(), which is simply strlen(c->fqdn) + 2. And if they don't match, you might overflow the space available here. On the other hand, you can't call encode_domain_name() because you don't know if there's enough space. What about returning nothing from encode_domain_name(), and simply use strlen(x) in the caller, instead of duplicating things? An intermediate variable would be nice to use (that's opt_len), so that that check becomes a bit more readable. You could compare things to OPT_MAX_SIZE directly, instead of subtracting and comparing to zero. That is, you are actually checking if "x fits into size y", not if "subtracting x from y yields zero or less". > + debug("DHCPv6: client fqdn option do not fit, skipping"); FQDN does > + return offset; > + } > + > + o = (struct opt_client_fqdn *)(buf + offset); > + opt_len = encode_domain_name(o->domain_name, c->fqdn); > + > + ++opt_len; Wait why? If it's because of 'flags', maybe you could add this below after you set o->flags (opt_len++ for consistency, instead of ++opt_len), so that it's clear what it refers to without the need for extra comments. That would also mean you should move the assignments of the option length, of course. > + > + o->hdr.t = OPT_CLIENT_FQDN; > + o->hdr.l = htons(opt_len); > + > + req_opt = dhcpv6_opt(p, &(size_t){ 0 }, OPT_CLIENT_FQDN); > + if (req_opt && ((struct opt_client_fqdn *)req_opt)->flags & 0x01/* S flag */) { You don't use req_opt for anything else, so it could have right away the right type, if you cast on assignment (not on usage) as needed. This exceeds 80 columns. No need for curly brackets. Leave a whitespace between 0x01 and start of comment, as usual. Look at the line below: > + o->flags = 0x02 /* O flag */; ...would you write 0x02/* O flag */? Probably not. > + } else { > + o->flags = 0x00; > + } > + > + return offset + sizeof(struct opt_hdr) + opt_len; > +} > + > /** > * dhcpv6() - Check if this is a DHCPv6 message, reply as needed > * @c: Execution context > @@ -544,6 +595,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(p, c, (char *)&resp, n); > > resp.hdr.xid = mh->xid; > > diff --git a/passt.1 b/passt.1 > index d9cd33e..54a49ac 100644 > --- a/passt.1 > +++ b/passt.1 > @@ -401,6 +401,17 @@ Enable IPv6-only operation. IPv4 traffic will be ignored. > By default, IPv4 operation is enabled as long as at least an IPv4 route and an > interface address are configured on a given host interface. > > +.TP > +.BR \-H ", " \-\-hostname " " \fIname > +Hostname to configure the client with. > +Send \fIname\fR as DHCP option 12 (hostname). > + > +.TP > +.BR \-\-fqdn " " \fIname > +FQDN to configure client with. For consistency (with what you're adding above): "the client". > +Send \fIname\fR as DHCP client FQDN option, for DHCP option 81 and for Now that DHCP is capitalised I tried to read this sentence, and I'm not sure if "DHCP client FQDN option" makes a lot of sense. That is, isn't it simply the "DHCP FQDN option"? And, at that point, can't we simply say: Send \fIname\fR as FQDN: DHCP option 81 and DHCPv6 option 39. ? > +DHCPv6 option 39. > + > .SS \fBpasst\fR-only options > > .TP > diff --git a/passt.h b/passt.h > index 0dd4efa..f3151f0 100644 > --- a/passt.h > +++ b/passt.h > @@ -209,6 +209,8 @@ struct ip6_ctx { > * @ifi4: Template interface for IPv4, -1: none, 0: IPv4 disabled > * @ip: IPv4 configuration > * @dns_search: DNS search list > + * @hostname: Guest hostname > + * @fqdn: Guest FQDN > * @ifi6: Template interface for IPv6, -1: none, 0: IPv6 disabled > * @ip6: IPv6 configuration > * @pasta_ifn: Name of namespace interface for pasta > @@ -269,6 +271,9 @@ struct ctx { > > struct fqdn dns_search[MAXDNSRCH]; > > + char hostname[PASST_MAXDNAME]; > + char fqdn[PASST_MAXDNAME]; > + > int ifi6; > struct ip6_ctx ip6; > > diff --git a/pasta.c b/pasta.c > index ff41c95..5668eb3 100644 > --- a/pasta.c > +++ b/pasta.c > @@ -169,10 +169,12 @@ void pasta_open_ns(struct ctx *c, const char *netns) > * struct pasta_spawn_cmd_arg - Argument for pasta_spawn_cmd() > * @exe: Executable to run > * @argv: Command and arguments to run > + * @ctx: Context to read config from > */ > struct pasta_spawn_cmd_arg { > const char *exe; > char *const *argv; > + struct ctx *c; > }; > > /** > @@ -186,6 +188,7 @@ static int pasta_spawn_cmd(void *arg) > { > char hostname[HOST_NAME_MAX + 1] = HOSTNAME_PREFIX; > const struct pasta_spawn_cmd_arg *a; > + size_t conf_hostname_len; > sigset_t set; > > /* We run in a detached PID and mount namespace: mount /proc over */ > @@ -195,9 +198,15 @@ static int pasta_spawn_cmd(void *arg) > if (write_file("/proc/sys/net/ipv4/ping_group_range", "0 0")) > warn("Cannot set ping_group_range, ICMP requests might fail"); > > - if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1, > - HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) || > - errno == ENAMETOOLONG) { > + a = (const struct pasta_spawn_cmd_arg *)arg; > + > + conf_hostname_len = strlen(a->c->hostname); > + if (conf_hostname_len > 0) { > + if (sethostname(a->c->hostname, conf_hostname_len)) > + warn("Unable to set configured hostname"); > + } else if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1, > + HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) || Same as for v5: this is an argument to gethostname(), so it should be aligned accordingly. ! is a unary operator, it doesn't take two operands. That is: } else if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1, HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) || see also the version without your patch. > + errno == ENAMETOOLONG) { > hostname[HOST_NAME_MAX] = '\0'; > if (sethostname(hostname, strlen(hostname))) > warn("Unable to set pasta-prefixed hostname"); > @@ -208,7 +217,6 @@ static int pasta_spawn_cmd(void *arg) > sigaddset(&set, SIGUSR1); > sigwaitinfo(&set, NULL); > > - a = (const struct pasta_spawn_cmd_arg *)arg; > execvp(a->exe, a->argv); > > die_perror("Failed to start command or shell"); > @@ -230,6 +238,7 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid, > struct pasta_spawn_cmd_arg arg = { > .exe = argv[0], > .argv = argv, > + .c = c, > }; > char uidmap[BUFSIZ], gidmap[BUFSIZ]; > char *sh_argv[] = { NULL, NULL }; > diff --git a/test/lib/setup b/test/lib/setup > index 580825f..ee67152 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.test -P ${STATESETUP}/passt.pid" > > # pidfile isn't created until passt is listening > wait_for [ -f "${STATESETUP}/passt.pid" ] > @@ -160,11 +160,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.test -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.test -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" ] > > @@ -243,7 +243,7 @@ setup_two_guests() { > [ ${TRACE} -eq 1 ] && __opts="${__opts} --trace" > [ ${VHOST_USER} -eq 1 ] && __opts="${__opts} --vhost-user" > > - 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.test -H hostname1 -t 10001 -u 10001" > wait_for [ -f "${STATESETUP}/passt_1.pid" ] > > __opts= > @@ -252,7 +252,7 @@ setup_two_guests() { > [ ${TRACE} -eq 1 ] && __opts="${__opts} --trace" > [ ${VHOST_USER} -eq 1 ] && __opts="${__opts} --vhost-user" > > - 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" ] > > __vmem="$((${MEM_KIB} / 1024 / 4))" > 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..145f1ba 100644 > --- a/test/passt/dhcp > +++ b/test/passt/dhcp > @@ -11,7 +11,7 @@ > # Copyright (c) 2021 Red Hat GmbH > # Author: Stefano Brivio > > -gtools ip jq dhclient sed tr > +gtools ip jq dhclient sed tr hostname > htools ip jq sed tr head > > test Interface name > @@ -47,7 +47,16 @@ 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.test" ] > + > 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 > @@ -70,3 +79,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.test" ] > diff --git a/util.c b/util.c > index 11973c4..7ad9b55 100644 > --- a/util.c > +++ b/util.c > @@ -837,3 +837,30 @@ void raw_random(void *buf, size_t buflen) > if (random_read < buflen) > die("Unexpected EOF on random data source"); > } > +/** > + * encode_domain_name() - Encode domain name according to RFC 1035, section 3.1 > + * @buf: Buffer to fill in with encoded domain name > + * @domain_name: Input domain name as null terminated string NULL > + * > + * Return: encoded domain name length > + */ > +size_t encode_domain_name(char* buf, const char *domain_name) char *buf, for consistency. > +{ > + size_t i, len; > + char *p; > + Stray tab (ask git if your editor doesn't show you). > + len = strlen(domain_name); > + buf[0] = strcspn(domain_name, "."); > + p = buf + 1; > + for (i = 0; i < len; i++) { > + if (domain_name[i] == '.') > + p[i] = strcspn(domain_name + i + 1, "."); > + else > + p[i] = domain_name[i]; > + } > + > + /* domain name is terminated by a length byte of zero */ > + p[len + 1] = 0x00; > + > + return len + 2; > +} > diff --git a/util.h b/util.h > index 3fa1d12..474cacc 100644 > --- a/util.h > +++ b/util.h > @@ -40,6 +40,9 @@ > #ifndef IP_MAX_MTU > #define IP_MAX_MTU USHRT_MAX > #endif > +#ifndef IPV6_MIN_MTU > +#define IPV6_MIN_MTU 1280 > +#endif > > #ifndef MIN > #define MIN(x, y) (((x) < (y)) ? (x) : (y)) > @@ -346,4 +349,7 @@ static inline int wrap_accept4(int sockfd, struct sockaddr *addr, > #define accept4(s, addr, addrlen, flags) \ > wrap_accept4((s), (addr), (addrlen), (flags)) > > +#define PASST_MAXDNAME 253 /* RFC 1035 */ > +size_t encode_domain_name(char *buf, const char *domain_name); > + > #endif /* UTIL_H */ -- Stefano