[pulseaudio-discuss] [PATCH] sink-input, source-output: add a couple of assertions

[Arun Raghavan]

On Mon, 30 Oct 2017, at 12:45 AM, Tanu Kaskinen wrote:
> Coverity complained about data->sink being possibly NULL when it's
> dereferenced later. It was difficult for me to figure out whether that
> was a false positive or not. Hopefully the comments make it a bit
> easier to reason about the code in the future.
> 
> It might be a good idea to set data->req_formats early, so that it's
> always set when setting the sink for a stream. Currently, if the
> application doesn't use the new format negotiation API, req_formats is
> set according to the sample spec at a very late stage. That means that
> sometimes data->sink gets set after data->req_formats, and sometimes
> data->req_formats gets set after data->sink, which makes it difficult to
> follow the code.

It's hard to deterministically set it earlier than pa_sink_input_new(),
unless we make it a contract that it is set before that call for all
callers. Or did you mean we should set it early just for the
protocol-native case?

> CID: 1323591
> ---
>  src/pulsecore/sink-input.c    | 5 +++++
>  src/pulsecore/source-output.c | 5 +++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/src/pulsecore/sink-input.c b/src/pulsecore/sink-input.c
> index 05fe2c026..f993322e9 100644
> --- a/src/pulsecore/sink-input.c
> +++ b/src/pulsecore/sink-input.c
> @@ -338,6 +338,11 @@ int pa_sink_input_new(
>          data->format =
>          pa_format_info_copy(pa_idxset_first(data->nego_formats, NULL));
>  
>      if (PA_LIKELY(data->format)) {
> +        /* We know that data->sink is set, because data->format has been
> set.
> +         * data->format is set after a successful format negotiation,
> and that
> +         * can't happen before data->sink has been set. */
> +        pa_assert(data->sink);
> +
>          pa_log_debug("Negotiated format: %s",
>          pa_format_info_snprint(fmt, sizeof(fmt), data->format));
>      } else {
>          pa_format_info *format;
> diff --git a/src/pulsecore/source-output.c
> b/src/pulsecore/source-output.c
> index f8a421aa8..f8f4e0ef0 100644
> --- a/src/pulsecore/source-output.c
> +++ b/src/pulsecore/source-output.c
> @@ -280,6 +280,11 @@ int pa_source_output_new(
>          data->format =
>          pa_format_info_copy(pa_idxset_first(data->nego_formats, NULL));
>  
>      if (PA_LIKELY(data->format)) {
> +        /* We know that data->source is set, because data->format has
> been set.
> +         * data->format is set after a successful format negotiation,
> and that
> +         * can't happen before data->source has been set. */
> +        pa_assert(data->source);
> +
>          pa_log_debug("Negotiated format: %s",
>          pa_format_info_snprint(fmt, sizeof(fmt), data->format));
>      } else {
>          pa_format_info *format;
> -- 

Looks good.

-- Arun