| Message ID | 1516732013-18272-5-git-send-email-walling@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/23/2018 12:26 PM, 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: > atoi > isdigit > > Added non C-standard function: > itostr > > Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com> > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> > Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com> > --- > +/** > + * atoi: > + * @str: the string to be converted. > + * > + * Given a string @str, convert it to an integer. Leading whitespace is > + * ignored. The first character (after any whitespace) is checked for the > + * negative sign. Any other non-numerical value will terminate the > + * conversion. > + * > + * Returns: an integer converted from the string @str. > + */ > +int atoi(const char *str) > +{ > + int val = 0; > + int sign = 1; > + > + if (!str || !str[0]) { > + return 0; > + } > + > + while (*str == ' ') { > + str++; > + } That's not "any whitespace", but only spaces. A fully compliant implementation would be checking isspace(), but I don't expect you to implement that; at a minimum, also checking '\t' would get you closer (but not all the way to) compliance. > +static char *_itostr(int num, char *str, size_t len) > +{ > + int num_idx = 0; > + int tmp = num; > + char sign = 0; > + > + if (!str) { > + return NULL; > + } > + > + /* Get index to ones place */ > + while ((tmp /= 10) != 0) { > + num_idx++; > + } > + > + if (num < 0) { > + num *= -1; > + sign = 1; > + } If num == INT_MIN, then num is still negative at this point... > + > + /* Check if we have enough space for num, sign, and null */ > + if (len <= num_idx + sign + 1) { > + return NULL; > + } > + > + str[num_idx + sign + 1] = '\0'; > + > + /* Convert int to string */ > + while (num_idx >= 0) { > + str[num_idx + sign] = num % 10 + '0'; ...which breaks this. Either make it work, or document the corner case as unsupported.
On 01/23/2018 02:23 PM, Eric Blake wrote: > On 01/23/2018 12:26 PM, Collin L. Walling wrote: >> [...] >> +/** >> + * atoi: >> + * @str: the string to be converted. >> + * >> + * Given a string @str, convert it to an integer. Leading whitespace is >> + * ignored. The first character (after any whitespace) is checked for the >> + * negative sign. Any other non-numerical value will terminate the >> + * conversion. >> + * >> + * Returns: an integer converted from the string @str. >> + */ >> +int atoi(const char *str) >> +{ >> + int val = 0; >> + int sign = 1; >> + >> + if (!str || !str[0]) { >> + return 0; >> + } >> + >> + while (*str == ' ') { >> + str++; >> + } > That's not "any whitespace", but only spaces. A fully compliant > implementation would be checking isspace(), but I don't expect you to > implement that; at a minimum, also checking '\t' would get you closer > (but not all the way to) compliance. I'll fix the comment to be more clear. I think it's okay to just have the menu code treat any other kind of whitespace as an error (it will check before calling atoi). I added support for negatives in bothfunctions because it was easy enough to do so and for the sakeof completeness. However, I worry trying to be 100% compliant will just bloat the code when we only need it for very specific use cases. Would you say what we have (along with the fix to itostr below) is sufficient enough? > > >> +static char *_itostr(int num, char *str, size_t len) >> +{ >> + int num_idx = 0; >> + int tmp = num; >> + char sign = 0; >> + >> + if (!str) { >> + return NULL; >> + } >> + >> + /* Get index to ones place */ >> + while ((tmp /= 10) != 0) { >> + num_idx++; >> + } >> + >> + if (num < 0) { >> + num *= -1; >> + sign = 1; >> + } > If num == INT_MIN, then num is still negative at this point... > >> + >> + /* Check if we have enough space for num, sign, and null */ >> + if (len <= num_idx + sign + 1) { >> + return NULL; >> + } >> + >> + str[num_idx + sign + 1] = '\0'; >> + >> + /* Convert int to string */ >> + while (num_idx >= 0) { >> + str[num_idx + sign] = num % 10 + '0'; > ...which breaks this. > > Either make it work, or document the corner case as unsupported. > Might as well just make it work at this point: #define INT32_MIN 0x80000000 static char *itostr(int num, char *str, size_t len) { int num_idx = 0; int tmp = num; char sign = !!(num & INT32_MIN); if (!str) { return NULL; } /* Get index to ones place */ while ((tmp /= 10) != 0) { num_idx++; } /* Check if we have enough space for num, sign, and null */ if (len <= num_idx + sign + 1) { return NULL; } str[num_idx + sign + 1] = '\0'; if (sign) { str[0] = '-'; if (num == INT32_MIN) { str[num_idx + sign] = '8'; num /= 10; num_idx--; } num *= -1; } /* Convert int to string */ while (num_idx >= 0) { str[num_idx + sign] = num % 10 + '0'; num /= 10; num_idx--; } return str; } Thoughts?
On 23.01.2018 23:33, Collin L. Walling wrote: > On 01/23/2018 02:23 PM, Eric Blake wrote: >> On 01/23/2018 12:26 PM, Collin L. Walling wrote: >>> [...] >>> +/** >>> + * atoi: >>> + * @str: the string to be converted. >>> + * >>> + * Given a string @str, convert it to an integer. Leading whitespace is >>> + * ignored. The first character (after any whitespace) is checked >>> for the >>> + * negative sign. Any other non-numerical value will terminate the >>> + * conversion. >>> + * >>> + * Returns: an integer converted from the string @str. >>> + */ >>> +int atoi(const char *str) >>> +{ >>> + int val = 0; >>> + int sign = 1; >>> + >>> + if (!str || !str[0]) { >>> + return 0; >>> + } >>> + >>> + while (*str == ' ') { >>> + str++; >>> + } >> That's not "any whitespace", but only spaces. A fully compliant >> implementation would be checking isspace(), but I don't expect you to >> implement that; at a minimum, also checking '\t' would get you closer >> (but not all the way to) compliance. > > > I'll fix the comment to be more clear. > > I think it's okay to just have the menu code treat any other kind > of whitespace as an error (it will check before calling atoi). I > added support for negatives in bothfunctions because it was easy > enough to do so and for the sakeof completeness. > > However, I worry trying to be 100% compliant will just bloat the > code when we only need it for very specific use cases. > > Would you say what we have (along with the fix to itostr below) is > sufficient enough? IMHO the current way is good enough for a BIOS implementation. We're not doing a full replacement of glibc here ;-) > >> >> >>> +static char *_itostr(int num, char *str, size_t len) >>> +{ >>> + int num_idx = 0; >>> + int tmp = num; >>> + char sign = 0; >>> + >>> + if (!str) { >>> + return NULL; >>> + } >>> + >>> + /* Get index to ones place */ >>> + while ((tmp /= 10) != 0) { >>> + num_idx++; >>> + } >>> + >>> + if (num < 0) { >>> + num *= -1; >>> + sign = 1; >>> + } >> If num == INT_MIN, then num is still negative at this point... >> >>> + >>> + /* Check if we have enough space for num, sign, and null */ >>> + if (len <= num_idx + sign + 1) { >>> + return NULL; >>> + } >>> + >>> + str[num_idx + sign + 1] = '\0'; >>> + >>> + /* Convert int to string */ >>> + while (num_idx >= 0) { >>> + str[num_idx + sign] = num % 10 + '0'; >> ...which breaks this. >> >> Either make it work, or document the corner case as unsupported. >> > > Might as well just make it work at this point: > > #define INT32_MIN 0x80000000 > > static char *itostr(int num, char *str, size_t len) > { > int num_idx = 0; > int tmp = num; > char sign = !!(num & INT32_MIN); > > if (!str) { > return NULL; > } > > /* Get index to ones place */ > while ((tmp /= 10) != 0) { > num_idx++; > } > > /* Check if we have enough space for num, sign, and null */ > if (len <= num_idx + sign + 1) { > return NULL; > } > > str[num_idx + sign + 1] = '\0'; > > if (sign) { > str[0] = '-'; > if (num == INT32_MIN) { > str[num_idx + sign] = '8'; > num /= 10; > num_idx--; > } > num *= -1; > } > > /* Convert int to string */ > while (num_idx >= 0) { > str[num_idx + sign] = num % 10 + '0'; > num /= 10; > num_idx--; > } > > return str; > } > > Thoughts? Looks fine to me. With that modification: Reviewed-by: Thomas Huth <thuth@redhat.com>
On 01/25/2018 06:06 AM, Thomas Huth wrote: >>> That's not "any whitespace", but only spaces. A fully compliant >>> implementation would be checking isspace(), but I don't expect you to >>> implement that; at a minimum, also checking '\t' would get you closer >>> (but not all the way to) compliance. >> >> >> I'll fix the comment to be more clear. >> >> I think it's okay to just have the menu code treat any other kind >> of whitespace as an error (it will check before calling atoi). I >> added support for negatives in bothfunctions because it was easy >> enough to do so and for the sakeof completeness. >> >> However, I worry trying to be 100% compliant will just bloat the >> code when we only need it for very specific use cases. >> >> Would you say what we have (along with the fix to itostr below) is >> sufficient enough? > > IMHO the current way is good enough for a BIOS implementation. We're not > doing a full replacement of glibc here ;-) Documenting the issue is the best approach; don't bloat the code for something that none of the callers care about. Perhaps as simple as adding: NOTE: This function is not quite like the standardized version in libc; it does not handle all forms of leading space or INT_MIN. (or whatever the actual differences are, based on your implementation choices). Another solution is to just not use the standardized name; keeping it named _atoi() instead of atoi() makes it obvious that you are calling an internal function that does not have to have standardized semantics.
On 01/25/2018 10:08 AM, Eric Blake wrote: > On 01/25/2018 06:06 AM, Thomas Huth wrote: >>>> That's not "any whitespace", but only spaces. A fully compliant >>>> implementation would be checking isspace(), but I don't expect you to >>>> implement that; at a minimum, also checking '\t' would get you closer >>>> (but not all the way to) compliance. >>> >>> I'll fix the comment to be more clear. >>> >>> I think it's okay to just have the menu code treat any other kind >>> of whitespace as an error (it will check before calling atoi). I >>> added support for negatives in bothfunctions because it was easy >>> enough to do so and for the sakeof completeness. >>> >>> However, I worry trying to be 100% compliant will just bloat the >>> code when we only need it for very specific use cases. >>> >>> Would you say what we have (along with the fix to itostr below) is >>> sufficient enough? >> IMHO the current way is good enough for a BIOS implementation. We're not >> doing a full replacement of glibc here ;-) > Documenting the issue is the best approach; don't bloat the code for > something that none of the callers care about. Perhaps as simple as adding: > > NOTE: This function is not quite like the standardized version in libc; > it does not handle all forms of leading space or INT_MIN. > > (or whatever the actual differences are, based on your implementation > choices). > > Another solution is to just not use the standardized name; keeping it > named _atoi() instead of atoi() makes it obvious that you are calling an > internal function that does not have to have standardized semantics. > Sound good. I'll prepend the underscore and document the differences. Thanks for your suggestions.
diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile index 6d0c2ee..9f7904f 100644 --- a/pc-bios/s390-ccw/Makefile +++ b/pc-bios/s390-ccw/Makefile @@ -9,7 +9,7 @@ $(call set-vpath, $(SRC_PATH)/pc-bios/s390-ccw) .PHONY : all clean build-all -OBJECTS = start.o main.o bootmap.o sclp.o virtio.o virtio-scsi.o virtio-blkdev.o +OBJECTS = start.o main.o bootmap.o sclp.o virtio.o virtio-scsi.o virtio-blkdev.o libc.o QEMU_CFLAGS := $(filter -W%, $(QEMU_CFLAGS)) QEMU_CFLAGS += -ffreestanding -fno-delete-null-pointer-checks -msoft-float QEMU_CFLAGS += -march=z900 -fPIE -fno-strict-aliasing diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c index b01e0f6..0da4c7f 100644 --- a/pc-bios/s390-ccw/bootmap.c +++ b/pc-bios/s390-ccw/bootmap.c @@ -494,7 +494,7 @@ static bool is_iso_bc_entry_compatible(IsoBcSection *s) "Failed to read image sector 0"); /* Checking bytes 8 - 32 for S390 Linux magic */ - return !_memcmp(magic_sec + 8, linux_s390_magic, 24); + return !memcmp(magic_sec + 8, linux_s390_magic, 24); } /* Location of the current sector of the directory */ @@ -623,7 +623,7 @@ static uint32_t find_iso_bc(void) if (vd->type == VOL_DESC_TYPE_BOOT) { IsoVdElTorito *et = &vd->vd.boot; - if (!_memcmp(&et->el_torito[0], el_torito_magic, 32)) { + if (!memcmp(&et->el_torito[0], el_torito_magic, 32)) { return bswap32(et->bc_offset); } } diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h index 460ec1a..a3a58f4 100644 --- a/pc-bios/s390-ccw/bootmap.h +++ b/pc-bios/s390-ccw/bootmap.h @@ -344,20 +344,6 @@ static inline bool magic_match(const void *data, const void *magic) return *((uint32_t *)data) == *((uint32_t *)magic); } -static inline int _memcmp(const void *s1, const void *s2, size_t n) -{ - int i; - const uint8_t *p1 = s1, *p2 = s2; - - for (i = 0; i < n; i++) { - if (p1[i] != p2[i]) { - return p1[i] > p2[i] ? 1 : -1; - } - } - - return 0; -} - static inline uint32_t iso_733_to_u32(uint64_t x) { return (uint32_t)x; @@ -450,7 +436,7 @@ const uint8_t vol_desc_magic[] = "CD001"; static inline bool is_iso_vd_valid(IsoVolDesc *vd) { - return !_memcmp(&vd->ident[0], vol_desc_magic, 5) && + return !memcmp(&vd->ident[0], vol_desc_magic, 5) && vd->version == 0x1 && vd->type <= VOL_DESC_TYPE_PARTITION; } diff --git a/pc-bios/s390-ccw/libc.c b/pc-bios/s390-ccw/libc.c new file mode 100644 index 0000000..fc8562a --- /dev/null +++ b/pc-bios/s390-ccw/libc.c @@ -0,0 +1,116 @@ +/* + * libc-style definitions and functions + * + * Copyright 2018 IBM Corp. + * Author(s): Collin L. Walling <walling@linux.vnet.ibm.com> + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + */ + +#include "libc.h" +#include "s390-ccw.h" + +/** + * atoi: + * @str: the string to be converted. + * + * Given a string @str, convert it to an integer. Leading whitespace is + * ignored. The first character (after any whitespace) is checked for the + * negative sign. Any other non-numerical value will terminate the + * conversion. + * + * Returns: an integer converted from the string @str. + */ +int atoi(const char *str) +{ + int val = 0; + int sign = 1; + + if (!str || !str[0]) { + return 0; + } + + while (*str == ' ') { + str++; + } + + if (*str == '-') { + sign = -1; + str++; + } + + while (*str) { + if (!isdigit(*str)) { + break; + } + val = val * 10 + *str - '0'; + str++; + } + + return val * sign; +} + +/** + * itostr: + * @num: an integer (base 10) to be converted. + * @str: a pointer to a string to store the conversion. + * @len: the length of the passed string. + * + * Given an integer @num, convert it to a string. The string @str must be + * allocated beforehand. The resulting string will be null terminated and + * returned. + * + * Returns: the string @str of the converted integer @num; NULL if @str + * is NULL or if there is not enough space allocated. + */ +static char *_itostr(int num, char *str, size_t len) +{ + int num_idx = 0; + int tmp = num; + char sign = 0; + + if (!str) { + return NULL; + } + + /* Get index to ones place */ + while ((tmp /= 10) != 0) { + num_idx++; + } + + if (num < 0) { + num *= -1; + sign = 1; + } + + /* Check if we have enough space for num, sign, and null */ + if (len <= num_idx + sign + 1) { + return NULL; + } + + str[num_idx + sign + 1] = '\0'; + + /* Convert int to string */ + while (num_idx >= 0) { + str[num_idx + sign] = num % 10 + '0'; + num /= 10; + num_idx--; + } + + if (sign) { + str[0] = '-'; + } + + return str; +} + +char *itostr(int num, char *str, size_t len) +{ + char *tmp = _itostr(num, str, len); + IPL_assert(str != NULL, "cannot convert NULL to int"); + IPL_assert(tmp != NULL, "array too small for integer to string conversion"); + return tmp; +} diff --git a/pc-bios/s390-ccw/libc.h b/pc-bios/s390-ccw/libc.h index 0142ea8..982c374 100644 --- a/pc-bios/s390-ccw/libc.h +++ b/pc-bios/s390-ccw/libc.h @@ -1,6 +1,8 @@ /* * libc-style definitions and functions * + * Copyright (c) 2013 Alexander Graf <agraf@suse.de> + * * This code is free software; you can redistribute it and/or modify it * under the terms of the GNU General Public License as published by the * Free Software Foundation; either version 2 of the License, or (at your @@ -33,7 +35,7 @@ static inline void *memcpy(void *s1, const void *s2, size_t n) { uint8_t *dest = s1; const uint8_t *src = s2; - int i; + size_t i; for (i = 0; i < n; i++) { dest[i] = src[i]; @@ -42,4 +44,35 @@ static inline void *memcpy(void *s1, const void *s2, size_t n) return s1; } +static inline int memcmp(const void *s1, const void *s2, size_t n) +{ + size_t i; + const uint8_t *p1 = s1, *p2 = s2; + + for (i = 0; i < n; i++) { + if (p1[i] != p2[i]) { + return p1[i] > p2[i] ? 1 : -1; + } + } + + return 0; +} + +static inline size_t strlen(const char *str) +{ + size_t i; + for (i = 0; *str; i++) { + str++; + } + return i; +} + +static inline int isdigit(int c) +{ + return (c >= '0') && (c <= '9'); +} + +int atoi(const char *str); +char *itostr(int num, char *str, size_t len); + #endif diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c index 401e9db..a8ef120 100644 --- a/pc-bios/s390-ccw/main.c +++ b/pc-bios/s390-ccw/main.c @@ -40,22 +40,7 @@ void panic(const char *string) unsigned int get_loadparm_index(void) { - const char *lp = loadparm; - int i; - unsigned int idx = 0; - - for (i = 0; i < 8; i++) { - char c = lp[i]; - - if (c < '0' || c > '9') { - break; - } - - idx *= 10; - idx += c - '0'; - } - - return idx; + return atoi(loadparm); } static bool find_dev(Schib *schib, int dev_no) diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c index b1fc8ff..486fce1 100644 --- a/pc-bios/s390-ccw/sclp.c +++ b/pc-bios/s390-ccw/sclp.c @@ -65,14 +65,6 @@ void sclp_setup(void) sclp_set_write_mask(); } -static int _strlen(const char *str) -{ - int i; - for (i = 0; *str; i++) - str++; - return i; -} - long write(int fd, const void *str, size_t len) { WriteEventData *sccb = (void *)_sccb; @@ -95,7 +87,7 @@ long write(int fd, const void *str, size_t len) void sclp_print(const char *str) { - write(1, str, _strlen(str)); + write(1, str, strlen(str)); } void sclp_get_loadparm_ascii(char *loadparm)