I'm happy with either 1 or 3. And yes, the purpose of the struct tag is to permit self-reference. Since it's not intended to be used by anyone except the typedef itself it can be at the implementer's discretion. If you have a self-referencing struct then use it, otherwise you can either omit it or include it to avoid the need to add it later should you extend things to include a self reference.
On Fri, Aug 29, 2014 at 12:19 PM, Mike Holmes <[email protected]> wrote: > Ok, I need a better feel for consensus here or it will flip flop :), as > this impacts all structs we really need a final answer > > typedef struct odp_<name>_t {...} odp_<name>_t; > OR > typedef struct odp_<name>_s {...} odp_<name>_t; > OR > typedef struct {...} odp_<name>_t; > > I prefer the latter, why even name it unless you need to reference the > name inside the struct being typedefed ? > > > > > On 29 August 2014 13:10, Bill Fischofer <[email protected]> wrote: > >> We've been using odp_<name>_t for typedef stuff, taking advantage that of >> the fact that struct tags occupy a different namespace. So >> >> typedef struct odp_<name>_t {...} odp_<name>_t; >> >> is perfectly valid and unambiguous C and avoids additional namespace >> clutter. >> >> odp_<name>_e has been suggested for ODP enums. >> >> Bill >> >> >> On Fri, Aug 29, 2014 at 12:05 PM, Mike Holmes <[email protected]> >> wrote: >> >>> >>> >>> >>> On 29 August 2014 03:59, Savolainen, Petri (NSN - FI/Espoo) < >>> [email protected]> wrote: >>> >>>> >>>> >>>> > -----Original Message----- >>>> > From: [email protected] [mailto:lng-odp- >>>> > [email protected]] On Behalf Of ext Mike Holmes >>>> > Sent: Thursday, August 28, 2014 9:26 PM >>>> > To: [email protected] >>>> > Subject: [lng-odp] [PATCH v4] Add-global_init-paramiters >>>> > >>>> > Signed-off-by: Mike Holmes <[email protected]> >>>> > --- >>>> > >>>> > v2: >>>> > Factor out odp_platform_init so that odp_global_init is only defined >>>> > for linux_generic and resued. >>>> > >>>> > v3: >>>> > Use -M to simplify the diff >>>> > Fix missing doxygen comments >>>> > >>>> > v4: >>>> > Improve the documentation. >>>> > >>>> > example/generator/odp_generator.c | 2 +- >>>> > example/l2fwd/odp_l2fwd.c | 2 +- >>>> > example/odp_example/odp_example.c | 2 +- >>>> > example/packet/odp_pktio.c | 2 +- >>>> > example/timer/odp_timer_test.c | 2 +- >>>> > platform/linux-dpdk/Makefile.am | 3 +- >>>> > platform/linux-dpdk/{odp_init.c => odp_platform.c} | 63 >>>> +---------------- >>>> > -- >>>> > platform/linux-generic/Makefile.am | 1 + >>>> > platform/linux-generic/include/api/odp_init.h | 34 +++++++--- >>>> > platform/linux-generic/include/odp_internal.h | 3 + >>>> > platform/linux-generic/odp_init.c | 9 ++- >>>> > platform/linux-generic/odp_platform.c | 14 +++++ >>>> > platform/linux-keystone2/Makefile.am | 3 +- >>>> > .../linux-keystone2/{odp_init.c => odp_platform.c} | 72 >>>> +++-------------- >>>> > ----- >>>> > test/api_test/odp_common.c | 2 +- >>>> > 15 files changed, 72 insertions(+), 142 deletions(-) >>>> > rename platform/linux-dpdk/{odp_init.c => odp_platform.c} (50%) >>>> > create mode 100644 platform/linux-generic/odp_platform.c >>>> > rename platform/linux-keystone2/{odp_init.c => odp_platform.c} (72%) >>>> > >>>> > diff --git a/example/generator/odp_generator.c >>>> > b/example/generator/odp_generator.c >>>> > index 9fa9b37..ba7aa68 100644 >>>> > --- a/example/generator/odp_generator.c >>>> > +++ b/example/generator/odp_generator.c >>>> > @@ -525,7 +525,7 @@ int main(int argc, char *argv[]) >>>> > int core_count; >>>> > >>>> > /* Init ODP before calling anything else */ >>>> > - if (odp_init_global()) { >>>> > + if (odp_init_global(NULL, NULL)) { >>>> > 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 d74449a..f8d6729 100644 >>>> > --- a/example/l2fwd/odp_l2fwd.c >>>> > +++ b/example/l2fwd/odp_l2fwd.c >>>> > @@ -323,7 +323,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(NULL, NULL)) { >>>> > 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 f0bdf29..3d38ac7 100644 >>>> > --- a/example/odp_example/odp_example.c >>>> > +++ b/example/odp_example/odp_example.c >>>> > @@ -951,7 +951,7 @@ int main(int argc, char *argv[]) >>>> > >>>> > memset(thread_tbl, 0, sizeof(thread_tbl)); >>>> > >>>> > - if (odp_init_global()) { >>>> > + if (odp_init_global(NULL, NULL)) { >>>> > printf("ODP global init failed.\n"); >>>> > return -1; >>>> > } >>>> > diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c >>>> > index f247bd0..f3543a0 100644 >>>> > --- a/example/packet/odp_pktio.c >>>> > +++ b/example/packet/odp_pktio.c >>>> > @@ -311,7 +311,7 @@ int main(int argc, char *argv[]) >>>> > int core_count; >>>> > >>>> > /* Init ODP before calling anything else */ >>>> > - if (odp_init_global()) { >>>> > + if (odp_init_global(NULL, NULL)) { >>>> > 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 bf1d7df..abf90aa 100644 >>>> > --- a/example/timer/odp_timer_test.c >>>> > +++ b/example/timer/odp_timer_test.c >>>> > @@ -260,7 +260,7 @@ int main(int argc, char *argv[]) >>>> > >>>> > memset(thread_tbl, 0, sizeof(thread_tbl)); >>>> > >>>> > - if (odp_init_global()) { >>>> > + if (odp_init_global(NULL, NULL)) { >>>> > printf("ODP global init failed.\n"); >>>> > return -1; >>>> > } >>>> > diff --git a/platform/linux-dpdk/Makefile.am b/platform/linux- >>>> > dpdk/Makefile.am >>>> > index ff49b7d..70fa549 100644 >>>> > --- a/platform/linux-dpdk/Makefile.am >>>> > +++ b/platform/linux-dpdk/Makefile.am >>>> > @@ -67,13 +67,14 @@ __LIB__libodp_la_SOURCES = \ >>>> > odp_buffer.c \ >>>> > odp_buffer_pool.c \ >>>> > ../linux-generic/odp_coremask.c \ >>>> > - odp_init.c \ >>>> > + ../linux-generic/odp_init.c \ >>>> > odp_linux.c \ >>>> > odp_packet.c \ >>>> > odp_packet_dpdk.c \ >>>> > ../linux-generic/odp_packet_flags.c \ >>>> > odp_packet_io.c \ >>>> > ../linux-generic/odp_packet_socket.c \ >>>> > + odp_platform.c \ >>>> > odp_queue.c \ >>>> > ../linux-generic/odp_ring.c \ >>>> > ../linux-generic/odp_rwlock.c \ >>>> > diff --git a/platform/linux-dpdk/odp_init.c b/platform/linux- >>>> > dpdk/odp_platform.c >>>> > similarity index 50% >>>> > rename from platform/linux-dpdk/odp_init.c >>>> > rename to platform/linux-dpdk/odp_platform.c >>>> > index ecc2066..5a054fd 100644 >>>> > --- a/platform/linux-dpdk/odp_init.c >>>> > +++ b/platform/linux-dpdk/odp_platform.c >>>> > @@ -1,4 +1,4 @@ >>>> > -/* Copyright (c) 2013, Linaro Limited >>>> > +/* Copyright (c) 2014, Linaro Limited >>>> > * All rights reserved. >>>> > * >>>> > * SPDX-License-Identifier: BSD-3-Clause >>>> > @@ -9,7 +9,7 @@ >>>> > #include <odp_debug.h> >>>> > #include <odp_packet_dpdk.h> >>>> > >>>> > -int odp_init_dpdk(void) >>>> > +int odp_init_platform(odp_global_platform_init_t *platform_params >>>> > ODP_UNUSED) >>>> > { >>>> > int test_argc = 5; >>>> > char *test_argv[6]; >>>> > @@ -50,64 +50,7 @@ int odp_init_dpdk(void) >>>> > return 0; >>>> > } >>>> > >>>> > -int odp_init_global(void) >>>> > +int odp_crypto_init_global(void) >>>> >>>> Is crypto init now in odp_platform.c? It should be in odp_crypto.c or >>>> similar... >>>> >>> no problem >>> I can make a dummy odp_crypto.c for ks2 and point DPDK at the linux >>> generic. I was trying to avoid >>> connecting internals. >>> >>>> >>>> > { >>>> > - odp_thread_init_global(); >>>> > - >>>> > - odp_system_info_init(); >>>> > - >>>> > - if (odp_init_dpdk()) { >>>> > - ODP_ERR("ODP dpdk init failed.\n"); >>>> > - return -1; >>>> > - } >>>> > - >>>> > - if (odp_shm_init_global()) { >>>> > - ODP_ERR("ODP shm init failed.\n"); >>>> > - return -1; >>>> > - } >>>> > - >>>> > - if (odp_buffer_pool_init_global()) { >>>> > - ODP_ERR("ODP buffer pool init failed.\n"); >>>> > - return -1; >>>> > - } >>>> > - >>>> > - if (odp_queue_init_global()) { >>>> > - ODP_ERR("ODP queue init failed.\n"); >>>> > - return -1; >>>> > - } >>>> > - >>>> > - if (odp_schedule_init_global()) { >>>> > - ODP_ERR("ODP schedule init failed.\n"); >>>> > - return -1; >>>> > - } >>>> > - >>>> > - if (odp_pktio_init_global()) { >>>> > - ODP_ERR("ODP packet io init failed.\n"); >>>> > - return -1; >>>> > - } >>>> > - >>>> > - if (odp_timer_init_global()) { >>>> > - ODP_ERR("ODP timer init failed.\n"); >>>> > - return -1; >>>> > - } >>>> > - >>>> > - return 0; >>>> > -} >>>> > - >>>> > - >>>> > -int odp_init_local(int thr_id) >>>> > -{ >>>> > - odp_thread_init_local(thr_id); >>>> > - >>>> > - if (odp_pktio_init_local()) { >>>> > - ODP_ERR("ODP packet io local init failed.\n"); >>>> > - return -1; >>>> > - } >>>> > - >>>> > - if (odp_schedule_init_local()) { >>>> > - ODP_ERR("ODP schedule local init failed.\n"); >>>> > - return -1; >>>> > - } >>>> > - >>>> > return 0; >>>> > } >>>> > diff --git a/platform/linux-generic/Makefile.am b/platform/linux- >>>> > generic/Makefile.am >>>> > index f4dfdc1..c4aae9d 100644 >>>> > --- a/platform/linux-generic/Makefile.am >>>> > +++ b/platform/linux-generic/Makefile.am >>>> > @@ -62,6 +62,7 @@ __LIB__libodp_la_SOURCES = \ >>>> > odp_packet_flags.c \ >>>> > odp_packet_io.c \ >>>> > odp_packet_socket.c \ >>>> > + odp_platform.c \ >>>> > odp_queue.c \ >>>> > odp_ring.c \ >>>> > odp_rwlock.c \ >>>> > diff --git a/platform/linux-generic/include/api/odp_init.h >>>> > b/platform/linux-generic/include/api/odp_init.h >>>> > index 490324a..ece4547 100644 >>>> > --- a/platform/linux-generic/include/api/odp_init.h >>>> > +++ b/platform/linux-generic/include/api/odp_init.h >>>> > @@ -22,24 +22,42 @@ extern "C" { >>>> > >>>> > #include <odp_std_types.h> >>>> > >>>> > - >>>> > - >>>> > +/** ODP initialisation data. >>>> > + * Data that is required to initialize the ODP API with the >>>> > + * application specific data such as specifying a logging callback, >>>> the >>>> > log >>>> > + * level etc. >>>> > + */ >>>> > +typedef struct odp_global_init { >>>> >>>> The struct name should be "struct odp_global_init_t", "struct >>>> odp_global_init_s" or "struct odp_global_init_". >>>> >>>> The name without postfix (odp_global_init) should be reserved for >>>> variable names. >>>> no probelm >>>> >>> I will go with odp_init_s >>> >>> >>> >>>> > +} odp_global_init_t; >>>> >>>> In short, this could be named odp_init_t. It's the main init struct >>>> there will be. >>>> If there will be a local init struct it could be then >>>> odp_init_local_t... >>>> >>>> > + >>>> > +/** ODP platform initialization data. >>>> > + * @note ODP API does nothing with this data. It is the underlying >>>> > + * implementation requires it and any data passed here is not >>>> portable. >>>> > + * It is required that the application takes care of identifying and >>>> > + * passing any required platform specific data. >>>> > + */ >>>> > +typedef struct odp_global_platform_init { >>>> >>>> Same thing, rename struct. >>>> >>> no problem >>> odp_init_platform_s >>> >>>> >>>> > +} odp_platform_init_t; >>>> >>>> >>>> Same thing, odp_platform_init_t ?? >>>> >>>> >>>> > >>>> > /** >>>> > - * Perform global ODP initalisation. >>>> > - * >>>> > - * This function must be called once before calling >>>> > - * any other ODP API functions. >>>> > + * Perform global ODP initialisation. >>>> > * >>>> > + * This function must be called once before calling any other ODP API >>>> > + * functions. >>>> > + * @param[in] params Those parameters that are interpreted by the >>>> ODP API >>>> >>>> "Those parameters that are common to all ODP implementations. Specified >>>> in this API" >>>> >>>> API does not interpret, implementation does... >>>> >>> >>> Ok, I am struggling with names >>> >>> how about "platform SDK" for the underlying code such as DPDK, so that >>> the platform has an API >>> implementation - which is actually implemented in terms of a platform >>> SDK >>> >>> We need to distinguish that implementations should not use the platform >>> parameters unless the underlying >>> framework requires it, not that any implementation is free to just add >>> local stuff - or thats how it feels to me at least. >>> >>>> >>>> > + * @param[in] platform_params Those parameters that are passed >>>> without >>>> > + * interpretation by the ODP API to the implementation. >>>> >>>> " ... implementation specific parameter. Specified outside of ODP API" >>>> >>>> > * @return 0 if successful >>>> > */ >>>> > -int odp_init_global(void); >>>> > +int odp_init_global(odp_global_init_t *params, >>>> > + odp_global_platform_init_t *platform_params); >>>> > >>>> > >>>> > /** >>>> > - * Perform thread local ODP initalisation. >>>> > + * Perform thread local ODP initialisation. >>>> > * >>>> > * All threads must call this function before calling >>>> > + * >>>> >>>> Extra line feed? >>>> >>> Will fix >>> >>>> >>>> > * any other ODP API functions. >>>> > * @param thr_id Thread id >>>> > * @return 0 if successful >>>> > diff --git a/platform/linux-generic/include/odp_internal.h >>>> > b/platform/linux-generic/include/odp_internal.h >>>> > index aa79493..6c71fc9 100644 >>>> > --- a/platform/linux-generic/include/odp_internal.h >>>> > +++ b/platform/linux-generic/include/odp_internal.h >>>> > @@ -4,6 +4,7 @@ >>>> > * SPDX-License-Identifier: BSD-3-Clause >>>> > */ >>>> > >>>> > +#include <odp_init.h> >>>> > >>>> > /** >>>> > * @file >>>> > @@ -42,6 +43,8 @@ int odp_schedule_init_local(void); >>>> > int odp_timer_init_global(void); >>>> > int odp_timer_disarm_all(void); >>>> > >>>> > +int odp_init_platform(odp_global_platform_init_t *platform_params); >>>> > + >>>> > #ifdef __cplusplus >>>> > } >>>> > #endif >>>> > diff --git a/platform/linux-generic/odp_init.c b/platform/linux- >>>> > generic/odp_init.c >>>> > index 5b7e192..f595def 100644 >>>> > --- a/platform/linux-generic/odp_init.c >>>> > +++ b/platform/linux-generic/odp_init.c >>>> > @@ -8,13 +8,18 @@ >>>> > #include <odp_internal.h> >>>> > #include <odp_debug.h> >>>> > >>>> > - >>>> > -int odp_init_global(void) >>>> > +int odp_init_global(odp_global_init_t *params ODP_UNUSED, >>>> > + odp_global_platform_init_t *platform_params >>>> ODP_UNUSED) >>>> > { >>>> > odp_thread_init_global(); >>>> > >>>> > odp_system_info_init(); >>>> > >>>> > + if (odp_init_platform(platform_params)) { >>>> >>>> platform_params is used here, so remove ODP_UNUSED from argument list. >>>> >>>> On the other hand, it would be better to remove odp_init_platform() >>>> altogether since does not do anything. It's better to add it when >>>> actually used. >>>> The order of init calls is very important, e.g. now you could not >>>> allocate shared >>>> memory in odp_init_platform() because it's executed before shm init... >>>> >>> odp_init_platform() does do something for DPDK / KS2 and we need a >>> place holder because >>> we want this to be shared between implementations don't we ? >>> This is the stuff that must happen before any ODP init starts to satisfy >>> the platform SDK >>> >>>> >>>> - Petri >>>> >>>> > + ODP_ERR("ODP platform init failed.\n"); >>>> > + return -1; >>>> > + } >>>> > + >>>> > if (odp_shm_init_global()) { >>>> > ODP_ERR("ODP shm init failed.\n"); >>>> > return -1; >>>> > diff --git a/platform/linux-generic/odp_platform.c b/platform/linux- >>>> > generic/odp_platform.c >>>> > new file mode 100644 >>>> > index 0000000..a9efa7f >>>> > --- /dev/null >>>> > +++ b/platform/linux-generic/odp_platform.c >>>> > @@ -0,0 +1,14 @@ >>>> > +/* Copyright (c) 2014, Linaro Limited >>>> > + * All rights reserved. >>>> > + * >>>> > + * SPDX-License-Identifier: BSD-3-Clause >>>> > + */ >>>> > + >>>> > +#include <odp_init.h> >>>> > +#include <odp_internal.h> >>>> > +#include <odp_debug.h> >>>> > + >>>> > +int odp_init_platform(odp_global_platform_init_t *platform_params >>>> > ODP_UNUSED) >>>> > +{ >>>> > + return 0; >>>> > +} >>>> >>>> >>>> >>>> >>>> >>> >>> >>> -- >>> *Mike Holmes* >>> Linaro Technical Manager / Lead >>> LNG - ODP >>> >>> _______________________________________________ >>> lng-odp mailing list >>> [email protected] >>> http://lists.linaro.org/mailman/listinfo/lng-odp >>> >>> >> > > > -- > *Mike Holmes* > Linaro Technical Manager / Lead > LNG - ODP >
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
