qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] replay: Fix build with -Werror=unused-result


From: Felipe Franciosi
Subject: Re: [Qemu-devel] [PATCH] replay: Fix build with -Werror=unused-result
Date: Wed, 21 Sep 2016 10:00:23 +0000

> On 21 Sep 2016, at 07:24, Markus Armbruster <address@hidden> wrote:
> 
> "Pavel Dovgalyuk" <address@hidden> writes:
> 
>>> From: Felipe Franciosi [mailto:address@hidden
>>> If compiling with -Werror=unused-result, replay-internal.c won't build
>>> due to a call to fwrite() where the returned value is ignored. A simple
>>> cast to (void) is not sufficient on recent GCCs, so this fixes it.
>>> 
>>> Signed-off-by: Felipe Franciosi <address@hidden>
>>> ---
>>> replay/replay-internal.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/replay/replay-internal.c b/replay/replay-internal.c
>>> index 5835e8d..6978d76 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);
>>> +        (void)(fwrite(buf, 1, size, replay_file)+1);
>>>     }
>>> }
>> 
>> This looks very weird.

Oh I couldn't agree more. I hate this syntax. It is, however, the simplest way 
to get around this issue. See:
http://stackoverflow.com/questions/11888594/ignoring-return-values-in-c

That doesn't mean we should do it. I actually rather we didn't. :)
I sent this patch to discuss how the maintainers feel about it given no one 
bumped into this yet (apparently).

>> I think it would be better to check the return value and stop
>> the simulation in case of error.

We can also make replay_put_array() return an int indicating whether an error 
occurred. The callers can then decide whether to care or not. I'll send a v2 
doing that instead for comments.

> Ignoring errors is wrong more often than not.  Whether it's wrong in
> this case I can't say.

True that. That's why I think a better first step is to make functions that can 
fail return an error code. Callers can make that decision later. Unfortunately, 
glibc decided that fwrite() should be tagged with WUR and that triggers this 
build failure.

> What I can say is 1. the commit message needs to
> explain *why* the error can be ignored,

The error is already ignored today. I'll send a v2 that makes 
replay_put_array() reflect the error, the callers can then decide to ignore it.

> and 2. the way you ignore the
> error is scandalously ugly :)

Oh we all agree on that. :-D

> You say gcc doesn't honor the traditional
> cast to void anymore.  What's the exact error message you see?

See this (lengthly discussion) for more details:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=25509

Thanks,
Felipe




reply via email to

[Prev in Thread] Current Thread [Next in Thread]