[FFmpeg-devel] [PATCH 2/2] cmdutils: allow specifying the report file

[Nicolas George]

Le primidi 11 brumaire, an CCXXI, Michael Niedermayer a écrit :
> This uses a environment variable as it is tricky through the command line.
> 
> Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> ---
>  cmdutils.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/cmdutils.c b/cmdutils.c
> index f696700..f68a25a 100644
> --- a/cmdutils.c
> +++ b/cmdutils.c
> @@ -542,6 +542,10 @@ int opt_report(const char *opt)
>               program_name,
>               tm->tm_year + 1900, tm->tm_mon + 1, tm->tm_mday,
>               tm->tm_hour, tm->tm_min, tm->tm_sec);
> +
> +    if(getenv("FFREPORT_FILE")) {
> +        av_strlcpy(filename, getenv("FFREPORT_FILE"), sizeof(filename));
> +    }
>      report_file = fopen(filename, "w");
>      if (!report_file) {
>          av_log(NULL, AV_LOG_ERROR, "Failed to open report \"%s\": %s\n",

From the technical point of view, it lacks a check for buffer overflow
and/or a larger buffer (the current buffer for the filename is rather small;
an overflow can lead to several logs being written to the same file), and a
documentation update.

From the user perspective, I do not like the multiplication of environment
variables.

I would like to suggest the following diff (which is not yet a patch): it
uses the FFREPORT environment variable, just as before, but it extracts a
filename from it if it has the form "file=something". I would like to
implement %-expansion (at least %p for program_name = "ffmpeg", "ffplay",
"ffprobe", etc., and %t for the date and time), but it is already working.

Of course, if other people think that a separate environment variable is
better, I can revise my position.

Regards,

-- 
  Nicolas George




diff --git a/cmdutils.c b/cmdutils.c
index f696700..564f3a2 100644
--- a/cmdutils.c
+++ b/cmdutils.c
@@ -58,6 +58,8 @@
 #include <sys/resource.h>
 #endif
 
+static int init_report(const char *env);
+
 struct SwsContext *sws_opts;
 SwrContext *swr_opts;
 AVDictionary *format_opts, *codec_opts;
@@ -414,13 +416,14 @@ static void dump_argument(const char *a)
 void parse_loglevel(int argc, char **argv, const OptionDef *options)
 {
     int idx = locate_option(argc, argv, options, "loglevel");
+    const char *env;
     if (!idx)
         idx = locate_option(argc, argv, options, "v");
     if (idx && argv[idx + 1])
         opt_loglevel(NULL, "loglevel", argv[idx + 1]);
     idx = locate_option(argc, argv, options, "report");
-    if (idx || getenv("FFREPORT")) {
-        opt_report("report");
+    if ((env = getenv("FFREPORT")) || idx) {
+        init_report(env);
         if (report_file) {
             int i;
             fprintf(report_file, "Command line:\n");
@@ -528,9 +531,10 @@ int opt_loglevel(void *optctx, const char *opt, const char *arg)
     return 0;
 }
 
-int opt_report(const char *opt)
+static int init_report(const char *env)
 {
     char filename[64];
+    const char *real_filename = filename;
     time_t now;
     struct tm *tm;
 
@@ -542,10 +546,12 @@ int opt_report(const char *opt)
              program_name,
              tm->tm_year + 1900, tm->tm_mon + 1, tm->tm_mday,
              tm->tm_hour, tm->tm_min, tm->tm_sec);
-    report_file = fopen(filename, "w");
+    if (env && !memcmp(env, "file=", 5))
+        real_filename = env + 5;
+    report_file = fopen(real_filename, "w");
     if (!report_file) {
         av_log(NULL, AV_LOG_ERROR, "Failed to open report \"%s\": %s\n",
-               filename, strerror(errno));
+               real_filename, strerror(errno));
         return AVERROR(errno);
     }
     av_log_set_callback(log_callback_report);
@@ -555,11 +561,16 @@ int opt_report(const char *opt)
            program_name,
            tm->tm_year + 1900, tm->tm_mon + 1, tm->tm_mday,
            tm->tm_hour, tm->tm_min, tm->tm_sec,
-           filename);
+           real_filename);
     av_log_set_level(FFMAX(av_log_get_level(), AV_LOG_VERBOSE));
     return 0;
 }
 
+int opt_report(const char *opt)
+{
+    return init_report(NULL);
+}
+
 int opt_max_alloc(void *optctx, const char *opt, const char *arg)
 {
     char *tail;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121102/2197439e/attachment.asc>