| Message ID | 1523629847-22369-1-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [for-2.13] pc-bios/s390-ccw: size_t should be unsigned | expand |
On 04/13/2018 11:30 AM, Thomas Huth wrote: > "size_t" should be an unsigned type - the signed counterpart is called > "ssize_t" in the C standard instead. Thus we should also use this > convention in the s390-ccw firmware to avoid confusion. I checked the > sources, and apart from one spot in libc.c (which now uses ssize_t with > this patch), the code should all be fine with this change. > > Buglink: https://bugs.launchpad.net/qemu/+bug/1753437 > Signed-off-by: Thomas Huth <thuth@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > pc-bios/s390-ccw/libc.c | 2 +- > pc-bios/s390-ccw/libc.h | 3 ++- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/pc-bios/s390-ccw/libc.c b/pc-bios/s390-ccw/libc.c > index 38ea77d..827d204 100644 > --- a/pc-bios/s390-ccw/libc.c > +++ b/pc-bios/s390-ccw/libc.c > @@ -63,7 +63,7 @@ uint64_t atoui(const char *str) > */ > char *uitoa(uint64_t num, char *str, size_t len) > { > - size_t num_idx = 1; /* account for NUL */ > + ssize_t num_idx = 1; /* account for NUL */ > uint64_t tmp = num; > > IPL_assert(str != NULL, "uitoa: no space allocated to store string"); > diff --git a/pc-bios/s390-ccw/libc.h b/pc-bios/s390-ccw/libc.h > index 63ece70..57c4199 100644 > --- a/pc-bios/s390-ccw/libc.h > +++ b/pc-bios/s390-ccw/libc.h > @@ -12,7 +12,8 @@ > #ifndef S390_CCW_LIBC_H > #define S390_CCW_LIBC_H > > -typedef long size_t; > +typedef unsigned long size_t; > +typedef signed long ssize_t; > typedef int bool; > typedef unsigned char uint8_t; > typedef unsigned short uint16_t; >
On 04/13/2018 10:30 AM, Thomas Huth wrote: > "size_t" should be an unsigned type - the signed counterpart is called > "ssize_t" in the C standard instead. Thus we should also use this > convention in the s390-ccw firmware to avoid confusion. I checked the > sources, and apart from one spot in libc.c (which now uses ssize_t with > this patch), the code should all be fine with this change. > > Buglink: https://bugs.launchpad.net/qemu/+bug/1753437 > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > pc-bios/s390-ccw/libc.c | 2 +- > pc-bios/s390-ccw/libc.h | 3 ++- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/pc-bios/s390-ccw/libc.c b/pc-bios/s390-ccw/libc.c > index 38ea77d..827d204 100644 > --- a/pc-bios/s390-ccw/libc.c > +++ b/pc-bios/s390-ccw/libc.c > @@ -63,7 +63,7 @@ uint64_t atoui(const char *str) > */ > char *uitoa(uint64_t num, char *str, size_t len) > { > - size_t num_idx = 1; /* account for NUL */ > + ssize_t num_idx = 1; /* account for NUL */ > uint64_t tmp = num; > > IPL_assert(str != NULL, "uitoa: no space allocated to store string"); > diff --git a/pc-bios/s390-ccw/libc.h b/pc-bios/s390-ccw/libc.h > index 63ece70..57c4199 100644 > --- a/pc-bios/s390-ccw/libc.h > +++ b/pc-bios/s390-ccw/libc.h > @@ -12,7 +12,8 @@ > #ifndef S390_CCW_LIBC_H > #define S390_CCW_LIBC_H > > -typedef long size_t; > +typedef unsigned long size_t; > +typedef signed long ssize_t; > typedef int bool; > typedef unsigned char uint8_t; > typedef unsigned short uint16_t; > Looks good to me as well. If another r-b is even necessary: Reviewed-by: Collin Walling <walling@linux.ibm.com>
On 04/13/2018 04:30 PM, Thomas Huth wrote: > "size_t" should be an unsigned type - the signed counterpart is called > "ssize_t" in the C standard instead. Thus we should also use this The first sentence sounds like ssize_t is too a type defined by some C standard. Is it or does ssize_t come form somewhere else? I find negative size a little difficult conceptually. > convention in the s390-ccw firmware to avoid confusion. I checked the > sources, and apart from one spot in libc.c (which now uses ssize_t with > this patch), the code should all be fine with this change. > > Buglink: https://bugs.launchpad.net/qemu/+bug/1753437 > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- This is certainly an improvement over the confusing signed size_t, so: Acked-by: Halil Pasic <pasic@linux.vnet.ibm.com> BTW The stuff behind the buglink is a bit misleading. The description states the problem as can't escape loop (IMHO) and the bug status say 'confirmed'. What actually happened is that it turned out the problem initially reported, was not existent. Yet the bug report helped us find another problem: confusing names. To complicate understanding even further, the comments on the bug only contain this realization hidden behind a link. It probably does not matter though.
On 13 April 2018 at 16:28, Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > > > On 04/13/2018 04:30 PM, Thomas Huth wrote: >> "size_t" should be an unsigned type - the signed counterpart is called >> "ssize_t" in the C standard instead. Thus we should also use this > > The first sentence sounds like ssize_t is too a type defined by some > C standard. Is it or does ssize_t come form somewhere else? I find negative > size a little difficult conceptually. I think ssize_t is from POSIX: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html "Used for a count of bytes or an error indication". thanks -- PMM
On 13.04.2018 17:28, Halil Pasic wrote: > > > On 04/13/2018 04:30 PM, Thomas Huth wrote: >> "size_t" should be an unsigned type - the signed counterpart is called >> "ssize_t" in the C standard instead. Thus we should also use this > > The first sentence sounds like ssize_t is too a type defined by some > C standard. Is it or does ssize_t come form somewhere else? Arrr, seems like ssize_t is rather coming from POSIX than from the C standard, thanks for the hint. I'll rephrase the first sentence to: "size_t" should be an unsigned type according to the C standard, and most libc implementations provide a signed counterpart called "ssize_t". OK? >> convention in the s390-ccw firmware to avoid confusion. I checked the >> sources, and apart from one spot in libc.c (which now uses ssize_t with >> this patch), the code should all be fine with this change. >> >> Buglink: https://bugs.launchpad.net/qemu/+bug/1753437 >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- > > This is certainly an improvement over the confusing signed size_t, so: > > Acked-by: Halil Pasic <pasic@linux.vnet.ibm.com> Thanks! > BTW The stuff behind the buglink is a bit misleading. The description > states the problem as can't escape loop (IMHO) and the bug > status say 'confirmed'. > > What actually happened is that it turned out the problem initially reported, > was not existent. Yet the bug report helped us find another problem: > confusing names. Ok, I've updated the bug title. > To complicate understanding even further, the comments on the bug > only contain this realization hidden behind a link. Oh well, yes, the bridge between the bugtracker and the mailing list really su...ffers from many problems. Normally replies should show up in the bug tracker as well, but in this case the bridge just failed. Thomas
On 04/13/2018 05:50 PM, Thomas Huth wrote: > On 13.04.2018 17:28, Halil Pasic wrote: >> >> On 04/13/2018 04:30 PM, Thomas Huth wrote: >>> "size_t" should be an unsigned type - the signed counterpart is called >>> "ssize_t" in the C standard instead. Thus we should also use this >> The first sentence sounds like ssize_t is too a type defined by some >> C standard. Is it or does ssize_t come form somewhere else? > Arrr, seems like ssize_t is rather coming from POSIX than from the C > standard, thanks for the hint. I'll rephrase the first sentence to: > > "size_t" should be an unsigned type according to the C standard, and > most libc implementations provide a signed counterpart called "ssize_t". > > OK? > This ssize_t seems to be an rather interesting type. For instance POSIX says """ size_t Used for sizes of objects. ssize_t Used for a count of bytes or an error indication. """ and """ The type ssize_t shall be capable of storing values at least in the range [-1, {SSIZE_MAX}]. """ And it does not mandate SSIZE_MIN in limits (but of course mandates SSIZE_MAX. I don't like this 'counterpart' word here, because AFAIU these don't have to be counterparts in any sense. That is SSIZE_MAX << SIZE_MAX is possible for example. I'm not sure about the every positive has a negative thing, but that's not important here. The code in question kind of uses both signed and unsigned size for the same (the string). We even have a signed to unsigned comparison which could result in warnings. I still think the change is OK in practice, but maybe avoiding introducing ssize_t (until we really need it) is a better course of action. I think uitoa can be easily rewritten so it does not need the ssize_t. How about that? Regards, Halil
On 04/13/2018 01:59 PM, Halil Pasic wrote: >>> On 04/13/2018 04:30 PM, Thomas Huth wrote: >>>> "size_t" should be an unsigned type - the signed counterpart is called >>>> "ssize_t" in the C standard instead. Thus we should also use this >>> The first sentence sounds like ssize_t is too a type defined by some >>> C standard. Is it or does ssize_t come form somewhere else? >> Arrr, seems like ssize_t is rather coming from POSIX than from the C >> standard, thanks for the hint. I'll rephrase the first sentence to: >> >> "size_t" should be an unsigned type according to the C standard, and >> most libc implementations provide a signed counterpart called "ssize_t". >> >> OK? >> > > This ssize_t seems to be an rather interesting type. For instance POSIX says > """ > size_t > Used for sizes of objects. > ssize_t > Used for a count of bytes or an error indication. > """ > and > """ > The type ssize_t shall be capable of storing values at least in the range [-1, {SSIZE_MAX}]. > """ > > And it does not mandate SSIZE_MIN in limits (but of course mandates SSIZE_MAX. > > I don't like this 'counterpart' word here, because AFAIU these don't have to > be counterparts in any sense. That is SSIZE_MAX << SIZE_MAX is possible for > example. I'm not sure about the every positive has a negative thing, but > that's not important here. > > The code in question kind of uses both signed and unsigned size for > the same (the string). We even have a signed to unsigned comparison which > could result in warnings. I still think the change is OK in practice, but > maybe avoiding introducing ssize_t (until we really need it) is a better > course of action. I think uitoa can be easily rewritten so it does not > need the ssize_t. > > How about that? This seems clever indeed.
On 04/13/2018 02:06 PM, Philippe Mathieu-Daudé wrote: > On 04/13/2018 01:59 PM, Halil Pasic wrote: >>>> On 04/13/2018 04:30 PM, Thomas Huth wrote: >>>>> "size_t" should be an unsigned type - the signed counterpart is called >>>>> "ssize_t" in the C standard instead. Thus we should also use this >>>> The first sentence sounds like ssize_t is too a type defined by some >>>> C standard. Is it or does ssize_t come form somewhere else? >>> Arrr, seems like ssize_t is rather coming from POSIX than from the C >>> standard, thanks for the hint. I'll rephrase the first sentence to: >>> >>> "size_t" should be an unsigned type according to the C standard, and >>> most libc implementations provide a signed counterpart called "ssize_t". >>> >>> OK? >>> >> >> This ssize_t seems to be an rather interesting type. For instance POSIX says >> """ >> size_t >> Used for sizes of objects. >> ssize_t >> Used for a count of bytes or an error indication. >> """ >> and >> """ >> The type ssize_t shall be capable of storing values at least in the range [-1, {SSIZE_MAX}]. >> """ >> >> And it does not mandate SSIZE_MIN in limits (but of course mandates SSIZE_MAX. >> >> I don't like this 'counterpart' word here, because AFAIU these don't have to >> be counterparts in any sense. That is SSIZE_MAX << SIZE_MAX is possible for >> example. I'm not sure about the every positive has a negative thing, but >> that's not important here. >> >> The code in question kind of uses both signed and unsigned size for >> the same (the string). We even have a signed to unsigned comparison which >> could result in warnings. I still think the change is OK in practice, but >> maybe avoiding introducing ssize_t (until we really need it) is a better >> course of action. I think uitoa can be easily rewritten so it does not >> need the ssize_t. >> >> How about that? > > This seems clever indeed. > This whole issue stems from my misuse of size_t in the first place. If it makes things easier, let's just make num_idx of type "signed long". After reading this discussion, I think it makes sense to drop ssize_t. No need to make it available for just one function unless there are strong claims to also use this type elsewhere in the pc-bios (I can't find any).
On Fri, 13 Apr 2018 14:09:55 -0400 Collin Walling <walling@linux.ibm.com> wrote: > On 04/13/2018 02:06 PM, Philippe Mathieu-Daudé wrote: > > On 04/13/2018 01:59 PM, Halil Pasic wrote: > >>>> On 04/13/2018 04:30 PM, Thomas Huth wrote: > >>>>> "size_t" should be an unsigned type - the signed counterpart is called > >>>>> "ssize_t" in the C standard instead. Thus we should also use this > >>>> The first sentence sounds like ssize_t is too a type defined by some > >>>> C standard. Is it or does ssize_t come form somewhere else? > >>> Arrr, seems like ssize_t is rather coming from POSIX than from the C > >>> standard, thanks for the hint. I'll rephrase the first sentence to: > >>> > >>> "size_t" should be an unsigned type according to the C standard, and > >>> most libc implementations provide a signed counterpart called "ssize_t". > >>> > >>> OK? > >>> > >> > >> This ssize_t seems to be an rather interesting type. For instance POSIX says > >> """ > >> size_t > >> Used for sizes of objects. > >> ssize_t > >> Used for a count of bytes or an error indication. > >> """ > >> and > >> """ > >> The type ssize_t shall be capable of storing values at least in the range [-1, {SSIZE_MAX}]. > >> """ > >> > >> And it does not mandate SSIZE_MIN in limits (but of course mandates SSIZE_MAX. > >> > >> I don't like this 'counterpart' word here, because AFAIU these don't have to > >> be counterparts in any sense. That is SSIZE_MAX << SIZE_MAX is possible for > >> example. I'm not sure about the every positive has a negative thing, but > >> that's not important here. > >> > >> The code in question kind of uses both signed and unsigned size for > >> the same (the string). We even have a signed to unsigned comparison which > >> could result in warnings. I still think the change is OK in practice, but > >> maybe avoiding introducing ssize_t (until we really need it) is a better > >> course of action. I think uitoa can be easily rewritten so it does not > >> need the ssize_t. > >> > >> How about that? > > > > This seems clever indeed. > > > > This whole issue stems from my misuse of size_t in the first place. If it makes > things easier, let's just make num_idx of type "signed long". > > After reading this discussion, I think it makes sense to drop ssize_t. No need > to make it available for just one function unless there are strong claims to > also use this type elsewhere in the pc-bios (I can't find any). > +1 to just using signed long. Let's not make this needlessly complicated.
On 04/13/2018 11:59 AM, Halil Pasic wrote: > This ssize_t seems to be an rather interesting type. For instance POSIX says > """ > size_t > Used for sizes of objects. > ssize_t > Used for a count of bytes or an error indication. > """ > and > """ > The type ssize_t shall be capable of storing values at least in the range [-1, {SSIZE_MAX}]. > """ > > And it does not mandate SSIZE_MIN in limits (but of course mandates SSIZE_MAX. I've tried to get POSIX to tighten things to require that 'size_t' and 'ssize_t' must have the same rank, so that you can sanely use printf("%zd",(ssize_t)val), but we are not there yet. > > I don't like this 'counterpart' word here, because AFAIU these don't have to > be counterparts in any sense. That is SSIZE_MAX << SIZE_MAX is possible for > example. I'm not sure about the every positive has a negative thing, but > that's not important here. Indeed, until the POSIX wording is tightened, it is technically possible (but a very poor quality of implementation, and none of qemu's compilation platforms fall in that category) that ssize_t has a different rank than size_t (whether or not they also have a different width).
diff --git a/pc-bios/s390-ccw/libc.c b/pc-bios/s390-ccw/libc.c index 38ea77d..827d204 100644 --- a/pc-bios/s390-ccw/libc.c +++ b/pc-bios/s390-ccw/libc.c @@ -63,7 +63,7 @@ uint64_t atoui(const char *str) */ char *uitoa(uint64_t num, char *str, size_t len) { - size_t num_idx = 1; /* account for NUL */ + ssize_t num_idx = 1; /* account for NUL */ uint64_t tmp = num; IPL_assert(str != NULL, "uitoa: no space allocated to store string"); diff --git a/pc-bios/s390-ccw/libc.h b/pc-bios/s390-ccw/libc.h index 63ece70..57c4199 100644 --- a/pc-bios/s390-ccw/libc.h +++ b/pc-bios/s390-ccw/libc.h @@ -12,7 +12,8 @@ #ifndef S390_CCW_LIBC_H #define S390_CCW_LIBC_H -typedef long size_t; +typedef unsigned long size_t; +typedef signed long ssize_t; typedef int bool; typedef unsigned char uint8_t; typedef unsigned short uint16_t;
"size_t" should be an unsigned type - the signed counterpart is called "ssize_t" in the C standard instead. Thus we should also use this convention in the s390-ccw firmware to avoid confusion. I checked the sources, and apart from one spot in libc.c (which now uses ssize_t with this patch), the code should all be fine with this change. Buglink: https://bugs.launchpad.net/qemu/+bug/1753437 Signed-off-by: Thomas Huth <thuth@redhat.com> --- pc-bios/s390-ccw/libc.c | 2 +- pc-bios/s390-ccw/libc.h | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-)