[PATCH 0/2] Add QEMU_WARN_UNUSED_RESULT function attribute

[PATCH 0/2] Add QEMU_WARN_UNUSED_RESULT function attribute

[Manos Pitsidianakis]

Functions that return a value to indicate success or failure can be 
decorated with the warn_unused_result attribute. GCC will stop 
compilation if a caller does not check the return value after calling 
such a function. This was not possible to spot statically before, but 
Coverity detects this kind of bug. I was prompted by

https://lore.kernel.org/qemu-devel/CAFEAcA_TS-B0gc-DUYT6BaKnm8Uauhsx3rW2dmVNUgTToVjSJg@mail.gmail.com/

to prevent this from happening in the future.

This patch series depends on 
<20231109162034.2108018-1-manos.pitsidianakis@linaro.org>

https://lore.kernel.org/qemu-devel/20231109162034.2108018-1-manos.pitsidianakis@linaro.org/

Manos Pitsidianakis (2):
  Add QEMU_WARN_UNUSED_RESULT attribute
  Add warn_unused_result attr to AUD_register_card

 audio/audio.h           |  2 +-
 hw/arm/omap2.c          |  8 +++++++-
 hw/input/tsc210x.c      |  8 +++++++-
 include/qemu/compiler.h | 14 ++++++++++++++
 4 files changed, 29 insertions(+), 3 deletions(-)


base-commit: ad6ef0a42e314a8c6ac6c96d5f6e607a1e5644b5
prerequisite-patch-id: 484ec9f7f6109c10d4be0484fe8e3c2550c415f4
-- 
2.39.2

[PATCH 1/2] Add QEMU_WARN_UNUSED_RESULT attribute

[Manos Pitsidianakis]

This commit adds QEMU_WARN_UNUSED_RESULT, a macro for the gcc function
attribute `warn_unused_result`. The utility of this attribute is to
ensure functions that return values that need to be inspected are not
ignored by the caller.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 include/qemu/compiler.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index c797f0d457..7ddbf1f1cf 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -212,6 +212,20 @@
 # define QEMU_USED
 #endif
 
+/*
+ * From GCC documentation:
+ *
+ *   The warn_unused_result attribute causes a warning to be emitted if a
+ *   caller of the function with this attribute does not use its return value.
+ *   This is useful for functions where not checking the result is either a
+ *   security problem or always a bug, such as realloc.
+ */
+#if __has_attribute(warn_unused_result)
+# define QEMU_WARN_UNUSED_RESULT __attribute__((warn_unused_result))
+#else
+# define QEMU_WARN_UNUSED_RESULT
+#endif
+
 /*
  * Ugly CPP trick that is like "defined FOO", but also works in C
  * code.  Useful to replace #ifdef with "if" statements; assumes
-- 
2.39.2

[PATCH 2/2] Add warn_unused_result attr to AUD_register_card

[Manos Pitsidianakis]

Ignoring the return value by accident is easy to miss as a bug. Such a
bug was spotted by Coverity CID 1523899. Now, future instances of this
type of bug will produce a warning when using GCC.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 audio/audio.h      | 2 +-
 hw/arm/omap2.c     | 8 +++++++-
 hw/input/tsc210x.c | 8 +++++++-
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/audio/audio.h b/audio/audio.h
index fcc22307be..b78c75962e 100644
--- a/audio/audio.h
+++ b/audio/audio.h
@@ -94,7 +94,7 @@ typedef struct QEMUAudioTimeStamp {
 void AUD_vlog (const char *cap, const char *fmt, va_list ap) G_GNUC_PRINTF(2, 0);
 void AUD_log (const char *cap, const char *fmt, ...) G_GNUC_PRINTF(2, 3);
 
-bool AUD_register_card (const char *name, QEMUSoundCard *card, Error **errp);
+bool AUD_register_card (const char *name, QEMUSoundCard *card, Error **errp) QEMU_WARN_UNUSED_RESULT;
 void AUD_remove_card (QEMUSoundCard *card);
 CaptureVoiceOut *AUD_add_capture(
     AudioState *s,
diff --git a/hw/arm/omap2.c b/hw/arm/omap2.c
index f170728e7e..59fc061120 100644
--- a/hw/arm/omap2.c
+++ b/hw/arm/omap2.c
@@ -614,7 +614,13 @@ static struct omap_eac_s *omap_eac_init(struct omap_target_agent_s *ta,
         s->codec.card.name = g_strdup(current_machine->audiodev);
         s->codec.card.state = audio_state_by_name(s->codec.card.name, &error_fatal);
     }
-    AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal);
+    /*
+     * We pass error_fatal so on error QEMU will exit(). But we check the
+     * return value to make the warn_unused_result compiler warning go away.
+     */
+    if (!AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)) {
+        exit(1);
+    }
 
     memory_region_init_io(&s->iomem, NULL, &omap_eac_ops, s, "omap.eac",
                           omap_l4_region_size(ta, 0));
