Bill Mill's point about debuggers would seem to tip the argument back to option 1, which is what we have for the most part.
On Fri, Aug 29, 2014 at 12:34 PM, Mike Holmes <[email protected]> wrote: > 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
