[Qemu-devel] [PATCH v4 0/4] strtosz() cleanups

[Qemu-devel] [PATCH v4 0/4] strtosz() cleanups

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Hi,

Here is an updated version of the strtosz() fixes that were discussed
earlier. Per discussing with Anthony on irc, he is ok with them going
in like this.

This is a respin to fix a patch conflict in the block tree, and it
pulls the two patch sets into one set of four patches instead. No
functional change.

Cheers,
Jes


Jes Sorensen (4):
  strtosz(): use unsigned char and switch to qemu_isspace()
  strtosz() use qemu_toupper() to simplify switch statement
  strtosz(): Fix name confusion in use of modf()
  strtosz(): Use suffix macros in switch() statement

 cutils.c |   28 ++++++++++++----------------
 1 files changed, 12 insertions(+), 16 deletions(-)

-- 
1.7.3.4

[Qemu-devel] [PATCH 1/4] strtosz(): use unsigned char and switch to qemu_isspace()

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

isspace() behavior is undefined for signed char.

Bug pointed out by Eric Blake, thanks!

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 cutils.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/cutils.c b/cutils.c
index 4d2e27c..a067cf4 100644
--- a/cutils.c
+++ b/cutils.c
@@ -294,7 +294,8 @@ int fcntl_setfl(int fd, int flag)
 int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
 {
     int64_t retval = -1;
-    char *endptr, c, d;
+    char *endptr;
+    unsigned char c, d;
     int mul_required = 0;
     double val, mul, integral, fraction;
 
@@ -314,7 +315,7 @@ int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
      */
     c = *endptr;
     d = c;
-    if (isspace(c) || c == '\0' || c == ',') {
+    if (qemu_isspace(c) || c == '\0' || c == ',') {
         c = 0;
         if (default_suffix) {
             d = default_suffix;
@@ -361,7 +362,7 @@ int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
      */
     if (c != 0) {
         endptr++;
-        if (!isspace(*endptr) && *endptr != ',' && *endptr != 0) {
+        if (!qemu_isspace(*endptr) && *endptr != ',' && *endptr != 0) {
             goto fail;
         }
     }
-- 
1.7.3.4

[Qemu-devel] [PATCH 2/4] strtosz() use qemu_toupper() to simplify switch statement

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 cutils.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/cutils.c b/cutils.c
index a067cf4..78d35e2 100644
--- a/cutils.c
+++ b/cutils.c
@@ -323,16 +323,14 @@ int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
             d = c;
         }
     }
-    switch (d) {
+    switch (qemu_toupper(d)) {
     case 'B':
-    case 'b':
         mul = 1;
         if (mul_required) {
             goto fail;
         }
         break;
     case 'K':
-    case 'k':
         mul = 1 << 10;
         break;
     case 0:
@@ -340,15 +338,12 @@ int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
             goto fail;
         }
     case 'M':
-    case 'm':
         mul = 1ULL << 20;
         break;
     case 'G':
-    case 'g':
         mul = 1ULL << 30;
         break;
     case 'T':
-    case 't':
         mul = 1ULL << 40;
         break;
     default:
-- 
1.7.3.4

[Qemu-devel] [PATCH 3/4] strtosz(): Fix name confusion in use of modf()

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 cutils.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cutils.c b/cutils.c
index 78d35e2..369a016 100644
--- a/cutils.c
+++ b/cutils.c
@@ -304,8 +304,8 @@ int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
     if (isnan(val) || endptr == nptr || errno != 0) {
         goto fail;
     }
-    integral = modf(val, &fraction);
-    if (integral != 0) {
+    fraction = modf(val, &integral);
+    if (fraction != 0) {
         mul_required = 1;
     }
     /*
-- 
1.7.3.4

[Qemu-devel] [PATCH 4/4] strtosz(): Use suffix macros in switch() statement

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 cutils.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/cutils.c b/cutils.c
index 369a016..8d562b2 100644
--- a/cutils.c
+++ b/cutils.c
@@ -324,26 +324,26 @@ int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
         }
     }
     switch (qemu_toupper(d)) {
-    case 'B':
+    case STRTOSZ_DEFSUFFIX_B:
         mul = 1;
         if (mul_required) {
             goto fail;
         }
         break;
-    case 'K':
+    case STRTOSZ_DEFSUFFIX_KB:
         mul = 1 << 10;
         break;
     case 0:
         if (mul_required) {
             goto fail;
         }
-    case 'M':
+    case STRTOSZ_DEFSUFFIX_MB:
         mul = 1ULL << 20;
         break;
-    case 'G':
+    case STRTOSZ_DEFSUFFIX_GB:
         mul = 1ULL << 30;
         break;
-    case 'T':
+    case STRTOSZ_DEFSUFFIX_TB:
         mul = 1ULL << 40;
         break;
     default:
-- 
1.7.3.4

[Qemu-devel] Re: [PATCH v4 0/4] strtosz() cleanups

[Kevin Wolf]

Am 24.01.2011 16:33, schrieb Jes.Sorensen@redhat.com:
> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> Hi,
> 
> Here is an updated version of the strtosz() fixes that were discussed
> earlier. Per discussing with Anthony on irc, he is ok with them going
> in like this.
> 
> This is a respin to fix a patch conflict in the block tree, and it
> pulls the two patch sets into one set of four patches instead. No
> functional change.
> 
> Cheers,
> Jes
> 
> 
> Jes Sorensen (4):
>   strtosz(): use unsigned char and switch to qemu_isspace()
>   strtosz() use qemu_toupper() to simplify switch statement
>   strtosz(): Fix name confusion in use of modf()
>   strtosz(): Use suffix macros in switch() statement
> 
>  cutils.c |   28 ++++++++++++----------------
>  1 files changed, 12 insertions(+), 16 deletions(-)
> 

Thanks, applied all to the block branch.

Kevin

Re: [Qemu-devel] [PATCH 4/4] strtosz(): Use suffix macros in switch() statement

[Markus Armbruster]

Jes.Sorensen@redhat.com writes:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  cutils.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/cutils.c b/cutils.c
> index 369a016..8d562b2 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -324,26 +324,26 @@ int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
>          }
>      }
>      switch (qemu_toupper(d)) {
> -    case 'B':
> +    case STRTOSZ_DEFSUFFIX_B:
>          mul = 1;
>          if (mul_required) {
>              goto fail;
>          }
>          break;
> -    case 'K':
> +    case STRTOSZ_DEFSUFFIX_KB:
>          mul = 1 << 10;
>          break;
>      case 0:
>          if (mul_required) {
>              goto fail;
>          }
> -    case 'M':
> +    case STRTOSZ_DEFSUFFIX_MB:
>          mul = 1ULL << 20;
>          break;
> -    case 'G':
> +    case STRTOSZ_DEFSUFFIX_GB:
>          mul = 1ULL << 30;
>          break;
> -    case 'T':
> +    case STRTOSZ_DEFSUFFIX_TB:
>          mul = 1ULL << 40;
>          break;
>      default:

Phony abstraction.  And it leaks: code here assumes the
STRTOSZ_DEFSUFFIX_T* are all upper case.

Re: [Qemu-devel] [PATCH 4/4] strtosz(): Use suffix macros in switch() statement

[Jes Sorensen]

On 01/24/11 17:08, Markus Armbruster wrote:
> Jes.Sorensen@redhat.com writes:
> 
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>> ---
>>  cutils.c |   10 +++++-----
>>  1 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/cutils.c b/cutils.c
>> index 369a016..8d562b2 100644
>> --- a/cutils.c
>> +++ b/cutils.c
>> @@ -324,26 +324,26 @@ int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
>>          }
>>      }
>>      switch (qemu_toupper(d)) {
>> -    case 'B':
>> +    case STRTOSZ_DEFSUFFIX_B:
>>          mul = 1;
>>          if (mul_required) {
>>              goto fail;
>>          }
>>          break;
>> -    case 'K':
>> +    case STRTOSZ_DEFSUFFIX_KB:
>>          mul = 1 << 10;
>>          break;
>>      case 0:
>>          if (mul_required) {
>>              goto fail;
>>          }
>> -    case 'M':
>> +    case STRTOSZ_DEFSUFFIX_MB:
>>          mul = 1ULL << 20;
>>          break;
>> -    case 'G':
>> +    case STRTOSZ_DEFSUFFIX_GB:
>>          mul = 1ULL << 30;
>>          break;
>> -    case 'T':
>> +    case STRTOSZ_DEFSUFFIX_TB:
>>          mul = 1ULL << 40;
>>          break;
>>      default:
> 
> Phony abstraction.  And it leaks: code here assumes the
> STRTOSZ_DEFSUFFIX_T* are all upper case.

qemu_toupper() - whats the problem?

Jes

Re: [Qemu-devel] [PATCH 1/4] strtosz(): use unsigned char and switch to qemu_isspace()

[Markus Armbruster]

Jes.Sorensen@redhat.com writes:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> isspace() behavior is undefined for signed char.
>
> Bug pointed out by Eric Blake, thanks!
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  cutils.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/cutils.c b/cutils.c
> index 4d2e27c..a067cf4 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -294,7 +294,8 @@ int fcntl_setfl(int fd, int flag)
>  int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
>  {
>      int64_t retval = -1;
> -    char *endptr, c, d;
> +    char *endptr;
> +    unsigned char c, d;
>      int mul_required = 0;
>      double val, mul, integral, fraction;
>  

I doubt this hunk is still needed.

> @@ -314,7 +315,7 @@ int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
>       */
>      c = *endptr;
>      d = c;
> -    if (isspace(c) || c == '\0' || c == ',') {
> +    if (qemu_isspace(c) || c == '\0' || c == ',') {
>          c = 0;
>          if (default_suffix) {
>              d = default_suffix;
> @@ -361,7 +362,7 @@ int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
>       */
>      if (c != 0) {
>          endptr++;
> -        if (!isspace(*endptr) && *endptr != ',' && *endptr != 0) {
> +        if (!qemu_isspace(*endptr) && *endptr != ',' && *endptr != 0) {
>              goto fail;
>          }
>      }

Re: [Qemu-devel] [PATCH 1/4] strtosz(): use unsigned char and switch to qemu_isspace()

[Jes Sorensen]

On 01/24/11 17:10, Markus Armbruster wrote:
> Jes.Sorensen@redhat.com writes:
> 
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> isspace() behavior is undefined for signed char.
>>
>> Bug pointed out by Eric Blake, thanks!
>>
>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>> ---
>>  cutils.c |    7 ++++---
>>  1 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/cutils.c b/cutils.c
>> index 4d2e27c..a067cf4 100644
>> --- a/cutils.c
>> +++ b/cutils.c
>> @@ -294,7 +294,8 @@ int fcntl_setfl(int fd, int flag)
>>  int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
>>  {
>>      int64_t retval = -1;
>> -    char *endptr, c, d;
>> +    char *endptr;
>> +    unsigned char c, d;
>>      int mul_required = 0;
>>      double val, mul, integral, fraction;
>>  
> 
> I doubt this hunk is still needed.

It isn't strictly due to qemu_toupper() but it is prettier to match the
real behavior of pure toupper() anyway.

Jes

Re: [Qemu-devel] [PATCH 4/4] strtosz(): Use suffix macros in switch() statement

[Markus Armbruster]

Jes Sorensen <Jes.Sorensen@redhat.com> writes:

> On 01/24/11 17:08, Markus Armbruster wrote:
>> Jes.Sorensen@redhat.com writes:
>> 
>>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>
>>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>>> ---
>>>  cutils.c |   10 +++++-----
>>>  1 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/cutils.c b/cutils.c
>>> index 369a016..8d562b2 100644
>>> --- a/cutils.c
>>> +++ b/cutils.c
>>> @@ -324,26 +324,26 @@ int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
>>>          }
>>>      }
>>>      switch (qemu_toupper(d)) {
>>> -    case 'B':
>>> +    case STRTOSZ_DEFSUFFIX_B:
>>>          mul = 1;
>>>          if (mul_required) {
>>>              goto fail;
>>>          }
>>>          break;
>>> -    case 'K':
>>> +    case STRTOSZ_DEFSUFFIX_KB:
>>>          mul = 1 << 10;
>>>          break;
>>>      case 0:
>>>          if (mul_required) {
>>>              goto fail;
>>>          }
>>> -    case 'M':
>>> +    case STRTOSZ_DEFSUFFIX_MB:
>>>          mul = 1ULL << 20;
>>>          break;
>>> -    case 'G':
>>> +    case STRTOSZ_DEFSUFFIX_GB:
>>>          mul = 1ULL << 30;
>>>          break;
>>> -    case 'T':
>>> +    case STRTOSZ_DEFSUFFIX_TB:
>>>          mul = 1ULL << 40;
>>>          break;
>>>      default:
>> 
>> Phony abstraction.  And it leaks: code here assumes the
>> STRTOSZ_DEFSUFFIX_T* are all upper case.
>
> qemu_toupper() - whats the problem?

If a STRTOSZ_DEFSUFFIX_T? expands to a lower case character, its case
will not match any input.

Re: [Qemu-devel] [PATCH 1/4] strtosz(): use unsigned char and switch to qemu_isspace()

[Markus Armbruster]

Jes Sorensen <Jes.Sorensen@redhat.com> writes:

> On 01/24/11 17:10, Markus Armbruster wrote:
>> Jes.Sorensen@redhat.com writes:
>> 
>>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>
>>> isspace() behavior is undefined for signed char.
>>>
>>> Bug pointed out by Eric Blake, thanks!
>>>
>>> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>>> ---
>>>  cutils.c |    7 ++++---
>>>  1 files changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/cutils.c b/cutils.c
>>> index 4d2e27c..a067cf4 100644
>>> --- a/cutils.c
>>> +++ b/cutils.c
>>> @@ -294,7 +294,8 @@ int fcntl_setfl(int fd, int flag)
>>>  int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
>>>  {
>>>      int64_t retval = -1;
>>> -    char *endptr, c, d;
>>> +    char *endptr;
>>> +    unsigned char c, d;
>>>      int mul_required = 0;
>>>      double val, mul, integral, fraction;
>>>  
>> 
>> I doubt this hunk is still needed.
>
> It isn't strictly due to qemu_toupper() but it is prettier to match the
> real behavior of pure toupper() anyway.

Frequent casting between signed and unsigned has been shown to cause
headaches in laboratory rats.

Re: [Qemu-devel] [PATCH 4/4] strtosz(): Use suffix macros in switch() statement

[Jes Sorensen]

On 01/24/11 17:39, Markus Armbruster wrote:
>>>> +    case STRTOSZ_DEFSUFFIX_TB:
>>>> >>>          mul = 1ULL << 40;
>>>> >>>          break;
>>>> >>>      default:
>>> >> 
>>> >> Phony abstraction.  And it leaks: code here assumes the
>>> >> STRTOSZ_DEFSUFFIX_T* are all upper case.
>> >
>> > qemu_toupper() - whats the problem?
> If a STRTOSZ_DEFSUFFIX_T? expands to a lower case character, its case
> will not match any input.

Right, so one has to be careful when adding new suffix constants.
However given that we already have all the likely to be used ones for
the near future, that isn't exactly a big issue.

On the other hand forcing the use of the macros makes it less likely
that someone specifies an unsupported constant by hitting 'y' instead of
't' or similar.

Jes

Re: [Qemu-devel] Re: [PATCH v4 0/4] strtosz() cleanups

[Stefan Weil]

Am 24.01.2011 16:45, schrieb Kevin Wolf:
> Am 24.01.2011 16:33, schrieb Jes.Sorensen@redhat.com:
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> Hi,
>>
>> Here is an updated version of the strtosz() fixes that were discussed
>> earlier. Per discussing with Anthony on irc, he is ok with them going
>> in like this.
>>
>> This is a respin to fix a patch conflict in the block tree, and it
>> pulls the two patch sets into one set of four patches instead. No
>> functional change.
>>
>> Cheers,
>> Jes
>>
>>
>> Jes Sorensen (4):
>> strtosz(): use unsigned char and switch to qemu_isspace()
>> strtosz() use qemu_toupper() to simplify switch statement
>> strtosz(): Fix name confusion in use of modf()
>> strtosz(): Use suffix macros in switch() statement
>>
>> cutils.c | 28 ++++++++++++----------------
>> 1 files changed, 12 insertions(+), 16 deletions(-)
>>
>
> Thanks, applied all to the block branch.
>
> Kevin

There was some discussion regarding this patch set.
I agree with Markus that part of the first patch
should be removed: don't change char to unsigned char.

It's not necessary, and the result is, that now unsigned chars
are assigned to chars which might raise future compiler
warnings.

Stefan

Re: [Qemu-devel] [PATCH 4/4] strtosz(): Use suffix macros in switch() statement

[Markus Armbruster]

Jes Sorensen <Jes.Sorensen@redhat.com> writes:

> On 01/24/11 17:39, Markus Armbruster wrote:
>>>>> +    case STRTOSZ_DEFSUFFIX_TB:
>>>>> >>>          mul = 1ULL << 40;
>>>>> >>>          break;
>>>>> >>>      default:
>>>> >> 
>>>> >> Phony abstraction.  And it leaks: code here assumes the
>>>> >> STRTOSZ_DEFSUFFIX_T* are all upper case.
>>> >
>>> > qemu_toupper() - whats the problem?
>> If a STRTOSZ_DEFSUFFIX_T? expands to a lower case character, its case
>> will not match any input.
>
> Right, so one has to be careful when adding new suffix constants.

Calls for a comment right next to the definition of the
STRTOSZ_DEFSUFFIX_T*.

I hate unstated restrictions that are hidden far away from the place
where you can break them.

> However given that we already have all the likely to be used ones for
> the near future, that isn't exactly a big issue.
>
> On the other hand forcing the use of the macros makes it less likely
> that someone specifies an unsupported constant by hitting 'y' instead of
> 't' or similar.

Takes a combination of butterfingers, cross-eyedness, and near-total
incompetence at basic smoke-testing.

Re: [Qemu-devel] Re: [PATCH v4 0/4] strtosz() cleanups

[Jes Sorensen]

On 01/24/11 18:28, Stefan Weil wrote:
> There was some discussion regarding this patch set.
> I agree with Markus that part of the first patch
> should be removed: don't change char to unsigned char.

The unsigned char should definitely go in, leaving it as a signed char
doesn't serve any purpose.

> It's not necessary, and the result is, that now unsigned chars
> are assigned to chars which might raise future compiler
> warnings.

We already do that cast by calling qemu_toupper() and qemu_isspace() so
that argument is void.

Jes

Re: [Qemu-devel] [PATCH 4/4] strtosz(): Use suffix macros in switch() statement

[Jes Sorensen]

On 01/24/11 18:47, Markus Armbruster wrote:
> Jes Sorensen <Jes.Sorensen@redhat.com> writes:
>>>>> qemu_toupper() - whats the problem?
>>> If a STRTOSZ_DEFSUFFIX_T? expands to a lower case character, its case
>>> will not match any input.
>>
>> Right, so one has to be careful when adding new suffix constants.
> 
> Calls for a comment right next to the definition of the
> STRTOSZ_DEFSUFFIX_T*.
> 
> I hate unstated restrictions that are hidden far away from the place
> where you can break them.

Well I am fine with a comment in the code.

>> However given that we already have all the likely to be used ones for
>> the near future, that isn't exactly a big issue.
>>
>> On the other hand forcing the use of the macros makes it less likely
>> that someone specifies an unsupported constant by hitting 'y' instead of
>> 't' or similar.
> 
> Takes a combination of butterfingers, cross-eyedness, and near-total
> incompetence at basic smoke-testing.

Not really, all it takes is someone writing a piece of code, not
thinking about it, therefore only testing things where a suffix is
specified as an argument and it gets missed.

Jes

Re: [Qemu-devel] Re: [PATCH v4 0/4] strtosz() cleanups

[Markus Armbruster]

Jes Sorensen <Jes.Sorensen@redhat.com> writes:

> On 01/24/11 18:28, Stefan Weil wrote:
>> There was some discussion regarding this patch set.
>> I agree with Markus that part of the first patch
>> should be removed: don't change char to unsigned char.
>
> The unsigned char should definitely go in, leaving it as a signed char
> doesn't serve any purpose.

Leaving something as is doesn't need justification.  Changing it does.
The justification presented so far was "it is prettier to match the real
behavior of pure toupper()".  Which I don't buy.  But without commit
access, I'm not a buyer.

[...]

Re: [Qemu-devel] [PATCH 4/4] strtosz(): Use suffix macros in switch() statement

[Markus Armbruster]

Jes Sorensen <Jes.Sorensen@redhat.com> writes:

> On 01/24/11 18:47, Markus Armbruster wrote:
>> Jes Sorensen <Jes.Sorensen@redhat.com> writes:
>>>>>> qemu_toupper() - whats the problem?
>>>> If a STRTOSZ_DEFSUFFIX_T? expands to a lower case character, its case
>>>> will not match any input.
>>>
>>> Right, so one has to be careful when adding new suffix constants.
>> 
>> Calls for a comment right next to the definition of the
>> STRTOSZ_DEFSUFFIX_T*.
>> 
>> I hate unstated restrictions that are hidden far away from the place
>> where you can break them.
>
> Well I am fine with a comment in the code.

Such a comment improves it from "wrong" to merely "ugly".  I can live
with that.

Thanks.

[...]

Re: [Qemu-devel] Re: [PATCH v4 0/4] strtosz() cleanups

[Jes Sorensen]

On 01/25/11 11:14, Markus Armbruster wrote:
> Jes Sorensen <Jes.Sorensen@redhat.com> writes:
> 
>> On 01/24/11 18:28, Stefan Weil wrote:
>>> There was some discussion regarding this patch set.
>>> I agree with Markus that part of the first patch
>>> should be removed: don't change char to unsigned char.
>>
>> The unsigned char should definitely go in, leaving it as a signed char
>> doesn't serve any purpose.
> 
> Leaving something as is doesn't need justification.  Changing it does.
> The justification presented so far was "it is prettier to match the real
> behavior of pure toupper()".  Which I don't buy.  But without commit
> access, I'm not a buyer.

Well that is just too bad. qemu_toupper() is a hack around the fact that
people often forget to use the right type, it is not an excuse for using
the wrong type in the code.

Jes

Re: [Qemu-devel] [PATCH 4/4] strtosz(): Use suffix macros in switch() statement

[Jes Sorensen]

On 01/25/11 11:17, Markus Armbruster wrote:
> Jes Sorensen <Jes.Sorensen@redhat.com> writes:
> 
>> On 01/24/11 18:47, Markus Armbruster wrote:
>>> Jes Sorensen <Jes.Sorensen@redhat.com> writes:
>>>>>>> qemu_toupper() - whats the problem?
>>>>> If a STRTOSZ_DEFSUFFIX_T? expands to a lower case character, its case
>>>>> will not match any input.
>>>>
>>>> Right, so one has to be careful when adding new suffix constants.
>>>
>>> Calls for a comment right next to the definition of the
>>> STRTOSZ_DEFSUFFIX_T*.
>>>
>>> I hate unstated restrictions that are hidden far away from the place
>>> where you can break them.
>>
>> Well I am fine with a comment in the code.
> 
> Such a comment improves it from "wrong" to merely "ugly".  I can live
> with that.

I realize that you view it as such, in other eyes it goes from hackish
to correct ... there is zero point in arguing over this, we can just
agree to disagree.

Jes

Re: [Qemu-devel] Re: [PATCH v4 0/4] strtosz() cleanups

[Markus Armbruster]

Jes Sorensen <Jes.Sorensen@redhat.com> writes:

> On 01/25/11 11:14, Markus Armbruster wrote:
>> Jes Sorensen <Jes.Sorensen@redhat.com> writes:
>> 
>>> On 01/24/11 18:28, Stefan Weil wrote:
>>>> There was some discussion regarding this patch set.
>>>> I agree with Markus that part of the first patch
>>>> should be removed: don't change char to unsigned char.
>>>
>>> The unsigned char should definitely go in, leaving it as a signed char
>>> doesn't serve any purpose.
>> 
>> Leaving something as is doesn't need justification.  Changing it does.
>> The justification presented so far was "it is prettier to match the real
>> behavior of pure toupper()".  Which I don't buy.  But without commit
>> access, I'm not a buyer.
>
> Well that is just too bad. qemu_toupper() is a hack around the fact that
> people often forget to use the right type, it is not an excuse for using
> the wrong type in the code.

qemu_toupper() is designed to work correctly for any character argument,
be it signed or unsigned.

The fact that toupper() works only for unsigned character values is
irrelevant for qemu_toupper().

The fact that qemu_toupper()'s implementation uses toupper() is an
implementation detail.