[FFmpeg-devel] [PATCH 1/2] fftools: Use right function signature and pointers

Paul B Mahol onemda at gmail.com
Wed Aug 14 19:06:04 EEST 2019


LGTM

On Mon, Aug 12, 2019 at 11:59 PM Andreas Rheinhardt <
andreas.rheinhardt at gmail.com> wrote:

> Andreas Rheinhardt:
> > The option tables of the various fftools (in particular ffprobe) are
> > arrays of OptionDef; said type contains a union of a pointer to void and
> > a function pointer of type int (*)(void *, const char *, const char *)
> > as well as a size_t. Some entries (namely the common entry for writing a
> > report as well as several more of ffprobe's entries) used the pointer to
> > void to store a pointer to functions of type int (*)(const char *) or
> > type int (*)(const char *, const char *); nevertheless, when the
> functions
> > are actually called in write_option (in cmdutils.c), it is done via a
> > pointer of the first type.
> >
> > There are two things wrong here:
> > 1. Pointer to void can be converted to any pointer to incomplete or
> > object type and back; but they are nevertheless not completely generic
> > pointers: There is no provision in the C standard that guarantees their
> > convertibility with function pointers. C90 lacks a generic function
> > pointer, C99 made every function pointer a generic function pointer and
> > still disallows the convertibility with void *.
> > 2. The signature of the called function differs from the signature
> > of the pointed-to type. This is undefined behaviour in C99 (given that
> > C90 lacks a way to convert function pointers at all, it doesn't say
> > anything about such a situation). It only works because none of the
> > functions this patch is about make any use of their parameters at all.
> >
> > Therefore this commit changes the type of the relevant functions
> > to match the type used for the call and uses the union's function
> > pointer to store it. This is legal even in C90.
> >
> > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> > ---
> > There are other places in the codebase that use a pointer to void to
> > store a function pointer. Should they be changed or not?
> >
> >  fftools/cmdutils.c |  2 +-
> >  fftools/cmdutils.h |  4 ++--
> >  fftools/ffprobe.c  | 26 +++++++++++++-------------
> >  3 files changed, 16 insertions(+), 16 deletions(-)
> >
> > diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c
> > index 9cfbc45c2b..fdcd376b76 100644
> > --- a/fftools/cmdutils.c
> > +++ b/fftools/cmdutils.c
> > @@ -1046,7 +1046,7 @@ static int init_report(const char *env)
> >      return 0;
> >  }
> >
> > -int opt_report(const char *opt)
> > +int opt_report(void *optctx, const char *opt, const char *arg)
> >  {
> >      return init_report(NULL);
> >  }
> > diff --git a/fftools/cmdutils.h b/fftools/cmdutils.h
> > index 6e2e0a2acb..1917510589 100644
> > --- a/fftools/cmdutils.h
> > +++ b/fftools/cmdutils.h
> > @@ -99,7 +99,7 @@ int opt_default(void *optctx, const char *opt, const
> char *arg);
> >   */
> >  int opt_loglevel(void *optctx, const char *opt, const char *arg);
> >
> > -int opt_report(const char *opt);
> > +int opt_report(void *optctx, const char *opt, const char *arg);
> >
> >  int opt_max_alloc(void *optctx, const char *opt, const char *arg);
> >
> > @@ -236,7 +236,7 @@ void show_help_options(const OptionDef *options,
> const char *msg, int req_flags,
> >      { "colors",      OPT_EXIT,             { .func_arg = show_colors
> },      "show available color names" },            \
> >      { "loglevel",    HAS_ARG,              { .func_arg = opt_loglevel
> },     "set logging level", "loglevel" },         \
> >      { "v",           HAS_ARG,              { .func_arg = opt_loglevel
> },     "set logging level", "loglevel" },         \
> > -    { "report",      0,                    { (void*)opt_report },
>       "generate a report" },                     \
> > +    { "report",      0,                    { .func_arg = opt_report },
>      "generate a report" },                     \
> >      { "max_alloc",   HAS_ARG,              { .func_arg = opt_max_alloc
> },    "set maximum size of a single allocated block", "bytes" }, \
> >      { "cpuflags",    HAS_ARG | OPT_EXPERT, { .func_arg = opt_cpuflags
> },     "force specific cpu flags", "flags" },     \
> >      { "hide_banner", OPT_BOOL | OPT_EXPERT, {&hide_banner},     "do not
> show program banner", "hide_banner" },          \
> > diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
> > index 5aaddb0308..2380417229 100644
> > --- a/fftools/ffprobe.c
> > +++ b/fftools/ffprobe.c
> > @@ -3471,7 +3471,7 @@ static int opt_sections(void *optctx, const char
> *opt, const char *arg)
> >      return 0;
> >  }
> >
> > -static int opt_show_versions(const char *opt, const char *arg)
> > +static int opt_show_versions(void *optctx, const char *opt, const char
> *arg)
> >  {
> >      mark_section_show_entries(SECTION_ID_PROGRAM_VERSION, 1, NULL);
> >      mark_section_show_entries(SECTION_ID_LIBRARY_VERSION, 1, NULL);
> > @@ -3479,7 +3479,7 @@ static int opt_show_versions(const char *opt,
> const char *arg)
> >  }
> >
> >  #define DEFINE_OPT_SHOW_SECTION(section, target_section_id)
>  \
> > -    static int opt_show_##section(const char *opt, const char *arg)
>  \
> > +    static int opt_show_##section(void *optctx, const char *opt, const
> char *arg) \
> >      {
>  \
> >          mark_section_show_entries(SECTION_ID_##target_section_id, 1,
> NULL); \
> >          return 0;
>  \
> > @@ -3514,9 +3514,9 @@ static const OptionDef real_options[] = {
> >      { "sections", OPT_EXIT, {.func_arg = opt_sections}, "print sections
> structure and section information, and exit" },
> >      { "show_data",    OPT_BOOL, {(void*)&do_show_data}, "show packets
> data" },
> >      { "show_data_hash", OPT_STRING | HAS_ARG, {(void*)&show_data_hash},
> "show packets data hash" },
> > -    { "show_error",   0, {(void*)&opt_show_error},  "show probing
> error" },
> > -    { "show_format",  0, {(void*)&opt_show_format}, "show
> format/container info" },
> > -    { "show_frames",  0, {(void*)&opt_show_frames}, "show frames info"
> },
> > +    { "show_error",   0, { .func_arg = &opt_show_error },  "show
> probing error" },
> > +    { "show_format",  0, { .func_arg = &opt_show_format }, "show
> format/container info" },
> > +    { "show_frames",  0, { .func_arg = &opt_show_frames }, "show frames
> info" },
> >      { "show_format_entry", HAS_ARG, {.func_arg = opt_show_format_entry},
> >        "show a particular entry from the format/container info", "entry"
> },
> >      { "show_entries", HAS_ARG, {.func_arg = opt_show_entries},
> > @@ -3524,16 +3524,16 @@ static const OptionDef real_options[] = {
> >  #if HAVE_THREADS
> >      { "show_log", OPT_INT|HAS_ARG, {(void*)&do_show_log}, "show log" },
> >  #endif
> > -    { "show_packets", 0, {(void*)&opt_show_packets}, "show packets
> info" },
> > -    { "show_programs", 0, {(void*)&opt_show_programs}, "show programs
> info" },
> > -    { "show_streams", 0, {(void*)&opt_show_streams}, "show streams
> info" },
> > -    { "show_chapters", 0, {(void*)&opt_show_chapters}, "show chapters
> info" },
> > +    { "show_packets", 0, { .func_arg = &opt_show_packets }, "show
> packets info" },
> > +    { "show_programs", 0, { .func_arg = &opt_show_programs }, "show
> programs info" },
> > +    { "show_streams", 0, { .func_arg = &opt_show_streams }, "show
> streams info" },
> > +    { "show_chapters", 0, { .func_arg = &opt_show_chapters }, "show
> chapters info" },
> >      { "count_frames", OPT_BOOL, {(void*)&do_count_frames}, "count the
> number of frames per stream" },
> >      { "count_packets", OPT_BOOL, {(void*)&do_count_packets}, "count the
> number of packets per stream" },
> > -    { "show_program_version",  0, {(void*)&opt_show_program_version},
> "show ffprobe version" },
> > -    { "show_library_versions", 0, {(void*)&opt_show_library_versions},
> "show library versions" },
> > -    { "show_versions",         0, {(void*)&opt_show_versions}, "show
> program and library versions" },
> > -    { "show_pixel_formats", 0, {(void*)&opt_show_pixel_formats}, "show
> pixel format descriptions" },
> > +    { "show_program_version",  0, { .func_arg =
> &opt_show_program_version },  "show ffprobe version" },
> > +    { "show_library_versions", 0, { .func_arg =
> &opt_show_library_versions }, "show library versions" },
> > +    { "show_versions",         0, { .func_arg = &opt_show_versions },
> "show program and library versions" },
> > +    { "show_pixel_formats", 0, { .func_arg = &opt_show_pixel_formats },
> "show pixel format descriptions" },
> >      { "show_private_data", OPT_BOOL, {(void*)&show_private_data}, "show
> private data" },
> >      { "private",           OPT_BOOL, {(void*)&show_private_data}, "same
> as show_private_data" },
> >      { "bitexact", OPT_BOOL, {&do_bitexact}, "force bitexact output" },
> >
> Ping.
>
> - Andreas
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".


More information about the ffmpeg-devel mailing list