[Rpm-maint] [PATCH] build: fgetc returns int, not char.

[Panu Matilainen]

On 08/10/2016 11:54 PM, Richard W.M. Jones wrote:
> On Wed, Aug 10, 2016 at 04:45:16PM -0400, Neal Gompa wrote:
>> On Wed, Aug 10, 2016 at 10:09 AM, Richard W.M. Jones <rjones at redhat.com> wrote:
>>> Returning the value into a char is a mistake on all platforms, but is
>>> particularly bad on RISC-V.  On that platform (like ARM) char is
>>> unsigned.  Therefore EOF (-1) is returned as 255, and the subsequent
>>> test 'c == EOF' ('255 == -1') fails causing an infinite loop.
>>>
>>> Signed-off-by: Richard W.M. Jones <rjones at redhat.com>
>>> ---
>>>  build/parseSpec.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/build/parseSpec.c b/build/parseSpec.c
>>> index 85b0ba3..28f00bc 100644
>>> --- a/build/parseSpec.c
>>> +++ b/build/parseSpec.c
>>> @@ -323,7 +323,7 @@ retry:
>>>
>>>      /* Make sure we have something in the read buffer */
>>>      if (!(ofi->readPtr && *(ofi->readPtr))) {
>>> -       char c;
>>> +       int c;
>>>         int i = 0;
>>>
>>>         while((c = fgetc(ofi->fp)) != EOF) {
>>> --
>>> 2.7.4
>>>
>>
>> This patch looks good to me. Though, should we care about the size of
>> the integer?
>
> It's a good question.  I would go with the man page definition of
> fgetc which just declares it an int:
>
>   NAME
>        fgetc, fgets, getc, getchar, ungetc - input of characters and strings
>
>   SYNOPSIS
>        #include <stdio.h>
>
>        int fgetc(FILE *stream);
>   ...
>
> The important thing here is that it can cope with all characters and
> the special EOF value (-1).

Yup. The patch looks correct to me.

For the brave, try compiling rpm with -Wconversion. I'd expect many of 
the warnings to be "mostly harmless" for practical purposes but I'd also 
be surprised if this were the only concrete bug lurking in there.

	- Panu -

> Rich.
>