Re: [Qemu-devel] [PATCH] Fix gcc-4.6 compiler error

[Anthony Liguori <anthony@codemonkey.ws>]

On 07/29/2011 07:18 PM, Peter Maydell wrote:
> On 29 July 2011 20:30, Stefan Weil<weil@mail.berlios.de>  wrote:
>> Commit 3d3b8303c6f83b9b245bc774af530a6403cc4ce6
>> breaks builds with gcc-4.6:
>>
>> hw/fw_cfg.c: In function ‘probe_splashfile’:
>> hw/fw_cfg.c:66:9: error: variable ‘fop_ret’ set but not used [-Werror=unused-but-set-variable]
>> hw/fw_cfg.c: In function ‘fw_cfg_bootsplash’:
>> hw/fw_cfg.c:130:9: error: variable ‘fop_ret’ set but not used [-Werror=unused-but-set-variable]
>>
>> Remove fop_ret. Testing the result of fread() is normally
>> a good idea, but I don't think it is needed here.
>
>> @@ -86,7 +85,7 @@ static FILE *probe_splashfile(char *filename, int *file_sizep, int *file_typep)
>>      }
>>      /* check magic ID */
>>      fseek(fp, 0L, SEEK_SET);
>> -    fop_ret = fread(buf, 1, 2, fp);
>> +    (void)fread(buf, 1, 2, fp);
>
> Usually this kind of thing is added in order to stop gcc complaining about
> you ignoring the return value of a function which has been marked (by libc)
> as 'don't-ignore-return-value'. In such cases a "(void)" is not sufficient
> to suppress the "return value ignored" warning.
>
> At least some of these cases really should be checking fread return values;
> I see we also don't check fseek() return values either in all places. So
> the easiest fix is just to check all the fread() calls.
>
> Alternative suggestion: it would be easier to just slurp the whole file
> into memory (which is what we do once we've figured out it's an image)
> and then check the magic numbers in the memory buffer, which removes
> a lot of these unchecked function calls altogether. Since we're linking
> against glib anyway it looks like g_file_get_contents() would do 95%
> of the work for us. [disclaimer: I haven't used that API myself but it
> looks the right shape...g_malloc vs qemu_malloc issues, maybe?] Failing
> that, fopen/fstat/fread/fclose/check magic numbers.

As long as it's not mixed, it shouldn't be a problem.

I think using g_file_get_contents would make sense here.

Regards,

Anthony Liguori

>
> -- PMM
>
>