On Mon, Nov 25, 2024 at 09:30:10AM +0100, Stefano Brivio wrote: > On Mon, 25 Nov 2024 12:08:35 +1100 > David Gibson wrote: > > > On Mon, Nov 25, 2024 at 01:04:21AM +0100, Stefano Brivio wrote: > > > We want to add support for option 80 (Rapid Commit, RFC 4039), whose > > > length is 0. > > > > > > Signed-off-by: Stefano Brivio > > > --- > > > dhcp.c | 27 ++++++++++++++++++++------- > > > 1 file changed, 20 insertions(+), 7 deletions(-) > > > > > > diff --git a/dhcp.c b/dhcp.c > > > index a06f143..2fe4a4d 100644 > > > --- a/dhcp.c > > > +++ b/dhcp.c > > > @@ -36,9 +36,9 @@ > > > /** > > > * struct opt - DHCP option > > > * @sent: Convenience flag, set while filling replies > > > - * @slen: Length of option defined for server > > > + * @slen: Length of option defined for server, -1 if not going to be sent > > > * @s: Option payload from server > > > - * @clen: Length of option received from client > > > + * @clen: Length of option received from client, -1 if not received > > > * @c: Option payload from client > > > */ > > > struct opt { > > > @@ -154,17 +154,17 @@ static int fill(struct msg *m) > > > * option 53 at the beginning of the list. > > > * Put it there explicitly, unless requested via option 55. > > > */ > > > - if (!memchr(opts[55].c, 53, opts[55].clen)) > > > + if (opts[55].clen > 0 && !memchr(opts[55].c, 53, opts[55].clen)) > > > fill_one(m, 53, &offset); > > > > > > for (i = 0; i < opts[55].clen; i++) { > > > o = opts[55].c[i]; > > > - if (opts[o].slen) > > > + if (opts[o].slen != -1) > > > fill_one(m, o, &offset); > > > } > > > > > > for (o = 0; o < 255; o++) { > > > - if (opts[o].slen && !opts[o].sent) > > > + if (opts[o].slen != -1 && !opts[o].sent) > > > fill_one(m, o, &offset); > > > } > > > > > > @@ -264,6 +264,9 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len) > > > ".\xc0"); > > > } > > > } > > > + > > > + if (!opts[119].slen) > > > + opts[119].slen = -1; > > > } > > > > > > /** > > > @@ -313,6 +316,13 @@ int dhcp(const struct ctx *c, const struct pool *p) > > > > > > offset += offsetof(struct msg, o); > > > > > > + for (i = 0; i < ARRAY_SIZE(opts); i++) { > > > + if (!opts[i].slen) > > > + opts[i].slen = -1; > > > + > > > + opts[i].clen = -1; > > > + } > > > > Could this move to dhcp_init()? I think there you wouldn't need test > > and could unconditionally initialize all the lengths to -1 before > > initializing the options we actually use. > > No, because dhcp_init() is run only once, and 'opts' at this point > represents the status from the previous run, so: > > - we need to unconditionally reset all the 'clen' attributes which were > set in the previous run Ah, yes of course. > - we need to reset the 'slen' attributes for zero-length options (it's > just option 80 at the moment) because we need to re-evaluate their > inclusion. Sure, I could also clean things up at the end of any run, > but this is more practical and robust Hrm... I see that you need to do _something_ here, but the zero length check still seems weird to me. Ideally, this change would mean that "no option" is always represented by -1 - but in a few places 0 sort of still does as well. There's no nice way in C to statically initialise all the entries to -1, so I can see why we have to set things to -1 with code in dhcp_init(), but I don't much like having ambiguous representation past that point. Shouldn't whether we reset a server side option be dependent only on which option it is, not whether it was zero-length the last time? The fact that this only affects option 80, which happens to be zero-length seems only correct by accident. > > > > while (opt_off + 2 < opt_len) { > > > const uint8_t *olen, *val; > > > uint8_t *type; > > > @@ -334,8 +344,9 @@ int dhcp(const struct ctx *c, const struct pool *p) > > > if (opts[53].c[0] == DHCPDISCOVER) { > > > info("DHCP: offer to discover"); > > > opts[53].s[0] = DHCPOFFER; > > > - } else if (opts[53].c[0] == DHCPREQUEST || !opts[53].clen) { > > > - info("%s: ack to request", opts[53].clen ? "DHCP" : "BOOTP"); > > > + } else if (opts[53].c[0] == DHCPREQUEST || opts[53].clen <= 0) { > > > + info("%s: ack to request", > > > + (opts[53].clen <= 0) ? "DHCP" : "BOOTP"); > > > > Should this be <= 0, or < 0? i.e. Wouldn't even an empty option 53 > > indicate we're dealing with DHCP rather than BOOTP? > > It should really be <= 0, preserving the existing behaviour, because if > option 53 is empty, we don't know what kind of DHCP message that is. We > know for sure that it's not a valid DHCP message, but it probably is a > valid BOOTP message (with a vendor extension). Ah, that makes sense. A comment to that effect might be useful. > This might look like speculation, but there are some half-DHCP > implementations from the 1990s which we can happily handle as BOOTP > clients, but not really as DHCP. After all the fun we had with wattcp32 > and mTCP I would say it's not unlikely. > > > > opts[53].s[0] = DHCPACK; > > > } else { > > > return -1; > > > @@ -374,6 +385,8 @@ int dhcp(const struct ctx *c, const struct pool *p) > > > ((struct in_addr *)opts[6].s)[i] = c->ip4.dns[i]; > > > opts[6].slen += sizeof(uint32_t); > > > } > > > + if (!opts[6].slen) > > > + opts[6].slen = -1; > > > > > > if (!c->no_dhcp_dns_search) > > > opt_set_dns_search(c, sizeof(m->o)); > -- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson