Re: [PATCH] libbb/last_char_is: rewrite for smaller and faster code

2020-07-02 Thread Jody Bruchon



On July 2, 2020 3:26:29 PM EDT, Tito  wrote:
>hi,
>just for fun I add my version:
>
>char* FAST_FUNC last_char_is(const char *s, int c)  {
>   while (s && *s && *(s + 1)) s++;
>   return (c == *s) ? (char *)s : NULL;
>}

That's pretty small, but it performs three comparisons for each character until 
the end and effectively scans the string twice in "parallel." If a bad pointer 
is passed in s, it'll segfault at the equality check in the return statement 
even though the first comparison is supposed to be a null pointer safeguard.
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox


Re: [PATCH] libbb/last_char_is: rewrite for smaller and faster code

2020-07-02 Thread Tito
On 7/2/20 12:02 AM, Jody Bruchon wrote:
> From 10389ff81b4ee5a7db293cf5d6205d8944e51901 Mon Sep 17 00:00:00 2001
> From: Jody Bruchon 
> Date: Wed, 1 Jul 2020 17:55:04 -0400
> Subject: [PATCH] libbb/last_char_is: rewrite for smaller and faster code
> 
> Signed-off-by: Jody Bruchon 
> ---
>  libbb/last_char_is.c | 14 +-
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/libbb/last_char_is.c b/libbb/last_char_is.c
> index 918526e6c..b146b606f 100644
> --- a/libbb/last_char_is.c
> +++ b/libbb/last_char_is.c
> @@ -9,16 +9,12 @@
>  #include "libbb.h"
> 
>  /* Find out if the last character of a string matches the one given */
> -char* FAST_FUNC last_char_is(const char *s, int c)
> +char* FAST_FUNC last_char_is(char *s, char c)
>  {
> -    if (s) {
> -        size_t sz = strlen(s);
> -        /* Don't underrun the buffer if the string length is 0 */
> -        if (sz != 0) {
> -            s += sz - 1;
> -            if ((unsigned char)*s == c)
> -                return (char*)s;
> -        }
> +    if (s && *s) {
> +        while (*s != '\0') s++;
> +        s--;
> +        if (*s == c) return s;
>      }
>      return NULL;
>  }


hi,
just for fun I add my version:

char* FAST_FUNC last_char_is(const char *s, int c)  {
while (s && *s && *(s + 1)) s++;
return (c == *s) ? (char *)s : NULL;
}


make bloatcheck 
function old new   delta
last_char_is  53  47  -6
--
(add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-6)   Total: -6 bytes
   textdata bss dec hex filename
 980860   168911872  999623   f40c7 busybox_old
 980854   168911872  999617   f40c1 busybox_unstripped

Ciao,
Tito

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox


Re: [PATCH] libbb/last_char_is: rewrite for smaller and faster code

2020-07-02 Thread Kang-Che Sung
On Friday, July 3, 2020, Jody Bruchon  wrote:
>
>
> On July 2, 2020 11:29:06 AM EDT, Kang-Che Sung 
wrote:
>>On Thursday, July 2, 2020, Jody Bruchon  wrote:
>>>  /* Find out if the last character of a string matches the one given
>>*/
>>> -char* FAST_FUNC last_char_is(const char *s, int c)
>>> +char* FAST_FUNC last_char_is(char *s, char c)
>>
>>Why are you removing the const qualifier, and how would that reduce the
>>code size?
>
> Why does it need the const in the first place? The code I wrote does
reduce the code size.

Any pointer parameter whose content is meant to be read-only is good to be
declared const. It's a compiler safety check that you won't accidentally
modify the content there. The const qualifier by itself doesn't add code.
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox


Re: [PATCH] libbb/last_char_is: rewrite for smaller and faster code

2020-07-02 Thread Kang-Che Sung
On Thursday, July 2, 2020, Jody Bruchon  wrote:
>  /* Find out if the last character of a string matches the one given */
> -char* FAST_FUNC last_char_is(const char *s, int c)
> +char* FAST_FUNC last_char_is(char *s, char c)

Why are you removing the const qualifier, and how would that reduce the
code size?
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox


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

2020-07-02 Thread Martin Lewis
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 
wrote:

> Applied, thanks
>
> On Tue, Jun 23, 2020 at 3:41 PM Martin Lewis 
> 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 
> > ---
> >  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(, new_opt);
> >
> > if (!packet)
> > return envp;
> >
> > +   init_scan_state(packet, _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, _state)) !=
> NULL) {
> > +