Lib: Util: String conversion API

[Andrewpini]

Adds string conversion API.
Implements basic string to parameter convertion functions.

Signed-off-by: Anders Storrø [email protected]

[Andrewpini]

After talking to @PavelVPV we have agreed that this API should not be public. Further I have decided that the changes to subsys\bluetooth\shell.c should be done later as a separate task since there are additional changes that needs to be done.

[alwa-nordic]

I would like these API additions to go into the shell subsystem. They are not mesh-specific and I think others could benefit. And it will hopefully encourage a uniform interface.

[alxelax]

LGTM, agree with @PavelVPV comments

[Andrewpini]

Any CO that wants to add something? If there is no more dire requests for changes I would appreciate approval of this since it is a blocker for other tasks.

[carlescufi]

Is there a particular reason why you are not augmenting Zephyr's minimal C library instead? i.e. adding sscanf() to lib/libc/minimal?

[carlescufi]

@de-nordic and @alwa-nordic please take another look

[stephanosio]

I am not convinced why we need non-standard functions like this when there already exist the C standard functions to do this:

string_conv_str2long - strtol
string_conv_str2ulong - strtoul
string_conv_str2dbl - strtod

strtol and strtoul are already part of the minimal libc.
strtod is currently not part of the minimal libc (it is available if you use newlib though).

[de-nordic]

Why srlen(trimmed_buf) again? isnt line 122 already assigning the len with trimmed_buf len?

[de-nordic]

Is there a particular reason why you are not augmenting Zephyr's minimal C library instead? i.e. adding sscanf() to lib/libc/minimal?

Maybe that should be done instead.

[Andrewpini]

I am not convinced why we need non-standard functions like this when there already exist the C standard functions to do this:

string_conv_str2long - strtol
string_conv_str2ulong - strtoul
string_conv_str2dbl - strtod

strtol and strtoul are already part of the minimal libc. strtod is currently not part of the minimal libc (it is available if you use newlib though).

Well for my own part I do not have a strong opinion on this, other than that what I am trying to accomplish is not a unique usecase. This is the original description of the task that caused the PR:

The way the Bluetooth mesh shell clients extracts numeric values is vulnerable to garbage input strings.
In its current state a invalid numeric string will in most cases be translated to the value 0, 
which may corrupt the state of a device unintentionally. To improve this the parsing of 
numeric strings should be guarded by checks that ensure that the input is a valid 
numeric string, and prevent further command execution if it is not a valid numeric string.

So the main objective I want to acheive is to get error feedback on conversion of corrupt numeric strings. The implementation of strtol has, to the best of my knowledge, only error feedback on conversion overflow.

Initially this was placed as utility under the shell module for Bluetooth Mesh. Then it was pointed out that this was not mesh spesific functionality, and that it should be general shell utility. Then it was pointed out that this was not a shell spesific usecase either, and here we are. I guess moving it back to where I started is one viable option.

[stephanosio]

The implementation of strtol has, to the best of my knowledge, only error feedback on conversion overflow.

strto* supports error reporting through errno (ERANGE and EINVAL) and endptr (e.g. it will be NULL if no conversion is performed).

See https://pubs.opengroup.org/onlinepubs/9699919799/functions/strtol.html

Although the minimal libc implementation may not be fully compliant to the specification linked above at this time, it is important to note that the strto* functions themselves are capable of providing input error checks.

Waiting until dicussion on why not instead use libc functions is resolved.

[alwa-nordic]

Initially this was placed as utility under the shell module for Bluetooth Mesh. Then it was pointed out that this was not mesh spesific functionality, and that it should be general shell utility. Then it was pointed out that this was not a shell spesific usecase either, and here we are. I guess moving it back to where I started is one viable option.

Please, no. If these functions really are safer to use, they should be available and preferred everywhere.

[Thalley]

Initially this was placed as utility under the shell module for Bluetooth Mesh. Then it was pointed out that this was not mesh spesific functionality, and that it should be general shell utility. Then it was pointed out that this was not a shell spesific usecase either, and here we are. I guess moving it back to where I started is one viable option.

Please, no. If these functions really are safer to use, they should be available and preferred everywhere.

We could add them as wrappers around the existing functions, instead of re-inventing the logic. Basically just use the existing functions and add additional error handling.

[alwa-nordic]

I believe there is merit in a simpler API. strtol, strtoul and strtod are quite general, which is nice when we want to parse something complicated. But it can be complicated to use. E.g: Matching a decimal number with nothing around is surprisingly tedious. The following code demonstrates what is required.

/* The value of `out` is undefined in case of error. */
int parse_dec_to_long(long * out, char * str) {
        if (isspace(str[0])) {
                return -EINVAL;
        }
        char * endptr;
        errno = 0;
        *out = strtol(str, &endptr, 10);
        if (endptr[0] != '\0') {
                return -EINVAL;
        }
        return -errno;
}

Ignoring white space is not always the right thing to do. E.g. for protocols. But even when the user of strtol wants to ignore white space, they still need to check for garbage in endptr. (I.e. strtol("123abc", NULL, 10) will evaluate to 123 and not set an errno value.)

We should also consider the parsing of non-null-terminated strings. This cannot be easily done with strtol.