On Wed, Nov 26, 2014 at 11:18:23AM +0100, Fabiano Fidêncio wrote:
> On Wed, Nov 26, 2014 at 10:14 AM, Christophe Fergeau
> <cfergeau@xxxxxxxxxx> 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.
Ah right, the naming of the label is unusual, feel free to change it to
"end:" or "cleanup:" for something clearer.
Christophe
Attachment:
pgpjiE2I4uT0y.pgp
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel