| Message ID | 20220815183410.6741-1-ffmpeg@gyani.pro |
|---|---|
| State | Accepted |
| Commit | d5544f6457ad06987311beda0e4b7c08bf52915c |
| Headers | show |
| Series | [FFmpeg-devel] ffprobe: restore reporting error code for failed inputs | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On date Tuesday 2022-08-16 00:04:10 +0530, Gyan Doshi wrote: > c11fb46731 led to a regression whereby the return code for missing > input or input probe is overridden by writer close return code and > hence not conveyed in the exit code. > --- > fftools/ffprobe.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > Affects 5.1 so will need to be backported there. > > diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c > index ad633ccc44..8983dc28cc 100644 > --- a/fftools/ffprobe.c > +++ b/fftools/ffprobe.c > @@ -4032,7 +4032,7 @@ int main(int argc, char **argv) > WriterContext *wctx; > char *buf; > char *w_name = NULL, *w_args = NULL; > - int ret, i; > + int ret, input_ret, i; > > init_dynload(); > > @@ -4156,10 +4156,14 @@ int main(int argc, char **argv) > show_error(wctx, ret); > } > > + input_ret = ret; > + > writer_print_section_footer(wctx); > ret = writer_close(&wctx); > if (ret < 0) > av_log(NULL, AV_LOG_ERROR, "Writing output failed: %s\n", av_err2str(ret)); > + > + ret = FFMIN(ret, input_ret); maybe we should give priority to input_ret in case they are both negative? return input_ret < 0 ? input_ret : ret; ?
On 2022-08-17 04:36 am, Stefano Sabatini wrote: > On date Tuesday 2022-08-16 00:04:10 +0530, Gyan Doshi wrote: >> c11fb46731 led to a regression whereby the return code for missing >> input or input probe is overridden by writer close return code and >> hence not conveyed in the exit code. >> --- >> fftools/ffprobe.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> Affects 5.1 so will need to be backported there. >> >> diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c >> index ad633ccc44..8983dc28cc 100644 >> --- a/fftools/ffprobe.c >> +++ b/fftools/ffprobe.c >> @@ -4032,7 +4032,7 @@ int main(int argc, char **argv) >> WriterContext *wctx; >> char *buf; >> char *w_name = NULL, *w_args = NULL; >> - int ret, i; >> + int ret, input_ret, i; >> >> init_dynload(); >> >> @@ -4156,10 +4156,14 @@ int main(int argc, char **argv) >> show_error(wctx, ret); >> } >> >> + input_ret = ret; >> + >> writer_print_section_footer(wctx); >> ret = writer_close(&wctx); >> if (ret < 0) >> av_log(NULL, AV_LOG_ERROR, "Writing output failed: %s\n", av_err2str(ret)); >> + >> + ret = FFMIN(ret, input_ret); > maybe we should give priority to input_ret in case they are both > negative? > > return input_ret < 0 ? input_ret : ret; Scripts usually depend on exit code being 0 or something else. Also, error is logged for both input failure and writer_close failure, so it doesn't matter. Finally, input_ret is not initialized if writer_open fails, so we shouldn't be referencing it outside. Regards, Gyan
On Wed, 17 Aug 2022, Gyan Doshi wrote: > > > On 2022-08-17 04:36 am, Stefano Sabatini wrote: >> On date Tuesday 2022-08-16 00:04:10 +0530, Gyan Doshi wrote: >>> c11fb46731 led to a regression whereby the return code for missing >>> input or input probe is overridden by writer close return code and >>> hence not conveyed in the exit code. >>> --- >>> fftools/ffprobe.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> Affects 5.1 so will need to be backported there. >>> >>> diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c >>> index ad633ccc44..8983dc28cc 100644 >>> --- a/fftools/ffprobe.c >>> +++ b/fftools/ffprobe.c >>> @@ -4032,7 +4032,7 @@ int main(int argc, char **argv) >>> WriterContext *wctx; >>> char *buf; >>> char *w_name = NULL, *w_args = NULL; >>> - int ret, i; >>> + int ret, input_ret, i; >>> >>> init_dynload(); >>> >>> @@ -4156,10 +4156,14 @@ int main(int argc, char **argv) >>> show_error(wctx, ret); >>> } >>> + input_ret = ret; >>> + >>> writer_print_section_footer(wctx); >>> ret = writer_close(&wctx); >>> if (ret < 0) >>> av_log(NULL, AV_LOG_ERROR, "Writing output failed: %s\n", >>> av_err2str(ret)); >>> + >>> + ret = FFMIN(ret, input_ret); >> maybe we should give priority to input_ret in case they are both >> negative? >> >> return input_ret < 0 ? input_ret : ret; > > Scripts usually depend on exit code being 0 or something else. Also, error is > logged for both input failure and writer_close failure, so it doesn't matter. > Finally, input_ret is not initialized if writer_open fails, so we shouldn't > be referencing it outside. I would do something like ret2 = writer_close(); ret = FFMIN(ret2, ret); But fine either way, please push any version you prefer, this has been broken for too long. Sorry about that. Thanks, Marton
On 2022-08-17 12:37 pm, Marton Balint wrote: > > > On Wed, 17 Aug 2022, Gyan Doshi wrote: > >> >> >> On 2022-08-17 04:36 am, Stefano Sabatini wrote: >>> On date Tuesday 2022-08-16 00:04:10 +0530, Gyan Doshi wrote: >>>> c11fb46731 led to a regression whereby the return code for missing >>>> input or input probe is overridden by writer close return code and >>>> hence not conveyed in the exit code. >>>> --- >>>> fftools/ffprobe.c | 6 +++++- >>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>> >>>> Affects 5.1 so will need to be backported there. >>>> >>>> diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c >>>> index ad633ccc44..8983dc28cc 100644 >>>> --- a/fftools/ffprobe.c >>>> +++ b/fftools/ffprobe.c >>>> @@ -4032,7 +4032,7 @@ int main(int argc, char **argv) >>>> WriterContext *wctx; >>>> char *buf; >>>> char *w_name = NULL, *w_args = NULL; >>>> - int ret, i; >>>> + int ret, input_ret, i; >>>> >>>> init_dynload(); >>>> >>>> @@ -4156,10 +4156,14 @@ int main(int argc, char **argv) >>>> show_error(wctx, ret); >>>> } >>>> + input_ret = ret; >>>> + >>>> writer_print_section_footer(wctx); >>>> ret = writer_close(&wctx); >>>> if (ret < 0) >>>> av_log(NULL, AV_LOG_ERROR, "Writing output failed: >>>> %s\n", >>>> av_err2str(ret)); >>>> + >>>> + ret = FFMIN(ret, input_ret); >>> maybe we should give priority to input_ret in case they are both >>> negative? >>> >>> return input_ret < 0 ? input_ret : ret; >> >> Scripts usually depend on exit code being 0 or something else. Also, >> error is logged for both input failure and writer_close failure, so >> it doesn't matter. >> Finally, input_ret is not initialized if writer_open fails, so we >> shouldn't be referencing it outside. > > I would do something like > > ret2 = writer_close(); > ret = FFMIN(ret2, ret); > > But fine either way, please push any version you prefer, this has been > broken for too long. Sorry about that. The only diff is which return does the new variable store. I like my var name better :/ Will push in a couple of hours, 5.1 as well Regards, Gyan
Pushed as d5544f6457ad06987311beda0e4b7c08bf52915c to master and 4e4cc6e56a899f6b4302e80dbcd6b4462f340905 to 5.1 On 2022-08-17 12:43 pm, Gyan Doshi wrote: > > > On 2022-08-17 12:37 pm, Marton Balint wrote: >> >> >> On Wed, 17 Aug 2022, Gyan Doshi wrote: >> >>> >>> >>> On 2022-08-17 04:36 am, Stefano Sabatini wrote: >>>> On date Tuesday 2022-08-16 00:04:10 +0530, Gyan Doshi wrote: >>>>> c11fb46731 led to a regression whereby the return code for missing >>>>> input or input probe is overridden by writer close return code and >>>>> hence not conveyed in the exit code. >>>>> --- >>>>> fftools/ffprobe.c | 6 +++++- >>>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>>> >>>>> Affects 5.1 so will need to be backported there. >>>>> >>>>> diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c >>>>> index ad633ccc44..8983dc28cc 100644 >>>>> --- a/fftools/ffprobe.c >>>>> +++ b/fftools/ffprobe.c >>>>> @@ -4032,7 +4032,7 @@ int main(int argc, char **argv) >>>>> WriterContext *wctx; >>>>> char *buf; >>>>> char *w_name = NULL, *w_args = NULL; >>>>> - int ret, i; >>>>> + int ret, input_ret, i; >>>>> >>>>> init_dynload(); >>>>> >>>>> @@ -4156,10 +4156,14 @@ int main(int argc, char **argv) >>>>> show_error(wctx, ret); >>>>> } >>>>> + input_ret = ret; >>>>> + >>>>> writer_print_section_footer(wctx); >>>>> ret = writer_close(&wctx); >>>>> if (ret < 0) >>>>> av_log(NULL, AV_LOG_ERROR, "Writing output failed: >>>>> %s\n", >>>>> av_err2str(ret)); >>>>> + >>>>> + ret = FFMIN(ret, input_ret); >>>> maybe we should give priority to input_ret in case they are both >>>> negative? >>>> >>>> return input_ret < 0 ? input_ret : ret; >>> >>> Scripts usually depend on exit code being 0 or something else. Also, >>> error is logged for both input failure and writer_close failure, so >>> it doesn't matter. >>> Finally, input_ret is not initialized if writer_open fails, so we >>> shouldn't be referencing it outside. >> >> I would do something like >> >> ret2 = writer_close(); >> ret = FFMIN(ret2, ret); >> >> But fine either way, please push any version you prefer, this has >> been broken for too long. Sorry about that. > > The only diff is which return does the new variable store. I like my > var name better :/ > > Will push in a couple of hours, 5.1 as well > > Regards, > Gyan > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c index ad633ccc44..8983dc28cc 100644 --- a/fftools/ffprobe.c +++ b/fftools/ffprobe.c @@ -4032,7 +4032,7 @@ int main(int argc, char **argv) WriterContext *wctx; char *buf; char *w_name = NULL, *w_args = NULL; - int ret, i; + int ret, input_ret, i; init_dynload(); @@ -4156,10 +4156,14 @@ int main(int argc, char **argv) show_error(wctx, ret); } + input_ret = ret; + writer_print_section_footer(wctx); ret = writer_close(&wctx); if (ret < 0) av_log(NULL, AV_LOG_ERROR, "Writing output failed: %s\n", av_err2str(ret)); + + ret = FFMIN(ret, input_ret); } end: