[Spice-devel] [PATCH 2/2] audio: Avoid bad pipelines from gst_parse_launch

[Fabiano Fidêncio]

On Wed, Nov 26, 2014 at 10:14 AM, Christophe Fergeau
<cfergeau at redhat.com> wrote:
> On Tue, Nov 25, 2014 at 11:06:05PM +0100, Fabiano Fidêncio wrote:
>> On Tue, 2014-11-25 at 17:25 +0100, Victor Toso wrote:
>> > gst_parse_launch may return not NULL even when error is set.
>> > This can lead to data loss when playing or recording audio.
>> > ---
>> >  gtk/spice-gstaudio.c | 12 ++++++++++--
>> >  1 file changed, 10 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/gtk/spice-gstaudio.c b/gtk/spice-gstaudio.c
>> > index 5f9abb2..d6cc774 100644
>> > --- a/gtk/spice-gstaudio.c
>> > +++ b/gtk/spice-gstaudio.c
>> > @@ -245,7 +245,7 @@ static void record_start(SpiceRecordChannel *channel, gint format, gint channels
>> >                              "appsink caps=\"%s\" name=appsink", audio_caps);
>> >
>> >          p->record.pipe = gst_parse_launch(pipeline, &error);
>> > -        if (p->record.pipe == NULL) {
>> > +        if (error != NULL) {
>> >              g_warning("Failed to create pipeline: %s", error->message);
>> >              goto lerr;
>> >          }
>> > @@ -269,6 +269,10 @@ static void record_start(SpiceRecordChannel *channel, gint format, gint channels
>> >  #endif
>> >
>> >  lerr:
>> > +        if (error != NULL && p->record.pipe != NULL) {
>> > +            gst_object_unref(p->record.pipe);
>> > +            p->record.pipe = NULL;
>> > +        }
>>
>> It's kind-of related to your code here ...
>> I don't see a reason to have the goto and this lerr label here.
>> - audio_caps could only be set when the pipeline is NULL and freed right
>> inside this context.
>> - pipeline could be freed just after gst_parse_launch() and right before
>> the error checking.
>> - the error checking could cleanup the error and pipe inside the if and
>> simply return here.
>
> For what it's worth, grouping all cleanup code to the end of the
> function avoids cluttering the main code path with too much
> housekeeping, if we add more codepath which can error out, then
> the new codepath will not introduce leaks of existing objects if it
> errors out, ... so I'd tend to keep the code this way unless there's a
> huge benefit of moving the cleanup code back to the error checking
> sites.

I understand the idea, but the code itself is not "clear" as it could be.
"lerr" as label name makes me think that I'm reaching that part in
case of error, then the error checking just after the label tells me
the opposite. Anyway, as it's not directly related to the patch, so
keep it as it is. If in the future we have to add more repeated checks
during the clean up we can come up with some thing better.

iow, ACK.

Best Regards.
-- 
Fabiano Fidêncio