[prev in list] [next in list] [prev in thread] [next in thread]
List: busybox
Subject: Re: [PATCH 2/2] udhcpc: fixed a TODO in fill_envp using option scanner
From: Denys Vlasenko <vda.linux () googlemail ! com>
Date: 2020-06-29 13:40:43
Message-ID: CAK1hOcM5_Cx85J80GdFu=MY1yMQm3Ea-qHv8zoV6U5aW1Q7BWg () mail ! gmail ! com
[Download RAW message or body]
Applied, thanks!!!!
On Tue, Jun 23, 2020 at 3:41 PM Martin Lewis <martin.lewis.x84@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@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@busybox.net
> http://lists.busybox.net/mailman/listinfo/busybox
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic