Mike, Please see inline
On 13 August 2014 03:13, Mike Holmes <[email protected]> wrote: > Signed-off-by: Mike Holmes <[email protected]> > --- > > This adds the two agreed upon init structure members, I left flags out but > the understanding is we add it along with its first agreed use case. > I wondered though if we need an init struct initializer function that > platforms can implement to set the defaults as they should be ? > Next steps would be to incorporate the logging callback and any debug level > control, presumably we will have another run time API that allows further > manipulation of the debug level, this would just be the initial state of that. > > > example/generator/odp_generator.c | 3 ++- > example/l2fwd/odp_l2fwd.c | 3 ++- > example/odp_example/odp_example.c | 3 ++- > example/packet/odp_pktio.c | 3 ++- > example/timer/odp_timer_test.c | 3 ++- > include/odp_init.h | 22 +++++++++++++++++----- > platform/linux-dpdk/odp_init.c | 2 +- > platform/linux-generic/odp_init.c | 2 +- > platform/linux-keystone2/odp_init.c | 2 +- > test/api_test/odp_common.c | 3 ++- > 10 files changed, 32 insertions(+), 14 deletions(-) > > diff --git a/example/generator/odp_generator.c > b/example/generator/odp_generator.c > index b10372e..893233f 100644 > --- a/example/generator/odp_generator.c > +++ b/example/generator/odp_generator.c > @@ -522,6 +522,7 @@ int main(int argc, char *argv[]) > { > odp_linux_pthread_t thread_tbl[MAX_WORKERS]; > odp_buffer_pool_t pool; > + odp_global_init_t init_data; > int thr_id; > int num_workers; > void *pool_base; > @@ -530,7 +531,7 @@ int main(int argc, char *argv[]) > int core_count; > > /* Init ODP before calling anything else */ > - if (odp_init_global()) { > + if (odp_init_global(init_data)) { Normally structure is not passed by value, it could be big, so it should be passed by pointer: if (odp_init_global(&init_data)) { IMHO given that all current config parameters are options, normally I would allow NULL to be passed to odp_init_global with meaning "use defaults". Also in this particular situation, the way I read above patch, it seems you have init_data allocated on stack of main - meaning it will have garbage in its content. It is wrong. odp_init_global should say that odp_global_init_t content should be memset to 0, fill in fields that current application understands leave remaining to 0. With such approach it would be possible to write code that will survive further addition to structure, i.e all new fields that application was not aware before will be 0. If you don't like memset rule one may provide odp_global_init_t initialization macro and implementation code decides how to initialize it properly. > ODP_ERR("Error: ODP global init failed.\n"); > exit(EXIT_FAILURE); > } > diff --git a/example/l2fwd/odp_l2fwd.c b/example/l2fwd/odp_l2fwd.c > index f89ea7a..12a6f53 100644 > --- a/example/l2fwd/odp_l2fwd.c > +++ b/example/l2fwd/odp_l2fwd.c > @@ -280,6 +280,7 @@ int main(int argc, char *argv[]) > { > odp_linux_pthread_t thread_tbl[MAX_WORKERS]; > odp_buffer_pool_t pool; > + odp_global_init_t init_data; > int thr_id; > void *pool_base; > int i; > @@ -288,7 +289,7 @@ int main(int argc, char *argv[]) > odp_pktio_t pktio; > > /* Init ODP before calling anything else */ > - if (odp_init_global()) { > + if (odp_init_global(init_data)) { > ODP_ERR("Error: ODP global init failed.\n"); > exit(EXIT_FAILURE); > } > diff --git a/example/odp_example/odp_example.c > b/example/odp_example/odp_example.c > index be96093..c82ef51 100644 > --- a/example/odp_example/odp_example.c > +++ b/example/odp_example/odp_example.c > @@ -917,6 +917,7 @@ static void parse_args(int argc, char *argv[], > test_args_t *args) > int main(int argc, char *argv[]) > { > odp_linux_pthread_t thread_tbl[MAX_WORKERS]; > + odp_global_init_t init_data; > test_args_t args; > int thr_id; > int num_workers; > @@ -934,7 +935,7 @@ int main(int argc, char *argv[]) > > memset(thread_tbl, 0, sizeof(thread_tbl)); > > - if (odp_init_global()) { > + if (odp_init_global(init_data)) { > printf("ODP global init failed.\n"); > return -1; > } > diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c > index 247a28a..aa3e848 100644 > --- a/example/packet/odp_pktio.c > +++ b/example/packet/odp_pktio.c > @@ -277,6 +277,7 @@ int main(int argc, char *argv[]) > { > odp_linux_pthread_t thread_tbl[MAX_WORKERS]; > odp_buffer_pool_t pool; > + odp_global_init_t init_data; > int thr_id; > int num_workers; > void *pool_base; > @@ -285,7 +286,7 @@ int main(int argc, char *argv[]) > int core_count; > > /* Init ODP before calling anything else */ > - if (odp_init_global()) { > + if (odp_init_global(init_data)) { > ODP_ERR("Error: ODP global init failed.\n"); > exit(EXIT_FAILURE); > } > diff --git a/example/timer/odp_timer_test.c b/example/timer/odp_timer_test.c > index dbe0e5b..1fce60f 100644 > --- a/example/timer/odp_timer_test.c > +++ b/example/timer/odp_timer_test.c > @@ -193,6 +193,7 @@ static void parse_args(int argc, char *argv[], > test_args_t *args) > int main(int argc, char *argv[]) > { > odp_linux_pthread_t thread_tbl[MAX_WORKERS]; > + odp_global_init_t init_data; > test_args_t args; > int thr_id; > int num_workers; > @@ -210,7 +211,7 @@ int main(int argc, char *argv[]) > > memset(thread_tbl, 0, sizeof(thread_tbl)); > > - if (odp_init_global()) { > + if (odp_init_global(init_data)) { > printf("ODP global init failed.\n"); > return -1; > } > diff --git a/include/odp_init.h b/include/odp_init.h > index 490324a..520e6eb 100644 > --- a/include/odp_init.h > +++ b/include/odp_init.h > @@ -4,7 +4,6 @@ > * SPDX-License-Identifier: BSD-3-Clause > */ > > - > /** > * @file > * > @@ -23,23 +22,36 @@ extern "C" { > #include <odp_std_types.h> > > > - > +/** > + * ODP initialisation struct > + */ > +typedef struct odp_global_init{ > + /** Callback for logging replaces the default implementation if > + * not NULL > + */ > + int (*logging)(char *format,...); Normally it should be 'const char *format'. What is 'int' return type is used for? I think in ODP currently there are two macros used one for reporting errors and another debug one. I think it is good idea to provide two callbacks here. Or if we want to introduce error report value, enumeration defining those level is needed and another arument to the function. Also I don't think it should be ... api, it should be va_arg API. Because normally this function will be used as second layer after ... api. In ODP log function implementation will have ... API, that it would call callback if any was provided or call vprintf and such if default reporting is used. I.e something like this void (*custom_error) (const char *format, va_list va); void odp_error(const char *format, ...) { va_list va; va_start(va, format); if (custom_error) { custom_error(format, va); } else { vprintf(format, va); } va_end(va); } If it would be ... api I am not sure how to call it. Thanks, Victor > + /** Platform specific data. ODP does nothing with > + * this pointer > + */ > + void *platform_implimentaion_data; > +} odp_global_init_t; > > /** > - * Perform global ODP initalisation. > + * Perform global ODP initialisation. > * > * This function must be called once before calling > * any other ODP API functions. > * > * @return 0 if successful > */ > -int odp_init_global(void); > +int odp_init_global(odp_global_init_t params); > > > /** > - * Perform thread local ODP initalisation. > + * Perform thread local ODP initialisation. > * > * All threads must call this function before calling > + * > * any other ODP API functions. > * @param thr_id Thread id > * @return 0 if successful > diff --git a/platform/linux-dpdk/odp_init.c b/platform/linux-dpdk/odp_init.c > index ecc2066..97a4024 100644 > --- a/platform/linux-dpdk/odp_init.c > +++ b/platform/linux-dpdk/odp_init.c > @@ -50,7 +50,7 @@ int odp_init_dpdk(void) > return 0; > } > > -int odp_init_global(void) > +int odp_init_global(odp_global_init_t params ODP_UNUSED) > { > odp_thread_init_global(); > > diff --git a/platform/linux-generic/odp_init.c > b/platform/linux-generic/odp_init.c > index d4c2eb8..db04903 100644 > --- a/platform/linux-generic/odp_init.c > +++ b/platform/linux-generic/odp_init.c > @@ -9,7 +9,7 @@ > #include <odp_debug.h> > > > -int odp_init_global(void) > +int odp_init_global(odp_global_init_t params ODP_UNUSED) > { > odp_thread_init_global(); > > diff --git a/platform/linux-keystone2/odp_init.c > b/platform/linux-keystone2/odp_init.c > index f832551..066dace 100644 > --- a/platform/linux-keystone2/odp_init.c > +++ b/platform/linux-keystone2/odp_init.c > @@ -90,7 +90,7 @@ static int ti_init_hw_config(void) > } > > > -int odp_init_global(void) > +int odp_init_global(odp_global_init_t init_data ODP_UNUSED) > { > odp_thread_init_global(); > > diff --git a/test/api_test/odp_common.c b/test/api_test/odp_common.c > index 89ebd2d..cfbef19 100644 > --- a/test/api_test/odp_common.c > +++ b/test/api_test/odp_common.c > @@ -54,9 +54,10 @@ void odp_print_system_info(void) > /** test init globals and call odp_init_global() */ > int odp_test_global_init(void) > { > + odp_global_init_t init_data; > memset(thread_tbl, 0, sizeof(thread_tbl)); > > - if (odp_init_global()) { > + if (odp_init_global(init_data)) { > ODP_ERR("ODP global init failed.\n"); > return -1; > } > -- > 1.9.1 > > > _______________________________________________ > lng-odp mailing list > [email protected] > http://lists.linaro.org/mailman/listinfo/lng-odp _______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