diff --git a/hw/input/tsc210x.c b/hw/input/tsc210x.c
index 950506fb38..003c664b56 100644
--- a/hw/input/tsc210x.c
+++ b/hw/input/tsc210x.c
@@ -1102,7 +1102,13 @@ static void tsc210x_init(TSC210xState *s,
         s->card.name = g_strdup(current_machine->audiodev);
         s->card.state = audio_state_by_name(s->card.name, &error_fatal);
     }
-    AUD_register_card(s->name, &s->card, &error_fatal);
+    /*
+     * We pass error_fatal so on error QEMU will exit(). But we check the
+     * return value to make the warn_unused_result compiler warning go away.
+     */
+    if (!AUD_register_card(s->name, &s->card, &error_fatal)) {
+        return;
+    }
 
     qemu_register_reset((void *) tsc210x_reset, s);
     vmstate_register(NULL, 0, vmsd, s);
-- 
2.39.2

Re: [PATCH 1/2] Add QEMU_WARN_UNUSED_RESULT attribute

[Daniel P. Berrangé]

On Fri, Nov 10, 2023 at 11:16:38AM +0200, Manos Pitsidianakis wrote:
> This commit adds QEMU_WARN_UNUSED_RESULT, a macro for the gcc function
> attribute `warn_unused_result`. The utility of this attribute is to
> ensure functions that return values that need to be inspected are not
> ignored by the caller.
> 
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>  include/qemu/compiler.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> index c797f0d457..7ddbf1f1cf 100644
> --- a/include/qemu/compiler.h
> +++ b/include/qemu/compiler.h
> @@ -212,6 +212,20 @@
>  # define QEMU_USED
>  #endif
>  
> +/*
> + * From GCC documentation:
> + *
> + *   The warn_unused_result attribute causes a warning to be emitted if a
> + *   caller of the function with this attribute does not use its return value.
> + *   This is useful for functions where not checking the result is either a
> + *   security problem or always a bug, such as realloc.
> + */
> +#if __has_attribute(warn_unused_result)
> +# define QEMU_WARN_UNUSED_RESULT __attribute__((warn_unused_result))
> +#else
> +# define QEMU_WARN_UNUSED_RESULT
> +#endif

GLib already provides us G_GNUC_WARN_UNUSED_RESULT so don't add this.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/2] Add warn_unused_result attr to AUD_register_card

[Peter Maydell]

