[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