[FFmpeg-devel] [PATCH 1/2] fftools: Use right function signature and pointers
Andreas Rheinhardt
andreas.rheinhardt at gmail.com
Tue Aug 13 00:52:00 EEST 2019
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
More information about the ffmpeg-devel
mailing list