On Fri, 10 Nov 2023 at 09:16, Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
>
> Ignoring the return value by accident is easy to miss as a bug. Such a
> bug was spotted by Coverity CID 1523899. Now, future instances of this
> type of bug will produce a warning when using GCC.
>
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>  audio/audio.h      | 2 +-
>  hw/arm/omap2.c     | 8 +++++++-
>  hw/input/tsc210x.c | 8 +++++++-
>  3 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/audio/audio.h b/audio/audio.h
> index fcc22307be..b78c75962e 100644
> --- a/audio/audio.h
> +++ b/audio/audio.h
> @@ -94,7 +94,7 @@ typedef struct QEMUAudioTimeStamp {
>  void AUD_vlog (const char *cap, const char *fmt, va_list ap) G_GNUC_PRINTF(2, 0);
>  void AUD_log (const char *cap, const char *fmt, ...) G_GNUC_PRINTF(2, 3);
>
> -bool AUD_register_card (const char *name, QEMUSoundCard *card, Error **errp);
> +bool AUD_register_card (const char *name, QEMUSoundCard *card, Error **errp) QEMU_WARN_UNUSED_RESULT;
>  void AUD_remove_card (QEMUSoundCard *card);
>  CaptureVoiceOut *AUD_add_capture(
>      AudioState *s,
> diff --git a/hw/arm/omap2.c b/hw/arm/omap2.c
> index f170728e7e..59fc061120 100644
> --- a/hw/arm/omap2.c
> +++ b/hw/arm/omap2.c
> @@ -614,7 +614,13 @@ static struct omap_eac_s *omap_eac_init(struct omap_target_agent_s *ta,
>          s->codec.card.name = g_strdup(current_machine->audiodev);
>          s->codec.card.state = audio_state_by_name(s->codec.card.name, &error_fatal);
>      }
> -    AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal);
> +    /*
> +     * We pass error_fatal so on error QEMU will exit(). But we check the
> +     * return value to make the warn_unused_result compiler warning go away.
> +     */
> +    if (!AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)) {
> +        exit(1);
> +    }

This kind of thing is why Coverity's unused-result warning has a
lot of false positives. We shouldn't introduce extra code like
this to work around the fact that the tooling doesn't understand
our error-handling convention (i.e. error_fatal, and the way
that some functions report errors both via the return value and
also via the Error** argument).

thanks
-- PMM

Re: [PATCH 2/2] Add warn_unused_result attr to AUD_register_card

[Manos Pitsidianakis]

On Fri, 10 Nov 2023 12:21, Peter Maydell <peter.maydell@linaro.org> wrote:
>This kind of thing is why Coverity's unused-result warning has a
>lot of false positives. We shouldn't introduce extra code like
>this to work around the fact that the tooling doesn't understand
>our error-handling convention (i.e. error_fatal, and the way
>that some functions report errors both via the return value and
>also via the Error** argument).

I respect that :). But I personally believe that clinging to C's 
inadequacies, instead of preventing bugs statically just because it adds 
some lines of code, is misguided. Proper code should strive to make bugs 
impossible in the first place. At least that is my perspective and I 
would like there to be constructive discussions about different 
approaches in the mailing list. Perhaps something good might come out of 
it!

Re: [PATCH 1/2] Add QEMU_WARN_UNUSED_RESULT attribute

[Philippe Mathieu-Daudé]

On 10/11/23 10:16, Manos Pitsidianakis wrote:
> This commit adds QEMU_WARN_UNUSED_RESULT, a macro for the gcc function
> attribute `warn_unused_result`. The utility of this attribute is to
> ensure functions that return values that need to be inspected are not
> ignored by the caller.
> 
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>   include/qemu/compiler.h | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)

> +/*
> + * From GCC documentation:
> + *
> + *   The warn_unused_result attribute causes a warning to be emitted if a
> + *   caller of the function with this attribute does not use its return value.
> + *   This is useful for functions where not checking the result is either a
> + *   security problem or always a bug, such as realloc.
> + */
> +#if __has_attribute(warn_unused_result)
> +# define QEMU_WARN_UNUSED_RESULT __attribute__((warn_unused_result))
> +#else
> +# define QEMU_WARN_UNUSED_RESULT
> +#endif

FWIW I sometimes use:

+#if __has_attribute(nonnull)
+# define QEMU_NONNULL(indexes...) __attribute__((nonnull((indexes))))
+#else
+# define QEMU_NONNULL(indexes...)
+#endif

when doing tree-wide refactors, then remove the attribute because
prototypes become really ugly.

Re: [PATCH 2/2] Add warn_unused_result attr to AUD_register_card

[Peter Maydell]

