| Message ID | 1429653771-11676-6-git-send-email-joe.hershberger@ni.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
Hi Joe, On 21 April 2015 at 16:02, Joe Hershberger <joe.hershberger@ni.com> wrote: > The logic to find the whole matching name was split needlessly between > the reverse_strstr function and its caller. Fully contain it to make the > interface for calling it more consistent. > > Signed-off-by: Joe Hershberger <joe.hershberger@ni.com> > --- > > common/env_attr.c | 79 +++++++++++++++++++++++++++++-------------------------- > 1 file changed, 41 insertions(+), 38 deletions(-) > You could perhaps add some environment tests in test/ for this function, or access it through getenv(), etc. > diff --git a/common/env_attr.c b/common/env_attr.c > index e791f44..d266142 100644 > --- a/common/env_attr.c > +++ b/common/env_attr.c > @@ -109,33 +109,55 @@ int env_attr_walk(const char *attr_list, > } > > /* > - * Search for the last matching string in another string with the option to > - * start looking at a certain point (i.e. ignore anything beyond that point). > + * Search for the last exactly matching name in an attribute list > */ > -static char *reverse_strstr(const char *searched, const char *search_for, > - const char *searched_start) > +static int reverse_name_search(const char *searched, const char *search_for, > + const char **result) > { > - char *result = NULL; > + int result_size = 0; > + const char *cur_searched = searched; > + > + if (result) > + *result = NULL; > > if (*search_for == '\0') > return (char *)searched; > > for (;;) { > - char *match = strstr(searched, search_for); > - > - /* > - * Stop looking if no new match is found or looking past the > - * searched_start pointer > - */ > - if (match == NULL || (searched_start != NULL && > - match + strlen(search_for) > searched_start)) > + const char *match = strstr(cur_searched, search_for); > + const char *prevch; > + const char *nextch; > + > + /* Stop looking if no new match is found */ > + if (match == NULL) > break; > > - result = match; > - searched = match + 1; > + prevch = match - 1; > + nextch = match + strlen(search_for); > + > + /* Skip spaces */ > + while (*prevch == ' ') > + prevch--; > + while (*nextch == ' ') > + nextch++; > + > + /* Start looking past the current match so last is found */ > + cur_searched = match + 1; > + > + /* Check for an exact match */ > + if (match != searched && > + *prevch != ENV_ATTR_LIST_DELIM) > + continue; > + if (*nextch != ENV_ATTR_SEP && > + *nextch != ENV_ATTR_LIST_DELIM && > + *nextch != '\0') > + continue; > + > + *result = match; > + result_size = strlen(search_for); > } > > - return result; > + return result_size; > } > > /* > @@ -145,6 +167,7 @@ static char *reverse_strstr(const char *searched, const char *search_for, > int env_attr_lookup(const char *attr_list, const char *name, char *attributes) > { > const char *entry = NULL; > + int entry_len; > > if (!attributes) > /* bad parameter */ > @@ -153,32 +176,12 @@ int env_attr_lookup(const char *attr_list, const char *name, char *attributes) > /* list not found */ > return -EINVAL; > > - entry = reverse_strstr(attr_list, name, NULL); > - while (entry != NULL) { > - const char *prevch = entry - 1; > - const char *nextch = entry + strlen(name); > - > - /* Skip spaces */ > - while (*prevch == ' ') > - prevch--; > - while (*nextch == ' ') > - nextch++; > - > - /* check for an exact match */ > - if ((entry == attr_list || > - *prevch == ENV_ATTR_LIST_DELIM) && > - (*nextch == ENV_ATTR_SEP || > - *nextch == ENV_ATTR_LIST_DELIM || > - *nextch == '\0')) > - break; > - > - entry = reverse_strstr(attr_list, name, entry); > - } > + entry_len = reverse_name_search(attr_list, name, &entry); > if (entry != NULL) { > int len; > > /* skip the name */ > - entry += strlen(name); > + entry += entry_len; > /* skip spaces */ > while (*entry == ' ') > entry++; > -- > 1.7.11.5 Regards, Simon
Hi Simon, On Thu, Apr 23, 2015 at 11:34 PM, Simon Glass <sjg@chromium.org> wrote: > Hi Joe, > > On 21 April 2015 at 16:02, Joe Hershberger <joe.hershberger@ni.com> wrote: >> The logic to find the whole matching name was split needlessly between >> the reverse_strstr function and its caller. Fully contain it to make the >> interface for calling it more consistent. >> >> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com> >> --- >> >> common/env_attr.c | 79 +++++++++++++++++++++++++++++-------------------------- >> 1 file changed, 41 insertions(+), 38 deletions(-) >> > > You could perhaps add some environment tests in test/ for this > function, or access it through getenv(), etc. I'll look into adding some unit tests for the env stuff. Cheers, -Joe
Hi Simon, On Mon, Apr 27, 2015 at 2:24 PM, Joe Hershberger <joe.hershberger@gmail.com> wrote: > Hi Simon, > > On Thu, Apr 23, 2015 at 11:34 PM, Simon Glass <sjg@chromium.org> wrote: >> Hi Joe, >> >> On 21 April 2015 at 16:02, Joe Hershberger <joe.hershberger@ni.com> wrote: >>> The logic to find the whole matching name was split needlessly between >>> the reverse_strstr function and its caller. Fully contain it to make the >>> interface for calling it more consistent. >>> >>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com> >>> --- >>> >>> common/env_attr.c | 79 +++++++++++++++++++++++++++++-------------------------- >>> 1 file changed, 41 insertions(+), 38 deletions(-) >>> >> >> You could perhaps add some environment tests in test/ for this >> function, or access it through getenv(), etc. > > I'll look into adding some unit tests for the env stuff. I'd like to reuse a bit of the unit test code from DM tests... do you have any plans to generalize some of it? Or should I take a crack at some of it (that I want to reuse)? Thanks, -Joe
Hi Joe, On 27 April 2015 at 13:31, Joe Hershberger <joe.hershberger@gmail.com> wrote: > Hi Simon, > > On Mon, Apr 27, 2015 at 2:24 PM, Joe Hershberger > <joe.hershberger@gmail.com> wrote: >> Hi Simon, >> >> On Thu, Apr 23, 2015 at 11:34 PM, Simon Glass <sjg@chromium.org> wrote: >>> Hi Joe, >>> >>> On 21 April 2015 at 16:02, Joe Hershberger <joe.hershberger@ni.com> wrote: >>>> The logic to find the whole matching name was split needlessly between >>>> the reverse_strstr function and its caller. Fully contain it to make the >>>> interface for calling it more consistent. >>>> >>>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com> >>>> --- >>>> >>>> common/env_attr.c | 79 +++++++++++++++++++++++++++++-------------------------- >>>> 1 file changed, 41 insertions(+), 38 deletions(-) >>>> >>> >>> You could perhaps add some environment tests in test/ for this >>> function, or access it through getenv(), etc. >> >> I'll look into adding some unit tests for the env stuff. > > I'd like to reuse a bit of the unit test code from DM tests... do you > have any plans to generalize some of it? Or should I take a crack at > some of it (that I want to reuse)? No plans, please go ahead! Regards, Simon
diff --git a/common/env_attr.c b/common/env_attr.c index e791f44..d266142 100644 --- a/common/env_attr.c +++ b/common/env_attr.c @@ -109,33 +109,55 @@ int env_attr_walk(const char *attr_list, } /* - * Search for the last matching string in another string with the option to - * start looking at a certain point (i.e. ignore anything beyond that point). + * Search for the last exactly matching name in an attribute list */ -static char *reverse_strstr(const char *searched, const char *search_for, - const char *searched_start) +static int reverse_name_search(const char *searched, const char *search_for, + const char **result) { - char *result = NULL; + int result_size = 0; + const char *cur_searched = searched; + + if (result) + *result = NULL; if (*search_for == '\0') return (char *)searched; for (;;) { - char *match = strstr(searched, search_for); - - /* - * Stop looking if no new match is found or looking past the - * searched_start pointer - */ - if (match == NULL || (searched_start != NULL && - match + strlen(search_for) > searched_start)) + const char *match = strstr(cur_searched, search_for); + const char *prevch; + const char *nextch; + + /* Stop looking if no new match is found */ + if (match == NULL) break; - result = match; - searched = match + 1; + prevch = match - 1; + nextch = match + strlen(search_for); + + /* Skip spaces */ + while (*prevch == ' ') + prevch--; + while (*nextch == ' ') + nextch++; + + /* Start looking past the current match so last is found */ + cur_searched = match + 1; + + /* Check for an exact match */ + if (match != searched && + *prevch != ENV_ATTR_LIST_DELIM) + continue; + if (*nextch != ENV_ATTR_SEP && + *nextch != ENV_ATTR_LIST_DELIM && + *nextch != '\0') + continue; + + *result = match; + result_size = strlen(search_for); } - return result; + return result_size; } /* @@ -145,6 +167,7 @@ static char *reverse_strstr(const char *searched, const char *search_for, int env_attr_lookup(const char *attr_list, const char *name, char *attributes) { const char *entry = NULL; + int entry_len; if (!attributes) /* bad parameter */ @@ -153,32 +176,12 @@ int env_attr_lookup(const char *attr_list, const char *name, char *attributes) /* list not found */ return -EINVAL; - entry = reverse_strstr(attr_list, name, NULL); - while (entry != NULL) { - const char *prevch = entry - 1; - const char *nextch = entry + strlen(name); - - /* Skip spaces */ - while (*prevch == ' ') - prevch--; - while (*nextch == ' ') - nextch++; - - /* check for an exact match */ - if ((entry == attr_list || - *prevch == ENV_ATTR_LIST_DELIM) && - (*nextch == ENV_ATTR_SEP || - *nextch == ENV_ATTR_LIST_DELIM || - *nextch == '\0')) - break; - - entry = reverse_strstr(attr_list, name, entry); - } + entry_len = reverse_name_search(attr_list, name, &entry); if (entry != NULL) { int len; /* skip the name */ - entry += strlen(name); + entry += entry_len; /* skip spaces */ while (*entry == ' ') entry++;
The logic to find the whole matching name was split needlessly between the reverse_strstr function and its caller. Fully contain it to make the interface for calling it more consistent. Signed-off-by: Joe Hershberger <joe.hershberger@ni.com> --- common/env_attr.c | 79 +++++++++++++++++++++++++++++-------------------------- 1 file changed, 41 insertions(+), 38 deletions(-)