[FFmpeg-devel] [PATCH 1/2] dshow: fix return code when opening device

[Stefano Sabatini]

On date Tuesday 2012-12-04 18:58:28 -0500, Don Moir encoded:
> >On date Tuesday 2012-12-04 04:22:06 -0200, Ramiro Polla encoded:
> >>Hi,
> >>
> >>I've sent this patch before, but I had forgotten why I had written it.
> >>Thanks to Don Moir for pointing out why it was necessary.
> >>
> >>Ramiro
> >
> >>From d2d9631a110bbb28d5aa2082fe508df5739543ac Mon Sep 17 00:00:00 2001
> >>From: Ramiro Polla <ramiro.polla at gmail.com>
> >>Date: Tue, 4 Dec 2012 02:29:43 -0200
> >>Subject: [PATCH 1/2] dshow: fix return code when opening device
> >>
> >>Successfully opening a device altered the ret variable, making the function
> >>not cleanup properly and return an incorrect value for errors that happened
> >>afterwards.
> >>---
> >> libavdevice/dshow.c |   18 ++++++++----------
> >> 1 files changed, 8 insertions(+), 10 deletions(-)
> >>
> >>diff --git a/libavdevice/dshow.c b/libavdevice/dshow.c
> >>index ea01b2a..18a4ee9 100644
> >>--- a/libavdevice/dshow.c
> >>+++ b/libavdevice/dshow.c
> >>@@ -929,20 +929,18 @@ static int dshow_read_header(AVFormatContext *avctx)
> >>     }
> >>
> >>     if (ctx->device_name[VideoDevice]) {
> >>-        ret = dshow_open_device(avctx, devenum, VideoDevice);
> >>-        if (ret < 0)
> >>-            goto error;
> >>-        ret = dshow_add_device(avctx, VideoDevice);
> >>-        if (ret < 0)
> >>+        if ((r = dshow_open_device(avctx, devenum, VideoDevice)) < 0 ||
> >>+            (r = dshow_add_device(avctx, VideoDevice) < 0)) {
> >>+            ret = r;
> >>             goto error;
> >>+        }
> >
> >Isn't this perfectly equivalent to the pre-existing code? The only
> >difference is that in case of success it sets the ret value to the
> >return code of dshow_open/add_device, which is finally returned (while
> >the other failures, e.g. if (!ctx->mutex), don't change the default
> >return value of 0, so won't cause a failure, is this intended?).
> 
> The problem with the pre-existing code is that ret has been set to a
> zero value indicating success. If it fails after the above chunk of
> code it just does a goto error with a successful return (ret) value.
> That patch has a ')'  wrong in 2 places though.

Oh I see. Well I suggest to just have a single ret and *always* set
the error code explicitely, this should simplify readability and a
macro like #define FAIL(err) { ret = err; goto end; } may help to
reduce code clutter.
-- 
FFmpeg = Frenzy Freak Meaningless Plastic Elfic Glue