On Fri, 10 Nov 2023 at 11:02, Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
>
> On Fri, 10 Nov 2023 12:21, Peter Maydell <peter.maydell@linaro.org> wrote:
> >This kind of thing is why Coverity's unused-result warning has a
> >lot of false positives. We shouldn't introduce extra code like
> >this to work around the fact that the tooling doesn't understand
> >our error-handling convention (i.e. error_fatal, and the way
> >that some functions report errors both via the return value and
> >also via the Error** argument).
>
> I respect that :). But I personally believe that clinging to C's
> inadequacies, instead of preventing bugs statically just because it adds
> some lines of code, is misguided. Proper code should strive to make bugs
> impossible in the first place.

I generally agree. The problem here really is that we've ended
up with this odd API convention that reports errors in two
ways. In an ideal world we'd tidy up our APIs to report errors
exactly in one way (presumably via the Error).

-- PMM

Re: [PATCH 2/2] Add warn_unused_result attr to AUD_register_card

[Daniel P. Berrangé]

On Fri, Nov 10, 2023 at 12:44:56PM +0200, Manos Pitsidianakis wrote:
> On Fri, 10 Nov 2023 12:21, Peter Maydell <peter.maydell@linaro.org> wrote:
> > This kind of thing is why Coverity's unused-result warning has a
> > lot of false positives. We shouldn't introduce extra code like
> > this to work around the fact that the tooling doesn't understand
> > our error-handling convention (i.e. error_fatal, and the way
> > that some functions report errors both via the return value and
> > also via the Error** argument).
> 
> I respect that :). But I personally believe that clinging to C's
> inadequacies, instead of preventing bugs statically just because it adds
> some lines of code, is misguided. Proper code should strive to make bugs
> impossible in the first place. At least that is my perspective and I would
> like there to be constructive discussions about different approaches in the
> mailing list. Perhaps something good might come out of it!

Your approach to the problem:

  if (!AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)) {
    exit(1);
  }

is adding dead-code because the exit(1) will never be reachable. So while
it lets you squelch the unused result warning, it is verbose and misleading
to anyone who sees it.

Perhaps a more viable option is to pull in gnulib's ignore_value macro

  #define ignore_value(x) \
    (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; }))

and then we would have just this:

 ignore_value(AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal));

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/2] Add warn_unused_result attr to AUD_register_card

[Daniel P. Berrangé]

On Fri, Nov 10, 2023 at 11:18:39AM +0000, Peter Maydell wrote:
> On Fri, 10 Nov 2023 at 11:02, Manos Pitsidianakis
> <manos.pitsidianakis@linaro.org> wrote:
> >
> > On Fri, 10 Nov 2023 12:21, Peter Maydell <peter.maydell@linaro.org> wrote:
> > >This kind of thing is why Coverity's unused-result warning has a
> > >lot of false positives. We shouldn't introduce extra code like
> > >this to work around the fact that the tooling doesn't understand
> > >our error-handling convention (i.e. error_fatal, and the way
> > >that some functions report errors both via the return value and
> > >also via the Error** argument).
> >
> > I respect that :). But I personally believe that clinging to C's
> > inadequacies, instead of preventing bugs statically just because it adds
> > some lines of code, is misguided. Proper code should strive to make bugs
> > impossible in the first place.
> 
> I generally agree. The problem here really is that we've ended
> up with this odd API convention that reports errors in two
> ways. In an ideal world we'd tidy up our APIs to report errors
> exactly in one way (presumably via the Error).

The compelling thing about our slightly odd &error_fatal/error_abort
approach is that we can directly get stack traces showing exactly
where the error happened.

If we just propagate the error up the stack to finally exit/abort,
the origin context is essentially lost, and when it does abort, we
don't get a snapshot of all threads at the time the error hit, as
we've moved on in time.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/2] Add warn_unused_result attr to AUD_register_card

[Manos Pitsidianakis]

