Re: [Qemu-devel] [PATCH 2/2] replay: Ignore the return value of fwrite()

[Felipe Franciosi]

> On 21 Sep 2016, at 19:17, Eric Blake <address@hidden> wrote:
> 
> On 09/21/2016 10:27 AM, Felipe Franciosi wrote:
>> If building with GCC 3.4 or newer (and using -Werror=unused-result),
>> replay-internal.c will fail to compile due to a call to fwrite() where
>> the return value is not used. Since fwrite() is declared with WUR in
>> glibc, callers should check the return value or find other ways to
>> ignore it. The error message in this specific case is:
>> 
>>    replay/replay-internal.c: In function ‘replay_put_array’:
>>    replay/replay-internal.c:68:15: error: ignoring return value of
>>    ‘fwrite’, declared with attribute warn_unused_result 
>> [-Werror=unused-result]
>>             fwrite(buf, 1, size, replay_file);
>>                   ^
>> 
>> This commit wraps the fwrite() call with the ignore_value() macro, which
>> currently suppresses the error for existing GCC versions.
> 
> This explains what you did, but not quite why.  In other words, convince
> me that ignoring the error is the right thing to do in the first place...

Sure, that's why I've linked these two in a patch series. :)

> 
>> 
>> Signed-off-by: Felipe Franciosi <address@hidden>
>> ---
>> replay/replay-internal.  | 0
>> replay/replay-internal.c | 2 +-
>> 2 files changed, 1 insertion(+), 1 deletion(-)
>> create mode 100644 replay/replay-internal.
>> 
>> diff --git a/replay/replay-internal. b/replay/replay-internal.
>> new file mode 100644
>> index 0000000..e69de29
>> diff --git a/replay/replay-internal.c b/replay/replay-internal.c
>> index 5835e8d..61de8f9 100644
>> --- a/replay/replay-internal.c
>> +++ b/replay/replay-internal.c
>> @@ -65,7 +65,7 @@ void replay_put_array(const uint8_t *buf, size_t size)
>> {
>>     if (replay_file) {
>>         replay_put_dword(size);
>> -        fwrite(buf, 1, size, replay_file);
>> +        ignore_value(fwrite(buf, 1, size, replay_file));
> 
> I would be more convinced that this patch is correct if you added a
> comment here why fwrite() failures can be ignored in this situation, so
> that someone doesn't undo your commit to add in proper error handling.

Right, so there are two things to be discussed here:
1) This patch merely fixes the build. It doesn't change the current behaviour.
2) We need to check whether fwrite() succeed.

The real question is: are we happy with 1? Or do we want to go down route 2?

Pavel already flagged that we should probably be checking whether fwrite() 
succeed. This is something I briefly mentioned in another e-mail [1], but then 
the direction got changed to use ignore_value() instead.
[1] https://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg05039.html

If we want to go down route 2, we should arguably revisit the whole replay 
implementation. For instance, other calls around "replay_file" (such as putc, 
fseek, etc) are all ignored today. Also worth noting, several checks while 
reading from "replay_file" result in an error_report(), but without any special 
handling. Maybe the underlying idea is that if you lost the ability to write to 
(or seek) a file stream, you have bigger problems to worry about and your host 
is catastrophically failing in another way.

Thoughts?

Felipe

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>