The common set is option 1, which is what we have in may places, I dont like that we make it possible to reference the struct without the typedef, but unless we have further dissent we have a winner.I think.
Mike On 29 August 2014 13:28, William Mills <[email protected]> wrote: > OK, Yes I never say anything on this list but ... > > > On 08/29/2014 01:19 PM, Mike Holmes 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 ? >> >> > I wrote/collected our internal coding conventions and we insisted on a > name. The reason (at least at that time) was some debuggers showed the > structure name but not the typedef name. > > We used struct odp_<name>_tag {...} to emphasize that it should not be > used in the code. In retrospect I don't like that but either the _s or > _t above are OK. > > >> >> >> On 29 August 2014 13:10, Bill Fischofer <[email protected] >> <mailto:[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] <mailto:[email protected]>> wrote: >> >> >> >> >> On 29 August 2014 03:59, Savolainen, Petri (NSN - FI/Espoo) >> <[email protected] <mailto:[email protected]>> >> wrote: >> >> >> >> > -----Original Message----- >> > From: [email protected] >> <mailto:[email protected]> [mailto:lng-odp- >> <mailto:lng-odp-> >> > [email protected] >> <mailto:[email protected]>] On Behalf Of ext Mike >> Holmes >> > Sent: Thursday, August 28, 2014 9:26 PM >> > To: [email protected] >> <mailto:[email protected]> >> > Subject: [lng-odp] [PATCH v4] Add-global_init-paramiters >> > >> > Signed-off-by: Mike Holmes <[email protected] >> <mailto:[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] <mailto:[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 >> >> >> >> To unsubscribe from this group and stop receiving emails from it, send an >> email to [email protected]. >> >> -- *Mike Holmes* Linaro Technical Manager / Lead LNG - ODP
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
