> On 21 Sep 2016, at 19:17, Eric Blake 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
>> ---
>> 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 000..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
>
