[PATCH 2/2] udhcpc: fixed a TODO in fill_envp using option scanner

Martin Lewis martin.lewis.x84 at gmail.com
Thu Jul 2 21:15:50 UTC 2020


Thank you for applying the commits!
I started implementing support for RFC 3396, hopefully it will be ready
soon.

Martin

On Mon, 29 Jun 2020 at 08:40, Denys Vlasenko <vda.linux at googlemail.com>
wrote:

> Applied, thanks!!!!
>
> On Tue, Jun 23, 2020 at 3:41 PM Martin Lewis <martin.lewis.x84 at gmail.com>
> wrote:
> >
> > fill_envp now iterates over the packet only once instead of a few
> hundred times
> > using the new option scanner.
> >
> > Signed-off-by: Martin Lewis <martin.lewis.x84 at gmail.com>
> > ---
> >  networking/udhcp/dhcpc.c | 201
> ++++++++++++++++++++---------------------------
> >  1 file changed, 87 insertions(+), 114 deletions(-)
> >
> > diff --git a/networking/udhcp/dhcpc.c b/networking/udhcp/dhcpc.c
> > index 102178a4f..2669428e1 100644
> > --- a/networking/udhcp/dhcpc.c
> > +++ b/networking/udhcp/dhcpc.c
> > @@ -386,59 +386,81 @@ static NOINLINE char
> *xmalloc_optname_optval(uint8_t *option, const struct dhcp_
> >         return ret;
> >  }
> >
> > +static void putenvp(llist_t **envp, char *new_opt)
> > +{
> > +       putenv(new_opt);
> > +       llist_add_to(envp, new_opt);
> > +}
> > +
> > +static int is_unknown_opt(uint8_t code, const struct dhcp_optflag **dh,
> const char **opt_name)
> > +{
> > +    *opt_name = "";
> > +
> > +    /* Find the option:
> > +     * dhcp_optflags is sorted so we stop searching when dh->code >=
> code, which is faster
> > +     * than iterating over the entire array.
> > +     * Options which don't have a match in dhcp_option_strings[], e.g
> DHCP_REQUESTED_IP,
> > +     * are located after the sorted array, so these entries will never
> be reached
> > +     * and they'll count as unknown options.
> > +     */
> > +    for (*dh = dhcp_optflags; (*dh)->code && (*dh)->code < code;
> (*dh)++);
> > +
> > +    if ((*dh)->code == code)
> > +        *opt_name = nth_string(dhcp_option_strings, (*dh -
> dhcp_optflags));
> > +
> > +    return (*dh)->code > code;
> > +}
> > +
> >  /* put all the parameters into the environment */
> > -static char **fill_envp(struct dhcp_packet *packet)
> > +static llist_t *fill_envp(struct dhcp_packet *packet)
> >  {
> > -       int envc;
> > -       int i;
> > -       char **envp, **curr;
> > -       const char *opt_name;
> > -       uint8_t *temp;
> > -       uint8_t overload = 0;
> > -
> > -#define BITMAP unsigned
> > -#define BBITS (sizeof(BITMAP) * 8)
> > -#define BMASK(i) (1 << (i & (sizeof(BITMAP) * 8 - 1)))
> > -#define FOUND_OPTS(i) (found_opts[(unsigned)i / BBITS])
> > -       BITMAP found_opts[256 / BBITS];
> > -
> > -       memset(found_opts, 0, sizeof(found_opts));
> > -
> > -       /* We need 7 elements for:
> > -        * "interface=IFACE"
> > -        * "ip=N.N.N.N" from packet->yiaddr
> > -        * "giaddr=IP" from packet->gateway_nip (unless 0)
> > -        * "siaddr=IP" from packet->siaddr_nip (unless 0)
> > -        * "boot_file=FILE" from packet->file (unless overloaded)
> > -        * "sname=SERVER_HOSTNAME" from packet->sname (unless overloaded)
> > -        * terminating NULL
> > -        */
> > -       envc = 7;
> > -       /* +1 element for each option, +2 for subnet option: */
> > -       if (packet) {
> > -               /* note: do not search for "pad" (0) and "end" (255)
> options */
> > -//TODO: change logic to scan packet _once_
> > -               for (i = 1; i < 255; i++) {
> > -                       temp = udhcp_get_option(packet, i);
> > -                       if (temp) {
> > -                               if (i == DHCP_OPTION_OVERLOAD)
> > -                                       overload |= *temp;
> > -                               else if (i == DHCP_SUBNET)
> > -                                       envc++; /* for $mask */
> > -                               envc++;
> > -                               /*if (i != DHCP_MESSAGE_TYPE)*/
> > -                               FOUND_OPTS(i) |= BMASK(i);
> > -                       }
> > -               }
> > -       }
> > -       curr = envp = xzalloc(sizeof(envp[0]) * envc);
> > +       char *new_opt;
> > +       uint8_t *optptr;
> > +       const struct dhcp_optflag *dh;
> > +       struct dhcp_scan_state scan_state;
> > +       const char *opt_name = "";
> > +       llist_t *envp = NULL;
> >
> > -       *curr = xasprintf("interface=%s", client_data.interface);
> > -       putenv(*curr++);
> > +       new_opt = xasprintf("interface=%s", client_data.interface);
> > +       putenvp(&envp, new_opt);
> >
> >         if (!packet)
> >                 return envp;
> >
> > +       init_scan_state(packet, &scan_state);
> > +
> > +    /* Iterate over the packet options.
> > +     * Handle each option based on whether it's an unknown / known
> option.
> > +     * There may be (although unlikely) duplicate options. For now,
> only the last
> > +     * appearing option will be stored in the environment, and all
> duplicates
> > +     * are freed properly.
> > +     * Long options may be implemented in the future (see RFC 3396) if
> needed.
> > +     */
> > +       while ((optptr = udhcp_scan_options(packet, &scan_state)) !=
> NULL) {
> > +               uint8_t code = optptr[OPT_CODE];
> > +               uint8_t len = optptr[OPT_LEN];
> > +               uint8_t *data = optptr + OPT_DATA;
> > +
> > +               if (is_unknown_opt(code, &dh, &opt_name)) {
> > +                       unsigned ofs;
> > +
> > +                       new_opt = xmalloc(sizeof("optNNN=") + 1 + len*2);
> > +                       ofs = sprintf(new_opt, "opt%u=", code);
> > +                       *bin2hex(new_opt + ofs, (char *)data, len) =
> '\0';
> > +                       putenvp(&envp, new_opt);
> > +               } else {
> > +                       new_opt = xmalloc_optname_optval(data, dh,
> opt_name);
> > +                       putenvp(&envp, new_opt);
> > +
> > +                       if (code == DHCP_SUBNET && len == 4) {
> > +                               uint32_t subnet;
> > +                               move_from_unaligned32(subnet, data);
> > +                               new_opt = xasprintf("mask=%u",
> mton(subnet));
> > +                               putenvp(&envp, new_opt);
> > +                       }
> > +               }
> > +       }
> > +
> >         /* Export BOOTP fields. Fields we don't (yet?) export:
> >          * uint8_t op;      // always BOOTREPLY
> >          * uint8_t htype;   // hardware address type. 1 = 10mb ethernet
> > @@ -452,77 +474,31 @@ static char **fill_envp(struct dhcp_packet *packet)
> >          * uint8_t chaddr[16]; // link-layer client hardware address
> (MAC)
> >          */
> >         /* Most important one: yiaddr as $ip */
> > -       *curr = xmalloc(sizeof("ip=255.255.255.255"));
> > -       sprint_nip(*curr, "ip=", (uint8_t *) &packet->yiaddr);
> > -       putenv(*curr++);
> > +       new_opt = xmalloc(sizeof("ip=255.255.255.255"));
> > +       sprint_nip(new_opt, "ip=", (uint8_t *) &packet->yiaddr);
> > +       putenvp(&envp, new_opt);
> > +
> >         if (packet->siaddr_nip) {
> >                 /* IP address of next server to use in bootstrap */
> > -               *curr = xmalloc(sizeof("siaddr=255.255.255.255"));
> > -               sprint_nip(*curr, "siaddr=", (uint8_t *)
> &packet->siaddr_nip);
> > -               putenv(*curr++);
> > +               new_opt = xmalloc(sizeof("siaddr=255.255.255.255"));
> > +               sprint_nip(new_opt, "siaddr=", (uint8_t *)
> &packet->siaddr_nip);
> > +               putenvp(&envp, new_opt);
> >         }
> >         if (packet->gateway_nip) {
> >                 /* IP address of DHCP relay agent */
> > -               *curr = xmalloc(sizeof("giaddr=255.255.255.255"));
> > -               sprint_nip(*curr, "giaddr=", (uint8_t *)
> &packet->gateway_nip);
> > -               putenv(*curr++);
> > +               new_opt = xmalloc(sizeof("giaddr=255.255.255.255"));
> > +               sprint_nip(new_opt, "giaddr=", (uint8_t *)
> &packet->gateway_nip);
> > +               putenvp(&envp, new_opt);
> >         }
> > -       if (!(overload & FILE_FIELD) && packet->file[0]) {
> > +       if (!(scan_state.overload & FILE_FIELD) && packet->file[0]) {
> >                 /* watch out for invalid packets */
> > -               *curr =
> xasprintf("boot_file=%."DHCP_PKT_FILE_LEN_STR"s", packet->file);
> > -               putenv(*curr++);
> > +               new_opt =
> xasprintf("boot_file=%."DHCP_PKT_FILE_LEN_STR"s", packet->file);
> > +               putenvp(&envp, new_opt);
> >         }
> > -       if (!(overload & SNAME_FIELD) && packet->sname[0]) {
> > +       if (!(scan_state.overload & SNAME_FIELD) && packet->sname[0]) {
> >                 /* watch out for invalid packets */
> > -               *curr = xasprintf("sname=%."DHCP_PKT_SNAME_LEN_STR"s",
> packet->sname);
> > -               putenv(*curr++);
> > -       }
> > -
> > -       /* Export known DHCP options */
> > -       opt_name = dhcp_option_strings;
> > -       i = 0;
> > -       while (*opt_name) {
> > -               uint8_t code = dhcp_optflags[i].code;
> > -               BITMAP *found_ptr = &FOUND_OPTS(code);
> > -               BITMAP found_mask = BMASK(code);
> > -               if (!(*found_ptr & found_mask))
> > -                       goto next;
> > -               *found_ptr &= ~found_mask; /* leave only unknown options
> */
> > -               temp = udhcp_get_option(packet, code);
> > -               *curr = xmalloc_optname_optval(temp, &dhcp_optflags[i],
> opt_name);
> > -               putenv(*curr++);
> > -               if (code == DHCP_SUBNET && temp[-OPT_DATA + OPT_LEN] ==
> 4) {
> > -                       /* Subnet option: make things like "$ip/$mask"
> possible */
> > -                       uint32_t subnet;
> > -                       move_from_unaligned32(subnet, temp);
> > -                       *curr = xasprintf("mask=%u", mton(subnet));
> > -                       putenv(*curr++);
> > -               }
> > - next:
> > -               opt_name += strlen(opt_name) + 1;
> > -               i++;
> > -       }
> > -       /* Export unknown options */
> > -       for (i = 0; i < 256;) {
> > -               BITMAP bitmap = FOUND_OPTS(i);
> > -               if (!bitmap) {
> > -                       i += BBITS;
> > -                       continue;
> > -               }
> > -               if (bitmap & BMASK(i)) {
> > -                       unsigned len, ofs;
> > -
> > -                       temp = udhcp_get_option(packet, i);
> > -                       /* udhcp_get_option returns ptr to data portion,
> > -                        * need to go back to get len
> > -                        */
> > -                       len = temp[-OPT_DATA + OPT_LEN];
> > -                       *curr = xmalloc(sizeof("optNNN=") + 1 + len*2);
> > -                       ofs = sprintf(*curr, "opt%u=", i);
> > -                       *bin2hex(*curr + ofs, (void*) temp, len) = '\0';
> > -                       putenv(*curr++);
> > -               }
> > -               i++;
> > +               new_opt = xasprintf("sname=%."DHCP_PKT_SNAME_LEN_STR"s",
> packet->sname);
> > +               putenvp(&envp, new_opt);
> >         }
> >
> >         return envp;
> > @@ -531,7 +507,7 @@ static char **fill_envp(struct dhcp_packet *packet)
> >  /* Call a script with a par file and env vars */
> >  static void udhcp_run_script(struct dhcp_packet *packet, const char
> *name)
> >  {
> > -       char **envp, **curr;
> > +       llist_t *envp;
> >         char *argv[3];
> >
> >         envp = fill_envp(packet);
> > @@ -543,11 +519,8 @@ static void udhcp_run_script(struct dhcp_packet
> *packet, const char *name)
> >         argv[2] = NULL;
> >         spawn_and_wait(argv);
> >
> > -       for (curr = envp; *curr; curr++) {
> > -               log2(" %s", *curr);
> > -               bb_unsetenv_and_free(*curr);
> > -       }
> > -       free(envp);
> > +    /* Free all allocated environment variables */
> > +       llist_free(envp, (void (*)(void *))bb_unsetenv_and_free);
> >  }
> >
> >
> > --
> > 2.11.0
> >
> > _______________________________________________
> > busybox mailing list
> > busybox at busybox.net
> > http://lists.busybox.net/mailman/listinfo/busybox
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20200702/1cf2e96c/attachment-0001.html>


More information about the busybox mailing list