| Message ID | 1516034665-27606-2-git-send-email-walling@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/15/2018 10:44 AM, 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 <walling@linux.vnet.ibm.com> > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> > Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com> > --- > +++ b/pc-bios/s390-ccw/libc.c > @@ -0,0 +1,83 @@ > +/* > + * libc-style definitions and functions > + * > + * 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. I'm not a lawyer, but generically, the GPL and its variants depend on a copyright owner to actually work. You may want to add a copyright line. > + */ > + > +#include "libc.h" > +#include "s390-ccw.h" > + > +/** > + * atoi: > + * @str: the string to be converted. > + * > + * Given a string @str, convert it to an integer. Any non-numerical value > + * will terminate the conversion. > + * > + * Returns: an integer converted from the string @str. > + */ > +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'; Silently gives garbage on integer overflow, but matches the fact that POSIX atoi() can't flag errors. However, it does not handle leading whitespace nor '-', which means it is NOT doing a POSIX-compatible atoi() implementation; naming it atoi() is perhaps thus a disservice to end users. > + } > + > + return val; > +} > + > +/** > + * itostr: > + * @num: the integer 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. > + */ > +static char *_itostr(int num, char *str, size_t len) > +{ > + long num_len = 1; > + int tmp = num; > + int i; > + > + /* Count length of num */ > + while ((tmp /= 10) > 0) { > + num_len++; > + } > + > + /* Check if we have enough space for num and null */ > + if (len <= num_len) { > + return 0; > + } > + > + /* Convert int to string */ > + for (i = num_len - 1; i >= 0; i--) { > + str[i] = num % 10 + '0'; > + num /= 10; > + } No handling of negative integers? > + > + str[num_len] = '\0'; > + > + return str; > +} > + > +char *itostr(int num, char *str, size_t len) > +{ > + char *tmp = _itostr(num, str, len); > + IPL_assert(tmp != 0, "array too small for itostr conversion"); > + return tmp; > +} > diff --git a/pc-bios/s390-ccw/libc.h b/pc-bios/s390-ccw/libc.h > index 0142ea8..00268e3 100644 > --- a/pc-bios/s390-ccw/libc.h > +++ b/pc-bios/s390-ccw/libc.h > @@ -42,4 +42,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) > +{ > + int i; > + const uint8_t *p1 = s1, *p2 = s2; > + > + for (i = 0; i < n; i++) { Are you safe comparing int to size_t, or would it be safer to use size_t for your iterator? I guess this shim is unlikely to be abused by someone trying to compare 2G of memory at once. > + if (p1[i] != p2[i]) { > + return p1[i] > p2[i] ? 1 : -1; > + } > + } Not the fastest of implementations, but that probably doesn't matter (the complexity of writing a vectored implementation that works on a long or larger per loop iteration is important in a generic libc, but less so in a compatibility shim). > + > + return 0; > +} > + > +static inline size_t strlen(const char *str) > +{ > + size_t i; > + for (i = 0; *str; i++) { > + str++; > + } > + return i; Again, not the fastest implementation, but that shouldn't matter. > +} > + > +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
On 01/15/2018 12:05 PM, Eric Blake wrote: > On 01/15/2018 10:44 AM, 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 <walling@linux.vnet.ibm.com> >> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> >> Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com> >> --- >> +++ b/pc-bios/s390-ccw/libc.c >> @@ -0,0 +1,83 @@ >> +/* >> + * libc-style definitions and functions >> + * >> + * 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. > I'm not a lawyer, but generically, the GPL and its variants depend on a > copyright owner to actually work. You may want to add a copyright line. I'll have to check on that. I was not the original author of the libc.h file. > >> + */ >> + >> +#include "libc.h" >> +#include "s390-ccw.h" >> + >> +/** >> + * atoi: >> + * @str: the string to be converted. >> + * >> + * Given a string @str, convert it to an integer. Any non-numerical value >> + * will terminate the conversion. >> + * >> + * Returns: an integer converted from the string @str. >> + */ >> +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'; > Silently gives garbage on integer overflow, but matches the fact that > POSIX atoi() can't flag errors. However, it does not handle leading > whitespace nor '-', which means it is NOT doing a POSIX-compatible > atoi() implementation; naming it atoi() is perhaps thus a disservice to > end users. Fair enough. Perhaps the "strtoi" convention suits this better. > >> + } >> + >> + return val; >> +} >> + >> +/** >> + * itostr: >> + * @num: the integer 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. >> + */ >> +static char *_itostr(int num, char *str, size_t len) >> +{ >> + long num_len = 1; >> + int tmp = num; >> + int i; >> + >> + /* Count length of num */ >> + while ((tmp /= 10) > 0) { >> + num_len++; >> + } >> + >> + /* Check if we have enough space for num and null */ >> + if (len <= num_len) { >> + return 0; >> + } >> + >> + /* Convert int to string */ >> + for (i = num_len - 1; i >= 0; i--) { >> + str[i] = num % 10 + '0'; >> + num /= 10; >> + } > No handling of negative integers? It's not necessary for the purpose it serves. The functions that call itostr do some preprocessing to reject the negative sign and the string. We only care about positive numbers in the use cases. > >> + >> + str[num_len] = '\0'; >> + >> + return str; >> +} >> + >> +char *itostr(int num, char *str, size_t len) >> +{ >> + char *tmp = _itostr(num, str, len); >> + IPL_assert(tmp != 0, "array too small for itostr conversion"); >> + return tmp; >> +} >> diff --git a/pc-bios/s390-ccw/libc.h b/pc-bios/s390-ccw/libc.h >> index 0142ea8..00268e3 100644 >> --- a/pc-bios/s390-ccw/libc.h >> +++ b/pc-bios/s390-ccw/libc.h >> @@ -42,4 +42,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) >> +{ >> + int i; >> + const uint8_t *p1 = s1, *p2 = s2; >> + >> + for (i = 0; i < n; i++) { > Are you safe comparing int to size_t, or would it be safer to use size_t > for your iterator? I guess this shim is unlikely to be abused by > someone trying to compare 2G of memory at once. It probably makes more sense to declare i as size_t, at least for the sake of consistency. I'll do some clean up. > >> + if (p1[i] != p2[i]) { >> + return p1[i] > p2[i] ? 1 : -1; >> + } >> + } > Not the fastest of implementations, but that probably doesn't matter > (the complexity of writing a vectored implementation that works on a > long or larger per loop iteration is important in a generic libc, but > less so in a compatibility shim). > >> + >> + return 0; >> +} >> + >> +static inline size_t strlen(const char *str) >> +{ >> + size_t i; >> + for (i = 0; *str; i++) { >> + str++; >> + } >> + return i; > Again, not the fastest implementation, but that shouldn't matter. > >> +} >> + >> +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
On 15.01.2018 18:23, Collin L. Walling wrote: > On 01/15/2018 12:05 PM, Eric Blake wrote: >> On 01/15/2018 10:44 AM, Collin L. Walling wrote: [...] >>> +/** >>> + * atoi: >>> + * @str: the string to be converted. >>> + * >>> + * Given a string @str, convert it to an integer. Any non-numerical >>> value >>> + * will terminate the conversion. >>> + * >>> + * Returns: an integer converted from the string @str. >>> + */ >>> +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'; >> Silently gives garbage on integer overflow, but matches the fact that >> POSIX atoi() can't flag errors. However, it does not handle leading >> whitespace nor '-', which means it is NOT doing a POSIX-compatible >> atoi() implementation; naming it atoi() is perhaps thus a disservice to >> end users. > > Fair enough. Perhaps the "strtoi" convention suits this better. Or maybe simply add an assert(str[0] != '-') for now. If we ever hit the assert, we can still add the support for negative numbers if necessary. >>> +static inline size_t strlen(const char *str) >>> +{ >>> + size_t i; >>> + for (i = 0; *str; i++) { >>> + str++; >>> + } >>> + return i; >> Again, not the fastest implementation, but that shouldn't matter. Yes, indeed, speed does not really matter here for the some few bytes that are handled during the life-time of the s390-ccw bios. >>> +} >>> + >>> +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 > > Thomas
On 01/15/2018 06:23 PM, Collin L. Walling wrote: > On 01/15/2018 12:05 PM, Eric Blake wrote: >> On 01/15/2018 10:44 AM, 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 <walling@linux.vnet.ibm.com> >>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> >>> Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com> >>> --- >>> +++ b/pc-bios/s390-ccw/libc.c >>> @@ -0,0 +1,83 @@ >>> +/* >>> + * libc-style definitions and functions >>> + * >>> + * 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. >> I'm not a lawyer, but generically, the GPL and its variants depend on a >> copyright owner to actually work. You may want to add a copyright line. > > > I'll have to check on that. I was not the original author of the libc.h file. I think you can add the normal IBM Copyright statement. (I think all authors could add theirs as well)
On 01/16/2018 06:07 AM, Christian Borntraeger wrote: > > On 01/15/2018 06:23 PM, Collin L. Walling wrote: >> On 01/15/2018 12:05 PM, Eric Blake wrote: >>> On 01/15/2018 10:44 AM, 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 <walling@linux.vnet.ibm.com> >>>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> >>>> Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com> >>>> --- >>>> +++ b/pc-bios/s390-ccw/libc.c >>>> @@ -0,0 +1,83 @@ >>>> +/* >>>> + * libc-style definitions and functions >>>> + * >>>> + * 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. >>> I'm not a lawyer, but generically, the GPL and its variants depend on a >>> copyright owner to actually work. You may want to add a copyright line. >> >> I'll have to check on that. I was not the original author of the libc.h file. > I think you can add the normal IBM Copyright statement. (I think all authors could add theirs as well) > > "Copyright 2018 IBM Corp." @Thomas, git shows you as the initial creator of libc.h. Shall I add "Copyright 2018 Thomas Huth, Red Hat Inc." in there as well?
On 16.01.2018 16:32, Collin L. Walling wrote: > On 01/16/2018 06:07 AM, Christian Borntraeger wrote: >> >> On 01/15/2018 06:23 PM, Collin L. Walling wrote: >>> On 01/15/2018 12:05 PM, Eric Blake wrote: [...] >>>> I'm not a lawyer, but generically, the GPL and its variants depend on a >>>> copyright owner to actually work. You may want to add a copyright >>>> line. >>> >>> I'll have to check on that. I was not the original author of the >>> libc.h file. >> I think you can add the normal IBM Copyright statement. (I think all >> authors could add theirs as well) > > "Copyright 2018 IBM Corp." > > @Thomas, git shows you as the initial creator of libc.h. Shall I add > "Copyright 2018 Thomas Huth, Red Hat Inc." in there as well? Actually, I only moved the functions from s390-ccw.h and sclp.c into that header file. I just tried to trace who introduced them first, and it seems like it was Alex who added them. So I guess you should add: Copyright (c) 2013 Alexander Graf <agraf@suse.de> to libc.h for that. Sorry that I did not do that right from the start. Thomas
On 01/16/2018 05:00 AM, Thomas Huth wrote: > On 15.01.2018 18:23, Collin L. Walling wrote: >> On 01/15/2018 12:05 PM, Eric Blake wrote: >>> On 01/15/2018 10:44 AM, Collin L. Walling wrote: > [...] >>>> +/** >>>> + * atoi: >>>> + * @str: the string to be converted. >>>> + * >>>> + * Given a string @str, convert it to an integer. Any non-numerical >>>> value >>>> + * will terminate the conversion. >>>> + * >>>> + * Returns: an integer converted from the string @str. >>>> + */ >>>> +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'; >>> Silently gives garbage on integer overflow, but matches the fact that >>> POSIX atoi() can't flag errors. However, it does not handle leading >>> whitespace nor '-', which means it is NOT doing a POSIX-compatible >>> atoi() implementation; naming it atoi() is perhaps thus a disservice to >>> end users. >> Fair enough. Perhaps the "strtoi" convention suits this better. > Or maybe simply add an assert(str[0] != '-') for now. If we ever hit the > assert, we can still add the support for negative numbers if necessary. Eh... honestly it's easy enough to just add a flag for the negative sign and handle it at the end. We don't need it for this patch series, but at least the support will be there. :) [...]
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 67a6123..6f8e30f 100644 --- a/pc-bios/s390-ccw/bootmap.c +++ b/pc-bios/s390-ccw/bootmap.c @@ -512,7 +512,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 */ @@ -641,7 +641,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 cf99a4c..4980838 100644 --- a/pc-bios/s390-ccw/bootmap.h +++ b/pc-bios/s390-ccw/bootmap.h @@ -310,20 +310,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; @@ -416,7 +402,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..3ce0fb7 --- /dev/null +++ b/pc-bios/s390-ccw/libc.c @@ -0,0 +1,83 @@ +/* + * libc-style definitions and functions + * + * 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. Any non-numerical value + * will terminate the conversion. + * + * Returns: an integer converted from the string @str. + */ +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: + * @num: the integer 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. + */ +static char *_itostr(int num, char *str, size_t len) +{ + long num_len = 1; + int tmp = num; + int i; + + /* Count length of num */ + while ((tmp /= 10) > 0) { + num_len++; + } + + /* Check if we have enough space for num and null */ + if (len <= num_len) { + return 0; + } + + /* Convert int to string */ + for (i = num_len - 1; i >= 0; i--) { + str[i] = num % 10 + '0'; + num /= 10; + } + + str[num_len] = '\0'; + + return str; +} + +char *itostr(int num, char *str, size_t len) +{ + char *tmp = _itostr(num, str, len); + IPL_assert(tmp != 0, "array too small for itostr conversion"); + return tmp; +} diff --git a/pc-bios/s390-ccw/libc.h b/pc-bios/s390-ccw/libc.h index 0142ea8..00268e3 100644 --- a/pc-bios/s390-ccw/libc.h +++ b/pc-bios/s390-ccw/libc.h @@ -42,4 +42,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) +{ + 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 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)