[Qemu-devel] [PATCH 1/2] compiler: add ignore_value() macro

[Qemu-devel] [PATCH 1/2] compiler: add ignore_value() macro

[Felipe Franciosi]

On GCC versions 3.4 and newer, simply using (void) in front of a
function that has been declared with WUR will no longer suppress a
compilation warning. This commit brings the ignore_value() macro from
GNULIB's ignore_value.h, licensed under the terms of LGPLv2+.

See the link below for the original author's comment:
https://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg05148.html

Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
---
 include/qemu/compiler.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index 338d3a6..655d0c7 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -26,6 +26,14 @@
 #define QEMU_WARN_UNUSED_RESULT
 #endif
 
+/* The ignore_value() macro comes from GNULIB's LGPLv2+ ignore-value.h */
+#if QEMU_GNUC_PREREQ(3, 4)
+# define ignore_value(x) \
+         (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; }))
+#else
+# define ignore_value(x) ((void) (x))
+#endif
+
 #if QEMU_GNUC_PREREQ(4, 0)
 #define QEMU_SENTINEL __attribute__((sentinel))
 #else
-- 
1.9.5

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

[Felipe Franciosi]

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.

Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
---
 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));
     }
 }
 
-- 
1.9.5

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2099 bytes --]

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...

> 
> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
> ---
>  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.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Felipe Franciosi]

> On 21 Sep 2016, at 19:17, Eric Blake <eblake@redhat.com> 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 <felipe@nutanix.com>
>> ---
>> 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
> 

Re: [Qemu-devel] [PATCH 1/2] compiler: add ignore_value() macro

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 762 bytes --]

On 09/21/2016 10:27 AM, Felipe Franciosi wrote:
> On GCC versions 3.4 and newer, simply using (void) in front of a
> function that has been declared with WUR will no longer suppress a
> compilation warning. This commit brings the ignore_value() macro from
> GNULIB's ignore_value.h, licensed under the terms of LGPLv2+.
> 
> See the link below for the original author's comment:
> https://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg05148.html
> 
> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
> ---
>  include/qemu/compiler.h | 8 ++++++++
>  1 file changed, 8 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/2] compiler: add ignore_value() macro

[Felipe Franciosi]

> On 21 Sep 2016, at 19:15, Eric Blake <eblake@redhat.com> wrote:
> 
> On 09/21/2016 10:27 AM, Felipe Franciosi wrote:
>> On GCC versions 3.4 and newer, simply using (void) in front of a
>> function that has been declared with WUR will no longer suppress a
>> compilation warning. This commit brings the ignore_value() macro from
>> GNULIB's ignore_value.h, licensed under the terms of LGPLv2+.
>> 
>> See the link below for the original author's comment:
>> https://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg05148.html
>> 
>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
>> ---
>> include/qemu/compiler.h | 8 ++++++++
>> 1 file changed, 8 insertions(+)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Hi Eric,

Now that the header and licensing of compiler.h got amended, can we look at this again?

I think the right solution (at least for the moment) is to stick with my series to fix the build.
1/2) We get the ignore_value() macro in.
2/2) We ignore the return value of that fwrite().

As I clarified on my last e-mail in this thread (21/sept), there are many other calls around the replay code which could potentially fail and are all unchecked. We could at least fix the build now and then talk about a separate series to address those, if neccesary.

Thanks,
Felipe

Re: [Qemu-devel] [PATCH 1/2] compiler: add ignore_value() macro

[Felipe Franciosi]

> On 12 Oct 2016, at 18:20, Felipe Franciosi <felipe@nutanix.com> wrote:
> 
> 
>> On 21 Sep 2016, at 19:15, Eric Blake <eblake@redhat.com> wrote:
>> 
>> On 09/21/2016 10:27 AM, Felipe Franciosi wrote:
>>> On GCC versions 3.4 and newer, simply using (void) in front of a
>>> function that has been declared with WUR will no longer suppress a
>>> compilation warning. This commit brings the ignore_value() macro from
>>> GNULIB's ignore_value.h, licensed under the terms of LGPLv2+.
>>> 
>>> See the link below for the original author's comment:
>>> https://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg05148.html
>>> 
>>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
>>> ---
>>> include/qemu/compiler.h | 8 ++++++++
>>> 1 file changed, 8 insertions(+)
>> 
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> 
>> -- 
>> Eric Blake   eblake redhat com    +1-919-301-3266
>> Libvirt virtualization library http://libvirt.org
>> 
> 
> Hi Eric,
> 
> Now that the header and licensing of compiler.h got amended, can we look at this again?
> 
> I think the right solution (at least for the moment) is to stick with my series to fix the build.
> 1/2) We get the ignore_value() macro in.
> 2/2) We ignore the return value of that fwrite().
> 
> As I clarified on my last e-mail in this thread (21/sept), there are many other calls around the replay code which could potentially fail and are all unchecked. We could at least fix the build now and then talk about a separate series to address those, if neccesary.
> 
> Thanks,
> Felipe


Ping?

Re: [Qemu-devel] [PATCH 1/2] compiler: add ignore_value() macro

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1216 bytes --]

On 10/25/2016 10:21 AM, Felipe Franciosi wrote:
> 
>> On 12 Oct 2016, at 18:20, Felipe Franciosi <felipe@nutanix.com> wrote:
>>
>>
>>> On 21 Sep 2016, at 19:15, Eric Blake <eblake@redhat.com> wrote:
>>>
>>> On 09/21/2016 10:27 AM, Felipe Franciosi wrote:
>>>> On GCC versions 3.4 and newer, simply using (void) in front of a
>>>> function that has been declared with WUR will no longer suppress a
>>>> compilation warning. This commit brings the ignore_value() macro from
>>>> GNULIB's ignore_value.h, licensed under the terms of LGPLv2+.
>>>>
>>>> See the link below for the original author's comment:
>>>> https://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg05148.html
>>>>
>>>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
>>>> ---
>>>> include/qemu/compiler.h | 8 ++++++++
>>>> 1 file changed, 8 insertions(+)
>>>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>

> 
> Ping?

It's missed 2.8, but I still think it is a useful patch; I just
commented on another thread about a case where a cast-to-void might look
better with the use of ignore_value().

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]