| Message ID | 20220119014315.1938157-4-sjg@chromium.org |
|---|---|
| State | RFC |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Initial implementation of standard boot | expand |
On 19/01/2022 02.42, Simon Glass wrote: > At present this has a minor bug in that it reads the byte before the > start of the string. Only for an empty string, AFAICS. Which is a bug, of course, but mostly the caller is to blame. Also it doesn't handle a non-numeric prefix which is > only one character long. > > Fix these bugs with a reworked implementation. How does your new implementation handle the case of no prefix at all, i.e. "456"? Shouldn't that also work? Or a single-digit "7"? Both the old and new seem to have a bug in that the end parameter is essentially ignored. If I have const char *s = "abc123"; and do trailing_strtoln(s, s+5); I'd expect 12. Rasmus
Hi Rasmus, On Mon, 31 Jan 2022 at 02:44, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > > On 19/01/2022 02.42, Simon Glass wrote: > > At present this has a minor bug in that it reads the byte before the > > start of the string. > > Only for an empty string, AFAICS. Which is a bug, of course, but mostly > the caller is to blame. Yes > > Also it doesn't handle a non-numeric prefix which is > > only one character long. > > > > Fix these bugs with a reworked implementation. > > How does your new implementation handle the case of no prefix at all, > i.e. "456"? Shouldn't that also work? Or a single-digit "7"? It is not designed to work for that case. You could just use dectoul() or similar. > > Both the old and new seem to have a bug in that the end parameter is > essentially ignored. If I have > > const char *s = "abc123"; > > and do > > trailing_strtoln(s, s+5); > > I'd expect 12. That is intentional though - end is expected to point to a letter. I can reword the function comment to cover both of these cases. Regards, Simon
diff --git a/lib/strto.c b/lib/strto.c index 7bba1e3e549..58fc10ecaea 100644 --- a/lib/strto.c +++ b/lib/strto.c @@ -189,11 +189,12 @@ long trailing_strtoln(const char *str, const char *end) if (!end) end = str + strlen(str); - if (isdigit(end[-1])) { - for (p = end - 1; p > str; p--) { - if (!isdigit(*p)) - return dectoul(p + 1, NULL); - } + p = end - 1; + if (p > str && isdigit(*p)) { + do { + if (!isdigit(p[-1])) + return dectoul(p, NULL); + } while (--p > str); } return -1; diff --git a/test/str_ut.c b/test/str_ut.c index 9674a59f2a6..058b3594379 100644 --- a/test/str_ut.c +++ b/test/str_ut.c @@ -257,6 +257,8 @@ static int str_trailing(struct unit_test_state *uts) ut_asserteq(123, trailing_strtoln(str1, str1 + 6)); ut_asserteq(-1, trailing_strtoln(str1, str1 + 9)); + ut_asserteq(3, trailing_strtol("a3")); + return 0; } STR_TEST(str_trailing, 0);
At present this has a minor bug in that it reads the byte before the start of the string. Also it doesn't handle a non-numeric prefix which is only one character long. Fix these bugs with a reworked implementation. Add a test for the second case. The first one is hard to test. Signed-off-by: Simon Glass <sjg@chromium.org> --- (no changes since v1) lib/strto.c | 11 ++++++----- test/str_ut.c | 2 ++ 2 files changed, 8 insertions(+), 5 deletions(-)