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