On Fri, 10 Nov 2023 13:20, "Daniel P. Berrangé" <berrange@redhat.com> wrote:
>Your approach to the problem:
>
>  if (!AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)) {
>    exit(1);
>  }
>
>is adding dead-code because the exit(1) will never be reachable. So while
>it lets you squelch the unused result warning, it is verbose and misleading
>to anyone who sees it.
>
>Perhaps a more viable option is to pull in gnulib's ignore_value macro
>
>  #define ignore_value(x) \
>    (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; }))
>
>and then we would have just this:
>
> ignore_value(AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal));

Good suggestion, thanks!

And to be fair, I did put a comment directly above that line to explain 
the unreachable code path. :)

+    /*
+     * We pass error_fatal so on error QEMU will exit(). But we check 
the
+     * return value to make the warn_unused_result compiler warning go 
away.
+     */

Re: [PATCH 2/2] Add warn_unused_result attr to AUD_register_card

[Manos Pitsidianakis]

On Fri, 10 Nov 2023 13:23, "Daniel P. Berrangé" <berrange@redhat.com> wrote:
>On Fri, Nov 10, 2023 at 11:18:39AM +0000, Peter Maydell wrote:
>> On Fri, 10 Nov 2023 at 11:02, Manos Pitsidianakis
>> <manos.pitsidianakis@linaro.org> wrote:
>> >
>> > On Fri, 10 Nov 2023 12:21, Peter Maydell <peter.maydell@linaro.org> wrote:
>> > >This kind of thing is why Coverity's unused-result warning has a
>> > >lot of false positives. We shouldn't introduce extra code like
>> > >this to work around the fact that the tooling doesn't understand
>> > >our error-handling convention (i.e. error_fatal, and the way
>> > >that some functions report errors both via the return value and
>> > >also via the Error** argument).
>> >
>> > I respect that :). But I personally believe that clinging to C's
>> > inadequacies, instead of preventing bugs statically just because it adds
>> > some lines of code, is misguided. Proper code should strive to make bugs
>> > impossible in the first place.
>> 
>> I generally agree. The problem here really is that we've ended
>> up with this odd API convention that reports errors in two
>> ways. In an ideal world we'd tidy up our APIs to report errors
>> exactly in one way (presumably via the Error).
>
>The compelling thing about our slightly odd &error_fatal/error_abort
>approach is that we can directly get stack traces showing exactly
>where the error happened.
>
>If we just propagate the error up the stack to finally exit/abort,
>the origin context is essentially lost, and when it does abort, we
>don't get a snapshot of all threads at the time the error hit, as
>we've moved on in time.

FYI: It is possible to get a stack trace using gnu libunwind which is 
portable, dependency-free and runs on many targets: 
https://github.com/libunwind/libunwind#libunwind

Or llvm's libunwind which is also of great quality: 
https://bcain-llvm.readthedocs.io/projects/libunwind/en/latest/

They are also compatible with co-routines and JIT code (At least, the 
GNU one is, not sure about llvm).

Re: [PATCH 2/2] Add warn_unused_result attr to AUD_register_card

[BALATON Zoltan]

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

On Fri, 10 Nov 2023, Daniel P. Berrangé wrote:
> On Fri, Nov 10, 2023 at 12:44:56PM +0200, Manos Pitsidianakis wrote:
>> On Fri, 10 Nov 2023 12:21, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> This kind of thing is why Coverity's unused-result warning has a
>>> lot of false positives. We shouldn't introduce extra code like
>>> this to work around the fact that the tooling doesn't understand
>>> our error-handling convention (i.e. error_fatal, and the way
>>> that some functions report errors both via the return value and
>>> also via the Error** argument).
>>
>> I respect that :). But I personally believe that clinging to C's
>> inadequacies, instead of preventing bugs statically just because it adds
>> some lines of code, is misguided. Proper code should strive to make bugs
>> impossible in the first place. At least that is my perspective and I would
>> like there to be constructive discussions about different approaches in the
>> mailing list. Perhaps something good might come out of it!
>
> Your approach to the problem:
>
>  if (!AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)) {
>    exit(1);
>  }
>
> is adding dead-code because the exit(1) will never be reachable. So while
> it lets you squelch the unused result warning, it is verbose and misleading
> to anyone who sees it.
>
> Perhaps a more viable option is to pull in gnulib's ignore_value macro
>
>  #define ignore_value(x) \
>    (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; }))
>
> and then we would have just this:
>
> ignore_value(AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal));

