[Qemu-devel] [PATCH for-2.13] pc-bios/s390-ccw: size_t should be unsigned

[Qemu-devel] [PATCH for-2.13] pc-bios/s390-ccw: size_t should be unsigned

[Thomas Huth]

"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;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH for-2.13] pc-bios/s390-ccw: size_t should be unsigned

[Philippe Mathieu-Daudé]

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;
> 

Re: [Qemu-devel] [PATCH for-2.13] pc-bios/s390-ccw: size_t should be unsigned

[Collin Walling]

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>

-- 
Respectfully,
- Collin Walling

Re: [Qemu-devel] [PATCH for-2.13] pc-bios/s390-ccw: size_t should be unsigned

[Halil Pasic]

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.

Re: [Qemu-devel] [PATCH for-2.13] pc-bios/s390-ccw: size_t should be unsigned

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH for-2.13] pc-bios/s390-ccw: size_t should be unsigned

[Thomas Huth]

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

Re: [Qemu-devel] [PATCH for-2.13] pc-bios/s390-ccw: size_t should be unsigned

[Halil Pasic]

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

Re: [Qemu-devel] [PATCH for-2.13] pc-bios/s390-ccw: size_t should be unsigned

[Philippe Mathieu-Daudé]

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.

Re: [Qemu-devel] [PATCH for-2.13] pc-bios/s390-ccw: size_t should be unsigned

[Collin Walling]

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).

-- 
Respectfully,
- Collin Walling

Re: [Qemu-devel] [PATCH for-2.13] pc-bios/s390-ccw: size_t should be unsigned

[Cornelia Huck]

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.

Re: [Qemu-devel] [PATCH for-2.13] pc-bios/s390-ccw: size_t should be unsigned

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1335 bytes --]

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).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]