On 27.11.2017 21:55, Collin L. Walling wrote:
> Moved:
> memcmp from bootmap.h to libc.h (renamed from _memcmp)
> strlen from sclp.c to libc.h (renamed from _strlen)
>
> Added C standard functions:
> isdigit
> atoi
>
> Added non-C standard function:
> itostr
>
> Signed-off-by: Collin L. Walling <wall...@linux.vnet.ibm.com>
> Acked-by: Christian Borntraeger <borntrae...@de.ibm.com>
> Reviewed-by: fran...@linux.vnet.ibm.com
> ---
[...]
> +/* atoi
> + *
> + * Given a string, convert it to an integer. Any
> + * non-numerical value will end the conversion.
> + *
> + * @str - the string to be converted
> + *
> + * @return - an integer converted from the string str
> + */
Looks like you want to use some kind of doc text markup here, but it
does not look like any valid syntax that I know ... may I suggest to use
gtk-doc as this is what most other QEMU developers are using (AFAIK)?
> +static inline int atoi(const char *str)
> +{
> + int i;
> + int val = 0;
> +
> + for (i = 0; str[i]; i++) {
> + char c = str[i];
> + if (!isdigit(c)) {
> + break;
> + }
> + val *= 10;
> + val += c - '0';
> + }
> +
> + return val;
> +}
> +
> +/* itostr
> + *
> + * Given an integer, convert it to a string. The string must be allocated
> + * beforehand. The resulting string will be null terminated and returned.
> + *
> + * @str - the integer to be converted
> + * @num - a pointer to a string to store the conversion
> + *
> + * @return - the string of the converted integer
> + */
Smells like a good candidate for hard-to-debug buffer overruns later.
Could you please add a "length" parameter to do some sanity checking of
the destination buffer length, please?
> +static inline char *itostr(int num, char *str)
> +{
> + int i;
> + int len = 0;
> + long tens = 1;
> +
> + /* Handle 0 */
> + if (num == 0) {
> + str[0] = '0';
> + str[1] = '\0';
> + return str;
> + }
> +
> + /* Count number of digits */
> + while (num / tens != 0) {
> + tens *= 10;
> + len++;
> + }
> +
> + /* Convert int -> string */
> + for (i = 0; i < len; i++) {
> + tens /= 10;
> + str[i] = num / tens % 10 + '0';
> + }
> +
> + str[i] = '\0';
> +
> + return str;
> +}
Having such a bigger, and likely performance-uncritical function as an
inline function in a header sounds somewhat wrong. Could you maybe move
this (and atoi()) into a proper .c file (libc.c?) instead, please?
Thanks,
Thomas