I wonder if just casting to (void) without assigning to a value could 
avoid the warning? In that case you would not need a macro just

(void)AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal);

Not sure it's clearer than a macro which states what it does but the 
definition is a bit cryptic while this cast is simpler but may also puzzle 
somebody not familiar with it.

Regards,
BALATON Zoltan

Re: [PATCH 2/2] Add warn_unused_result attr to AUD_register_card

[Daniel P. Berrangé]

On Fri, Nov 10, 2023 at 12:35:56PM +0100, BALATON Zoltan wrote:
> On Fri, 10 Nov 2023, Daniel P. Berrangé wrote:
> > On Fri, Nov 10, 2023 at 12:44:56PM +0200, Manos Pitsidianakis wrote:
> > > On Fri, 10 Nov 2023 12:21, Peter Maydell <peter.maydell@linaro.org> wrote:
> > > > This kind of thing is why Coverity's unused-result warning has a
> > > > lot of false positives. We shouldn't introduce extra code like
> > > > this to work around the fact that the tooling doesn't understand
> > > > our error-handling convention (i.e. error_fatal, and the way
> > > > that some functions report errors both via the return value and
> > > > also via the Error** argument).
> > > 
> > > I respect that :). But I personally believe that clinging to C's
> > > inadequacies, instead of preventing bugs statically just because it adds
> > > some lines of code, is misguided. Proper code should strive to make bugs
> > > impossible in the first place. At least that is my perspective and I would
> > > like there to be constructive discussions about different approaches in the
> > > mailing list. Perhaps something good might come out of it!
> > 
> > Your approach to the problem:
> > 
> >  if (!AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)) {
> >    exit(1);
> >  }
> > 
> > is adding dead-code because the exit(1) will never be reachable. So while
> > it lets you squelch the unused result warning, it is verbose and misleading
> > to anyone who sees it.
> > 
> > Perhaps a more viable option is to pull in gnulib's ignore_value macro
> > 
> >  #define ignore_value(x) \
> >    (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; }))
> > 
> > and then we would have just this:
> > 
> > ignore_value(AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal));
> 
> I wonder if just casting to (void) without assigning to a value could avoid
> the warning? In that case you would not need a macro just
> 
> (void)AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal);

Casting to void is not sufficient, which is why I suggested the
ignore_value macro, which does enough to fool the compiler into
thinking the return value is used.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/2] Add warn_unused_result attr to AUD_register_card

[Philippe Mathieu-Daudé]

On 10/11/23 12:20, Daniel P. Berrangé wrote:
> On Fri, Nov 10, 2023 at 12:44:56PM +0200, Manos Pitsidianakis wrote:
>> On Fri, 10 Nov 2023 12:21, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> This kind of thing is why Coverity's unused-result warning has a
>>> lot of false positives. We shouldn't introduce extra code like
>>> this to work around the fact that the tooling doesn't understand
>>> our error-handling convention (i.e. error_fatal, and the way
>>> that some functions report errors both via the return value and
>>> also via the Error** argument).
>>
>> I respect that :). But I personally believe that clinging to C's
>> inadequacies, instead of preventing bugs statically just because it adds
>> some lines of code, is misguided. Proper code should strive to make bugs
>> impossible in the first place. At least that is my perspective and I would
>> like there to be constructive discussions about different approaches in the
>> mailing list. Perhaps something good might come out of it!
> 
> Your approach to the problem:
> 
>    if (!AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)) {
>      exit(1);

Rather:

        g_assert_not_reached();

>    }
> 
> is adding dead-code because the exit(1) will never be reachable. So while
> it lets you squelch the unused result warning, it is verbose and misleading
> to anyone who sees it.