Re: [PATCH v11] dhcp, dhcpv6: Add hostname and client fqdn ops

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 11 Feb 2025 11:36:41 +0100
Enrique Llorente Pastora <ellorent@redhat.com> wrote:

> On Mon, Feb 10, 2025 at 7:15 PM Stefano Brivio <sbrivio@redhat.com> wrote:
> 
> > On Fri,  7 Feb 2025 12:36:55 +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>  
> >
> > I tried to break it but this time my little hammer broke instead...
> >
> > Applied, thanks!
> >
> > I noticed one (pre-existing) issue:
> >  
> > > +/* 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 */ ];  
> >
> > ...actually, option 255 is special in that it takes just one byte,
> > because it has no length byte.
> >
> > I just assumed things from my memory without re-reading the RFC, sorry
> > for that. That's RFC 2131, section 3.2.
> >
> > It's different from option 80, Rapid Commit, RFC 4039, which takes two
> > bytes (zero length, and one length byte).
> >
> > So we should change the two code lines above, and drop my stupid:
> >
> >         m->o[offset++] = 0;
> 
> And also change the "o" declaration as the following right ?
> 
> uint8_t o[OPT_MAX + 1 /* End option */ ];

Yes. I haven't checked the rest.

> > It's harmless because padding is harmless (option 0, also without
> > length byte, taking one byte), but not needed, and we could stuff one
> > extra byte in the response.
> 
> I will push a little follow up with these things.

Thanks!

-- 
Stefano