Re: [PATCH v5 6/7] dhcp: Add RFC 3396 option splitting for concatenation-requiring options

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4259 bytes --]

On Mon, Jul 20, 2026 at 02:44:34PM +0530, Anshu Kumari wrote:
> On Mon, Jul 20, 2026 at 9:54 AM David Gibson <david@gibson.dropbear.id.au>
> wrote:
> 
> > On Fri, Jul 17, 2026 at 11:26:43PM +0530, Anshu Kumari wrote:
> > > Implement option splitting per RFC 3396 for options that may exceed
> > > 255 bytes.  A new DHCP_OPT_STR_CONCAT type marks concatenation-
> > > requiring options (currently option 81, Client FQDN per RFC 4702).
> > >
> > > The opts[].s buffer is resized from 255 to 496 bytes to hold the
> > > maximum data that can be split across the options field, file field,
> > > and sname field.
> > >
> > > When a concatenation-requiring option does not fit as a single option
> > > in any field, fill() splits it across fields in RFC 3396 order:
> > > options field first, then file, then sname.
> > >
> > > Link: https://bugs.passt.top/show_bug.cgi?id=192
> > > Signed-off-by: Anshu Kumari <anskuma@redhat.com>
> > > ---
> > > v5:
> > >   - New patch: implement option splitting per RFC 3396 for options
> > exceeding 255 bytes
> > >   - Add DHCP_OPT_STR_CONCAT type, is_concat_opt(), fill_split() helpers
> > >   - Resize opts[].s from 255 to OPT_CONCAT_MAX (496) bytes
> > >   - Add /* fallthrough */ between DHCP_OPT_STR and DHCP_OPT_STR_CONCAT
> > case
> > >
> > > ---
> > >  dhcp.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 106 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/dhcp.c b/dhcp.c
> > > index cc910ee..6bebb5f 100644
> > > --- a/dhcp.c
> > > +++ b/dhcp.c
> > > @@ -34,6 +34,11 @@
> > >  #include "log.h"
> > >  #include "dhcp.h"
> > >
> > > +/* RFC 3396: maximum option data that can be split across options field,
> > > + * file field, and sname field (minus code+length overhead per portion).
> > > + */
> > > +#define OPT_CONCAT_MAX       496
> > > +
> > >  /**
> > >   * enum opt_state - DHCP option state
> > >   * @OPT_UNSET:               Option not configured
> > > @@ -58,7 +63,7 @@ enum opt_state {
> > >  struct opt {
> > >       int sent;
> > >       int slen;
> > > -     uint8_t s[255];
> > > +     uint8_t s[OPT_CONCAT_MAX];
> > >       int clen;
> > >       uint8_t c[255];
> > >       enum opt_state state;
> > > @@ -159,6 +164,7 @@ struct msg {
> > >   * @DHCP_OPT_UINT16: Unsigned 16-bit integer
> > >   * @DHCP_OPT_UINT32: Unsigned 32-bit integer
> > >   * @DHCP_OPT_INT32:  Signed 32-bit integer
> > > + * @DHCP_OPT_STR_CONCAT:Concatenation-requiring string (RFC 3396)
> >
> > It's not entirely clear to me that encoding this in the opt_type enum
> > makes sense.  Generally the dhcp_opt_type is saying how the option is
> > encoded as a string for the user.  This is saying how it's encoded
> > within the DHCP packet itself, which seems qualitatively different
> > information.
> >
> > Since we don't limit on the string length that user can set via
> --dhcp-opt, DHCP_OPT_STR rejects
> anything over 255 bytes but with STR_CONCAT longer input can be parsed.
> 
> is_concat_opt() checks dhcp_opt_types[] for STR_CONCAT to decide which
> options need splitting in fill().
> I think these two concerns are coupled: an option that accepts long strings
> from the CLI
> is exactly the same option that needs splitting on the wire.

Ah, ok.  I see the case for including this in the type now.  However,
I still think it's not the best approach on balance.  Remember that
unless there's only one option, the practical limit on the length of
these options will be shorter - 256 is just an upper bound.  We can
calculate different upper bounds depending on whether it's a concat
option or not, but I wouldn't consider it exactly part of the type.

If we store the concat bit in a separate field of the options table,
we can still apply that different upper bound calculation.  Plus, we
can also use it for things of other types without introducing yet more
type variants.  Similarly we can use it for fqdn, or anything else we
don't have a parsing type for at present.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]