I do like having the _s as it is more readable IMO. What happens when you have
a self reference in another structure.
#1
typedef struct odp_<name>_t {
struct odp_<name>_t * name;
} odp_<name>_t;
OR #2
typedef struct odp_<name>_s {
struct odp_<name>_s * name;
} odp_<name>_t;
OR #3
typedef struct odp_<name> {
struct odp_<name> * name;
} odp_<name>_t;
I feel like I am getting my eyes check is one better or two better or one
better or three better :-)
I vote for #2 as it is the cleanest and most readable, we should not rely on
the fact that structure name and typedef name are in different name spaces.
On 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
Keith Wiles, Principal Technologist with CTO office, Wind River mobile
972-213-5533
_______________________________________________
lng-odp mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/lng-odp
