| Message ID | 1518818879-18608-5-git-send-email-walling@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 16.02.2018 23:07, Collin L. Walling wrote: [...] > +/** > + * uitoa: > + * @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. This function only handles numbers between 0 and UINT64_MAX > + * inclusive. > + * > + * Returns: the string @str of the converted integer @num > + */ > +char *uitoa(uint64_t num, char *str, size_t len) > +{ > + size_t num_idx = 0; > + uint64_t tmp = num; > + > + IPL_assert(str != NULL, "uitoa: no space allocated to store string"); > + > + /* Get index to ones place */ > + while ((tmp /= 10) != 0) { > + num_idx++; > + } > + > + /* Check if we have enough space for num and null */ > + IPL_assert(len > num_idx, "uitoa: array too small for conversion"); Well, in v5 of this patch you've had "len >= num_idx + 1" where we agreed that it was wrong. Now you have "len > num_idx" which is pretty much the same. WTF? I still think you need "len > num_idx + 1" here to properly take the trailing NUL-byte into account properly. Please fix it! Thomas
On 02/17/2018 02:48 AM, Thomas Huth wrote: > On 16.02.2018 23:07, Collin L. Walling wrote: > [...] >> +/** >> + * uitoa: >> + * @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. This function only handles numbers between 0 and UINT64_MAX >> + * inclusive. >> + * >> + * Returns: the string @str of the converted integer @num >> + */ >> +char *uitoa(uint64_t num, char *str, size_t len) >> +{ >> + size_t num_idx = 0; >> + uint64_t tmp = num; >> + >> + IPL_assert(str != NULL, "uitoa: no space allocated to store string"); >> + >> + /* Get index to ones place */ >> + while ((tmp /= 10) != 0) { >> + num_idx++; >> + } >> + >> + /* Check if we have enough space for num and null */ >> + IPL_assert(len > num_idx, "uitoa: array too small for conversion"); > Well, in v5 of this patch you've had "len >= num_idx + 1" where we > agreed that it was wrong. Now you have "len > num_idx" which is pretty > much the same. WTF? > I still think you need "len > num_idx + 1" here to properly take the > trailing NUL-byte into account properly. Please fix it! > > Thomas > You're correct, and my apologies for not correcting the true problem here: I messed up the value of num_idx. It is off by one, but initializing the value to 1 instead of 0 should fix this. I must've accounted for this in my test file but forgot to update it in the actual source code. I think the assertion make more sense as len > num_idx, if num_idx were actually correct. Sorry for this mess...
On 19.02.2018 16:40, Collin L. Walling wrote: > On 02/17/2018 02:48 AM, Thomas Huth wrote: >> On 16.02.2018 23:07, Collin L. Walling wrote: >> [...] >>> +/** >>> + * uitoa: >>> + * @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. This function only handles numbers between 0 and >>> UINT64_MAX >>> + * inclusive. >>> + * >>> + * Returns: the string @str of the converted integer @num >>> + */ >>> +char *uitoa(uint64_t num, char *str, size_t len) >>> +{ >>> + size_t num_idx = 0; >>> + uint64_t tmp = num; >>> + >>> + IPL_assert(str != NULL, "uitoa: no space allocated to store >>> string"); >>> + >>> + /* Get index to ones place */ >>> + while ((tmp /= 10) != 0) { >>> + num_idx++; >>> + } >>> + >>> + /* Check if we have enough space for num and null */ >>> + IPL_assert(len > num_idx, "uitoa: array too small for conversion"); >> Well, in v5 of this patch you've had "len >= num_idx + 1" where we >> agreed that it was wrong. Now you have "len > num_idx" which is pretty >> much the same. WTF? >> I still think you need "len > num_idx + 1" here to properly take the >> trailing NUL-byte into account properly. Please fix it! >> >> Thomas >> > You're correct, and my apologies for not correcting the true problem here: > I messed up the value of num_idx. It is off by one, but initializing the > value to 1 instead of 0 should fix this. I must've accounted for this in > my test file but forgot to update it in the actual source code. Are you sure that initializing it to 1 is right? Unless you also change the final while loop in this function, this will put the character into the wrong location ("str[num_idx] = num % 10 + '0';"). Imagine a number that only consists of one digit ... the digit will be placed in str[1] which sounds wrong to me...? Thomas
On 02/19/2018 11:00 AM, Thomas Huth wrote: > On 19.02.2018 16:40, Collin L. Walling wrote: >> On 02/17/2018 02:48 AM, Thomas Huth wrote: >>> On 16.02.2018 23:07, Collin L. Walling wrote: >>> [...] >>>> +/** >>>> + * uitoa: >>>> + * @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. This function only handles numbers between 0 and >>>> UINT64_MAX >>>> + * inclusive. >>>> + * >>>> + * Returns: the string @str of the converted integer @num >>>> + */ >>>> +char *uitoa(uint64_t num, char *str, size_t len) >>>> +{ >>>> + size_t num_idx = 0; >>>> + uint64_t tmp = num; >>>> + >>>> + IPL_assert(str != NULL, "uitoa: no space allocated to store >>>> string"); >>>> + >>>> + /* Get index to ones place */ >>>> + while ((tmp /= 10) != 0) { >>>> + num_idx++; >>>> + } >>>> + >>>> + /* Check if we have enough space for num and null */ >>>> + IPL_assert(len > num_idx, "uitoa: array too small for conversion"); >>> Well, in v5 of this patch you've had "len >= num_idx + 1" where we >>> agreed that it was wrong. Now you have "len > num_idx" which is pretty >>> much the same. WTF? >>> I still think you need "len > num_idx + 1" here to properly take the >>> trailing NUL-byte into account properly. Please fix it! >>> >>> Thomas >>> >> You're correct, and my apologies for not correcting the true problem here: >> I messed up the value of num_idx. It is off by one, but initializing the >> value to 1 instead of 0 should fix this. I must've accounted for this in >> my test file but forgot to update it in the actual source code. > Are you sure that initializing it to 1 is right? Unless you also change > the final while loop in this function, this will put the character into > the wrong location ("str[num_idx] = num % 10 + '0';"). Imagine a number > that only consists of one digit ... the digit will be placed in str[1] > which sounds wrong to me...? > > Thomas > There's that, which we can solve by decrementing num_idx at the start of the loop. We also have to change the line str[num_idx + 1] = '\0'; to no longer add 1. It all boils down to "which way reads better", which I often struggle and bounce back-and-forth with (way) too much... Maybe I should also rename num_idx to just "idx" and let the comments explain everything?
On 02/19/2018 11:19 AM, Collin L. Walling wrote: > On 02/19/2018 11:00 AM, Thomas Huth wrote: >> On 19.02.2018 16:40, Collin L. Walling wrote: >>> On 02/17/2018 02:48 AM, Thomas Huth wrote: >>>> On 16.02.2018 23:07, Collin L. Walling wrote: >>>> [...] >>>>> +/** >>>>> + * uitoa: >>>>> + * @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. This function only handles numbers between 0 and >>>>> UINT64_MAX >>>>> + * inclusive. >>>>> + * >>>>> + * Returns: the string @str of the converted integer @num >>>>> + */ >>>>> +char *uitoa(uint64_t num, char *str, size_t len) >>>>> +{ >>>>> + size_t num_idx = 0; >>>>> + uint64_t tmp = num; >>>>> + >>>>> + IPL_assert(str != NULL, "uitoa: no space allocated to store >>>>> string"); >>>>> + >>>>> + /* Get index to ones place */ >>>>> + while ((tmp /= 10) != 0) { >>>>> + num_idx++; >>>>> + } >>>>> + >>>>> + /* Check if we have enough space for num and null */ >>>>> + IPL_assert(len > num_idx, "uitoa: array too small for >>>>> conversion"); >>>> Well, in v5 of this patch you've had "len >= num_idx + 1" where we >>>> agreed that it was wrong. Now you have "len > num_idx" which is pretty >>>> much the same. WTF? >>>> I still think you need "len > num_idx + 1" here to properly take the >>>> trailing NUL-byte into account properly. Please fix it! >>>> >>>> Thomas >>>> >>> You're correct, and my apologies for not correcting the true problem >>> here: >>> I messed up the value of num_idx. It is off by one, but >>> initializing the >>> value to 1 instead of 0 should fix this. I must've accounted for >>> this in >>> my test file but forgot to update it in the actual source code. >> Are you sure that initializing it to 1 is right? Unless you also change >> the final while loop in this function, this will put the character into >> the wrong location ("str[num_idx] = num % 10 + '0';"). Imagine a number >> that only consists of one digit ... the digit will be placed in str[1] >> which sounds wrong to me...? >> >> Thomas >> > There's that, which we can solve by decrementing num_idx at the start > of the loop. > We also have to change the line str[num_idx + 1] = '\0'; to no longer > add 1. > It all boils down to "which way reads better", which I often struggle and > bounce back-and-forth with (way) too much... > > Maybe I should also rename num_idx to just "idx" and let the comments > explain > everything? > How is this for a compromise? - start num_idx at 1, provide comment as for why - change while loop comment to explain we are "counting the _indices_ _of_ _num_" - str[num_idx] is assigned \0, and we also decrement num_idx in one line - in conversion loop, post decrement num_idx as it is used char *uitoa(int num, char *str, int len) { int num_idx = 1; /* account for NULL */ int tmp = num; assert(str != NULL, "uitoa: no space allocated to store string"); /* Count indices of num */ while ((tmp /= 10) != 0) num_idx++; /* Check if we have enough space for num and null */ assert(len > num_idx, "uitoa: array too small for conversion"); str[num_idx--] = '\0'; /* Convert int to string */ while (num_idx >= 0) { str[num_idx--] = num % 10 + '0'; num /= 10; } return str; }
On 02/19/2018 11:17 AM, Collin L. Walling wrote: > How is this for a compromise? > > - start num_idx at 1, provide comment as for why > - change while loop comment to explain we are "counting the > _indices_ _of_ _num_" > - str[num_idx] is assigned \0, and we also decrement num_idx in one > line > - in conversion loop, post decrement num_idx as it is used > > char *uitoa(int num, char *str, int len) > { > int num_idx = 1; /* account for NULL */ The single-byte character is named NUL (while NULL refers to the 8- or 4-byte pointer value). > int tmp = num; > > assert(str != NULL, "uitoa: no space allocated to store string"); > > /* Count indices of num */ > while ((tmp /= 10) != 0) > num_idx++; > > /* Check if we have enough space for num and null */ and again > assert(len > num_idx, "uitoa: array too small for conversion"); > > str[num_idx--] = '\0'; > > /* Convert int to string */ > while (num_idx >= 0) { > str[num_idx--] = num % 10 + '0'; > num /= 10; > } > > return str; Otherwise, it seems readable to me. > } > >
On 19.02.2018 18:17, Collin L. Walling wrote: > On 02/19/2018 11:19 AM, Collin L. Walling wrote: >> On 02/19/2018 11:00 AM, Thomas Huth wrote: >>> On 19.02.2018 16:40, Collin L. Walling wrote: >>>> On 02/17/2018 02:48 AM, Thomas Huth wrote: >>>>> On 16.02.2018 23:07, Collin L. Walling wrote: >>>>> [...] >>>>>> +/** >>>>>> + * uitoa: >>>>>> + * @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. This function only handles numbers between 0 and >>>>>> UINT64_MAX >>>>>> + * inclusive. >>>>>> + * >>>>>> + * Returns: the string @str of the converted integer @num >>>>>> + */ >>>>>> +char *uitoa(uint64_t num, char *str, size_t len) >>>>>> +{ >>>>>> + size_t num_idx = 0; >>>>>> + uint64_t tmp = num; >>>>>> + >>>>>> + IPL_assert(str != NULL, "uitoa: no space allocated to store >>>>>> string"); >>>>>> + >>>>>> + /* Get index to ones place */ >>>>>> + while ((tmp /= 10) != 0) { >>>>>> + num_idx++; >>>>>> + } >>>>>> + >>>>>> + /* Check if we have enough space for num and null */ >>>>>> + IPL_assert(len > num_idx, "uitoa: array too small for >>>>>> conversion"); >>>>> Well, in v5 of this patch you've had "len >= num_idx + 1" where we >>>>> agreed that it was wrong. Now you have "len > num_idx" which is pretty >>>>> much the same. WTF? >>>>> I still think you need "len > num_idx + 1" here to properly take the >>>>> trailing NUL-byte into account properly. Please fix it! >>>>> >>>>> Thomas >>>>> >>>> You're correct, and my apologies for not correcting the true problem >>>> here: >>>> I messed up the value of num_idx. It is off by one, but >>>> initializing the >>>> value to 1 instead of 0 should fix this. I must've accounted for >>>> this in >>>> my test file but forgot to update it in the actual source code. >>> Are you sure that initializing it to 1 is right? Unless you also change >>> the final while loop in this function, this will put the character into >>> the wrong location ("str[num_idx] = num % 10 + '0';"). Imagine a number >>> that only consists of one digit ... the digit will be placed in str[1] >>> which sounds wrong to me...? >>> >>> Thomas >>> >> There's that, which we can solve by decrementing num_idx at the start >> of the loop. >> We also have to change the line str[num_idx + 1] = '\0'; to no longer >> add 1. >> It all boils down to "which way reads better", which I often struggle and >> bounce back-and-forth with (way) too much... >> >> Maybe I should also rename num_idx to just "idx" and let the comments >> explain >> everything? >> > How is this for a compromise? > > - start num_idx at 1, provide comment as for why > - change while loop comment to explain we are "counting the > _indices_ _of_ _num_" > - str[num_idx] is assigned \0, and we also decrement num_idx in one > line > - in conversion loop, post decrement num_idx as it is used > > char *uitoa(int num, char *str, int len) > { > int num_idx = 1; /* account for NULL */ > int tmp = num; > > assert(str != NULL, "uitoa: no space allocated to store string"); > > /* Count indices of num */ > while ((tmp /= 10) != 0) > num_idx++; > > /* Check if we have enough space for num and null */ > assert(len > num_idx, "uitoa: array too small for conversion"); > > str[num_idx--] = '\0'; > > /* Convert int to string */ > while (num_idx >= 0) { > str[num_idx--] = num % 10 + '0'; > num /= 10; > } > > return str; > } Yes, looks fine to me that way (with the "NUL" change mentioned by Eric). Thomas
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 a94638d..092fb35 100644 --- a/pc-bios/s390-ccw/bootmap.c +++ b/pc-bios/s390-ccw/bootmap.c @@ -506,7 +506,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 */ @@ -635,7 +635,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 4bd95cd..4cf7e1e 100644 --- a/pc-bios/s390-ccw/bootmap.h +++ b/pc-bios/s390-ccw/bootmap.h @@ -328,20 +328,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; @@ -434,7 +420,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..a144388 --- /dev/null +++ b/pc-bios/s390-ccw/libc.c @@ -0,0 +1,89 @@ +/* + * 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" + +/** + * atoui: + * @str: the string to be converted. + * + * Given a string @str, convert it to an integer. Leading spaces are + * ignored. Any other non-numerical value will terminate the conversion + * and return 0. This function only handles numbers between 0 and + * UINT64_MAX inclusive. + * + * Returns: an integer converted from the string @str, or the number 0 + * if an error occurred. + */ +uint64_t atoui(const char *str) +{ + int val = 0; + + if (!str || !str[0]) { + return 0; + } + + while (*str == ' ') { + str++; + } + + while (*str) { + if (!isdigit(*str)) { + break; + } + val = val * 10 + *str - '0'; + str++; + } + + return val; +} + +/** + * uitoa: + * @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. This function only handles numbers between 0 and UINT64_MAX + * inclusive. + * + * Returns: the string @str of the converted integer @num + */ +char *uitoa(uint64_t num, char *str, size_t len) +{ + size_t num_idx = 0; + uint64_t tmp = num; + + IPL_assert(str != NULL, "uitoa: no space allocated to store string"); + + /* Get index to ones place */ + while ((tmp /= 10) != 0) { + num_idx++; + } + + /* Check if we have enough space for num and null */ + IPL_assert(len > num_idx, "uitoa: array too small for conversion"); + + str[num_idx + 1] = '\0'; + + /* Convert int to string */ + while (num_idx >= 0) { + str[num_idx] = num % 10 + '0'; + num /= 10; + num_idx--; + } + + return str; +} diff --git a/pc-bios/s390-ccw/libc.h b/pc-bios/s390-ccw/libc.h index 0142ea8..63ece70 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 @@ -19,7 +21,7 @@ typedef unsigned long long uint64_t; static inline void *memset(void *s, int c, size_t n) { - int i; + size_t i; unsigned char *p = s; for (i = 0; i < n; i++) { @@ -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'); +} + +uint64_t atoui(const char *str); +char *uitoa(uint64_t 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..e857ce4 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 atoui(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 90d1bc3..e6a0898 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; @@ -113,7 +105,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)