| Message ID | 1418730191-26327-1-git-send-email-bill.fischofer@linaro.org |
|---|---|
| State | Accepted |
| Commit | b6b0d41eeff78fc67a65e3acf5fc8b73ae33d668 |
| Headers | show |
Taras, please review that patch. Maxim. On 12/16/2014 02:43 PM, Bill Fischofer wrote: > Change odp_buffer_pool_info() output to use ODP_SHM_INVALID instead of > ODP_SHM_NULL. > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > --- > platform/linux-generic/include/api/odp_buffer_pool.h | 2 +- > platform/linux-generic/odp_buffer_pool.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/platform/linux-generic/include/api/odp_buffer_pool.h b/platform/linux-generic/include/api/odp_buffer_pool.h > index 4da5f84..8380ac1 100644 > --- a/platform/linux-generic/include/api/odp_buffer_pool.h > +++ b/platform/linux-generic/include/api/odp_buffer_pool.h > @@ -117,7 +117,7 @@ typedef struct odp_buffer_pool_info_t { > odp_shm_t shm; /**< handle of shared memory area > supplied by application to > contain buffer pool, or > - ODP_SHM_NULL if this pool is > + ODP_SHM_INVALID if this pool is > managed by ODP */ > odp_buffer_pool_param_t params; /**< pool parameters */ > } odp_buffer_pool_info_t; > diff --git a/platform/linux-generic/odp_buffer_pool.c b/platform/linux-generic/odp_buffer_pool.c > index 4c8b037..2d2aba2 100644 > --- a/platform/linux-generic/odp_buffer_pool.c > +++ b/platform/linux-generic/odp_buffer_pool.c > @@ -401,7 +401,7 @@ int odp_buffer_pool_info(odp_buffer_pool_t pool_hdl, > > info->name = pool->s.name; > info->shm = pool->s.flags.user_supplied_shm ? > - pool->s.pool_shm : ODP_SHM_NULL; > + pool->s.pool_shm : ODP_SHM_INVALID; > info->params.buf_size = pool->s.params.buf_size; > info->params.buf_align = pool->s.params.buf_align; > info->params.num_bufs = pool->s.params.num_bufs;
On 12/16/2014 01:43 PM, Bill Fischofer wrote: > Change odp_buffer_pool_info() output to use ODP_SHM_INVALID instead of > ODP_SHM_NULL. > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> To follow our naming convention it should be named: api: buffer: Change odp_buffer_pool_info_t output Otherwise Reviewed-by: Taras Kondratiuk <taras.kondratiuk@linaro.org> Should ODP_SHM_NULL definition be removed completely now from API?
On 12/18/2014 05:41 PM, Savolainen, Petri (NSN - FI/Espoo) wrote: > > >> -----Original Message----- >> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- >> bounces@lists.linaro.org] On Behalf Of ext Taras Kondratiuk >> Sent: Thursday, December 18, 2014 4:36 PM >> To: Bill Fischofer; lng-odp@lists.linaro.org >> Subject: Re: [lng-odp] [PATCH] Documentation: Change >> odp_buffer_pool_info_t output >> >> On 12/16/2014 01:43 PM, Bill Fischofer wrote: >>> Change odp_buffer_pool_info() output to use ODP_SHM_INVALID instead of >>> ODP_SHM_NULL. >>> >>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >> >> To follow our naming convention it should be named: >> api: buffer: Change odp_buffer_pool_info_t output >> >> Otherwise >> Reviewed-by: Taras Kondratiuk <taras.kondratiuk@linaro.org> >> >> Should ODP_SHM_NULL definition be removed completely now from API? > > No. The idea is that user input param is XXX_NULL if the param is optional and the user don't specify a handle. Output from ODP would be always XXX_INVALID or valid handle. > > > shm = odp_shm_reserve(...) // returns INVALID > > odp_buffer_pool_create(..., shm, ...); // I care but didn't check the shm output > > vs. > > odp_buffer_pool_create(..., ODP_SHM_NULL, ...); // I don't care Sorry if it was already discussed before, but what is the advantage of having both NULL and INVALID? Why not to leave only one of them? I assume on most of platforms they will anyway map to the same value.
On 18 December 2014 at 16:47, Taras Kondratiuk <taras.kondratiuk@linaro.org> wrote: > On 12/18/2014 05:41 PM, Savolainen, Petri (NSN - FI/Espoo) wrote: >> >> >> >>> -----Original Message----- >>> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- >>> bounces@lists.linaro.org] On Behalf Of ext Taras Kondratiuk >>> Sent: Thursday, December 18, 2014 4:36 PM >>> To: Bill Fischofer; lng-odp@lists.linaro.org >>> Subject: Re: [lng-odp] [PATCH] Documentation: Change >>> odp_buffer_pool_info_t output >>> >>> On 12/16/2014 01:43 PM, Bill Fischofer wrote: >>>> >>>> Change odp_buffer_pool_info() output to use ODP_SHM_INVALID instead of >>>> ODP_SHM_NULL. >>>> >>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >>> >>> >>> To follow our naming convention it should be named: >>> api: buffer: Change odp_buffer_pool_info_t output >>> >>> Otherwise >>> Reviewed-by: Taras Kondratiuk <taras.kondratiuk@linaro.org> >>> >>> Should ODP_SHM_NULL definition be removed completely now from API? >> >> >> No. The idea is that user input param is XXX_NULL if the param is optional >> and the user don't specify a handle. Output from ODP would be always >> XXX_INVALID or valid handle. >> >> >> shm = odp_shm_reserve(...) // returns INVALID >> >> odp_buffer_pool_create(..., shm, ...); // I care but didn't check the shm >> output >> >> vs. >> >> odp_buffer_pool_create(..., ODP_SHM_NULL, ...); // I don't care > > > Sorry if it was already discussed before, but what is the advantage of > having both NULL and INVALID? Why not to leave only one of them? > I assume on most of platforms they will anyway map to the same value. If Petri's example above should work (as I expect it to work), they cannot have the same value. I think it is confusing to provide both a _NULL and an _INVALID symbol. Perhaps the _NULL symbol should be renamed to _DEFAULT or something else that is semantically far away from _INVALID. The odp_buffer_pool_create() function could also specify that ODP_SHM_INVALID has the meaning of do default behavior. If the user is too lazy to check the return value of odp_shm_reserve() and unexpectedly passes ODP_SHM_INVALID, it could be viewed as their problem. > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
There is no architectural requirement that they be distinct and, as Raras noted, most implementations will just make them synonyms. That's why the patch is classified as a documentation change--there's no actual change in observable behavior. Bill On Thursday, December 18, 2014, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > On 18 December 2014 at 16:47, Taras Kondratiuk > <taras.kondratiuk@linaro.org <javascript:;>> wrote: > > On 12/18/2014 05:41 PM, Savolainen, Petri (NSN - FI/Espoo) wrote: > >> > >> > >> > >>> -----Original Message----- > >>> From: lng-odp-bounces@lists.linaro.org <javascript:;> [mailto:lng-odp- > <javascript:;> > >>> bounces@lists.linaro.org <javascript:;>] On Behalf Of ext Taras > Kondratiuk > >>> Sent: Thursday, December 18, 2014 4:36 PM > >>> To: Bill Fischofer; lng-odp@lists.linaro.org <javascript:;> > >>> Subject: Re: [lng-odp] [PATCH] Documentation: Change > >>> odp_buffer_pool_info_t output > >>> > >>> On 12/16/2014 01:43 PM, Bill Fischofer wrote: > >>>> > >>>> Change odp_buffer_pool_info() output to use ODP_SHM_INVALID instead of > >>>> ODP_SHM_NULL. > >>>> > >>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org > <javascript:;>> > >>> > >>> > >>> To follow our naming convention it should be named: > >>> api: buffer: Change odp_buffer_pool_info_t output > >>> > >>> Otherwise > >>> Reviewed-by: Taras Kondratiuk <taras.kondratiuk@linaro.org > <javascript:;>> > >>> > >>> Should ODP_SHM_NULL definition be removed completely now from API? > >> > >> > >> No. The idea is that user input param is XXX_NULL if the param is > optional > >> and the user don't specify a handle. Output from ODP would be always > >> XXX_INVALID or valid handle. > >> > >> > >> shm = odp_shm_reserve(...) // returns INVALID > >> > >> odp_buffer_pool_create(..., shm, ...); // I care but didn't check the > shm > >> output > >> > >> vs. > >> > >> odp_buffer_pool_create(..., ODP_SHM_NULL, ...); // I don't care > > > > > > Sorry if it was already discussed before, but what is the advantage of > > having both NULL and INVALID? Why not to leave only one of them? > > I assume on most of platforms they will anyway map to the same value. > If Petri's example above should work (as I expect it to work), they > cannot have the same value. > > I think it is confusing to provide both a _NULL and an _INVALID > symbol. Perhaps the _NULL symbol should be renamed to _DEFAULT or > something else that is semantically far away from _INVALID. > > The odp_buffer_pool_create() function could also specify that > ODP_SHM_INVALID has the meaning of do default behavior. If the user is > too lazy to check the return value of odp_shm_reserve() and > unexpectedly passes ODP_SHM_INVALID, it could be viewed as their > problem. > > > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org <javascript:;> > > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 18 December 2014 at 19:54, Bill Fischofer <bill.fischofer@linaro.org> wrote: > There is no architectural requirement that they be distinct and, as Raras > noted, most implementations will just make them synonyms. That's why the > patch is classified as a documentation change--there's no actual change in > observable behavior. If the semantics is the same, why then not have only one symbolic constant? How is the architecture better by having two symbolic constants that mean the same? > > Bill > > On Thursday, December 18, 2014, Ola Liljedahl <ola.liljedahl@linaro.org> > wrote: >> >> On 18 December 2014 at 16:47, Taras Kondratiuk >> <taras.kondratiuk@linaro.org> wrote: >> > On 12/18/2014 05:41 PM, Savolainen, Petri (NSN - FI/Espoo) wrote: >> >> >> >> >> >> >> >>> -----Original Message----- >> >>> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- >> >>> bounces@lists.linaro.org] On Behalf Of ext Taras Kondratiuk >> >>> Sent: Thursday, December 18, 2014 4:36 PM >> >>> To: Bill Fischofer; lng-odp@lists.linaro.org >> >>> Subject: Re: [lng-odp] [PATCH] Documentation: Change >> >>> odp_buffer_pool_info_t output >> >>> >> >>> On 12/16/2014 01:43 PM, Bill Fischofer wrote: >> >>>> >> >>>> Change odp_buffer_pool_info() output to use ODP_SHM_INVALID instead >> >>>> of >> >>>> ODP_SHM_NULL. >> >>>> >> >>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >> >>> >> >>> >> >>> To follow our naming convention it should be named: >> >>> api: buffer: Change odp_buffer_pool_info_t output >> >>> >> >>> Otherwise >> >>> Reviewed-by: Taras Kondratiuk <taras.kondratiuk@linaro.org> >> >>> >> >>> Should ODP_SHM_NULL definition be removed completely now from API? >> >> >> >> >> >> No. The idea is that user input param is XXX_NULL if the param is >> >> optional >> >> and the user don't specify a handle. Output from ODP would be always >> >> XXX_INVALID or valid handle. >> >> >> >> >> >> shm = odp_shm_reserve(...) // returns INVALID >> >> >> >> odp_buffer_pool_create(..., shm, ...); // I care but didn't check the >> >> shm >> >> output >> >> >> >> vs. >> >> >> >> odp_buffer_pool_create(..., ODP_SHM_NULL, ...); // I don't care >> > >> > >> > Sorry if it was already discussed before, but what is the advantage of >> > having both NULL and INVALID? Why not to leave only one of them? >> > I assume on most of platforms they will anyway map to the same value. >> If Petri's example above should work (as I expect it to work), they >> cannot have the same value. >> >> I think it is confusing to provide both a _NULL and an _INVALID >> symbol. Perhaps the _NULL symbol should be renamed to _DEFAULT or >> something else that is semantically far away from _INVALID. >> >> The odp_buffer_pool_create() function could also specify that >> ODP_SHM_INVALID has the meaning of do default behavior. If the user is >> too lazy to check the return value of odp_shm_reserve() and >> unexpectedly passes ODP_SHM_INVALID, it could be viewed as their >> problem. >> > >> > >> > _______________________________________________ >> > lng-odp mailing list >> > lng-odp@lists.linaro.org >> > http://lists.linaro.org/mailman/listinfo/lng-odp
That's not a question I can answer. I'm just implementing what was specified. :) On Thu, Dec 18, 2014 at 1:27 PM, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > > On 18 December 2014 at 19:54, Bill Fischofer <bill.fischofer@linaro.org> > wrote: > > There is no architectural requirement that they be distinct and, as Raras > > noted, most implementations will just make them synonyms. That's why the > > patch is classified as a documentation change--there's no actual change > in > > observable behavior. > If the semantics is the same, why then not have only one symbolic > constant? How is the architecture better by having two symbolic > constants that mean the same? > > > > > Bill > > > > On Thursday, December 18, 2014, Ola Liljedahl <ola.liljedahl@linaro.org> > > wrote: > >> > >> On 18 December 2014 at 16:47, Taras Kondratiuk > >> <taras.kondratiuk@linaro.org> wrote: > >> > On 12/18/2014 05:41 PM, Savolainen, Petri (NSN - FI/Espoo) wrote: > >> >> > >> >> > >> >> > >> >>> -----Original Message----- > >> >>> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- > >> >>> bounces@lists.linaro.org] On Behalf Of ext Taras Kondratiuk > >> >>> Sent: Thursday, December 18, 2014 4:36 PM > >> >>> To: Bill Fischofer; lng-odp@lists.linaro.org > >> >>> Subject: Re: [lng-odp] [PATCH] Documentation: Change > >> >>> odp_buffer_pool_info_t output > >> >>> > >> >>> On 12/16/2014 01:43 PM, Bill Fischofer wrote: > >> >>>> > >> >>>> Change odp_buffer_pool_info() output to use ODP_SHM_INVALID instead > >> >>>> of > >> >>>> ODP_SHM_NULL. > >> >>>> > >> >>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > >> >>> > >> >>> > >> >>> To follow our naming convention it should be named: > >> >>> api: buffer: Change odp_buffer_pool_info_t output > >> >>> > >> >>> Otherwise > >> >>> Reviewed-by: Taras Kondratiuk <taras.kondratiuk@linaro.org> > >> >>> > >> >>> Should ODP_SHM_NULL definition be removed completely now from API? > >> >> > >> >> > >> >> No. The idea is that user input param is XXX_NULL if the param is > >> >> optional > >> >> and the user don't specify a handle. Output from ODP would be always > >> >> XXX_INVALID or valid handle. > >> >> > >> >> > >> >> shm = odp_shm_reserve(...) // returns INVALID > >> >> > >> >> odp_buffer_pool_create(..., shm, ...); // I care but didn't check the > >> >> shm > >> >> output > >> >> > >> >> vs. > >> >> > >> >> odp_buffer_pool_create(..., ODP_SHM_NULL, ...); // I don't care > >> > > >> > > >> > Sorry if it was already discussed before, but what is the advantage of > >> > having both NULL and INVALID? Why not to leave only one of them? > >> > I assume on most of platforms they will anyway map to the same value. > >> If Petri's example above should work (as I expect it to work), they > >> cannot have the same value. > >> > >> I think it is confusing to provide both a _NULL and an _INVALID > >> symbol. Perhaps the _NULL symbol should be renamed to _DEFAULT or > >> something else that is semantically far away from _INVALID. > >> > >> The odp_buffer_pool_create() function could also specify that > >> ODP_SHM_INVALID has the meaning of do default behavior. If the user is > >> too lazy to check the return value of odp_shm_reserve() and > >> unexpectedly passes ODP_SHM_INVALID, it could be viewed as their > >> problem. > >> > > >> > > >> > _______________________________________________ > >> > lng-odp mailing list > >> > lng-odp@lists.linaro.org > >> > http://lists.linaro.org/mailman/listinfo/lng-odp >
Merged to staging with patch name correction as Taras said. Maxim. On 12/16/2014 02:43 PM, Bill Fischofer wrote: > Change odp_buffer_pool_info() output to use ODP_SHM_INVALID instead of > ODP_SHM_NULL. > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > --- > platform/linux-generic/include/api/odp_buffer_pool.h | 2 +- > platform/linux-generic/odp_buffer_pool.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/platform/linux-generic/include/api/odp_buffer_pool.h b/platform/linux-generic/include/api/odp_buffer_pool.h > index 4da5f84..8380ac1 100644 > --- a/platform/linux-generic/include/api/odp_buffer_pool.h > +++ b/platform/linux-generic/include/api/odp_buffer_pool.h > @@ -117,7 +117,7 @@ typedef struct odp_buffer_pool_info_t { > odp_shm_t shm; /**< handle of shared memory area > supplied by application to > contain buffer pool, or > - ODP_SHM_NULL if this pool is > + ODP_SHM_INVALID if this pool is > managed by ODP */ > odp_buffer_pool_param_t params; /**< pool parameters */ > } odp_buffer_pool_info_t; > diff --git a/platform/linux-generic/odp_buffer_pool.c b/platform/linux-generic/odp_buffer_pool.c > index 4c8b037..2d2aba2 100644 > --- a/platform/linux-generic/odp_buffer_pool.c > +++ b/platform/linux-generic/odp_buffer_pool.c > @@ -401,7 +401,7 @@ int odp_buffer_pool_info(odp_buffer_pool_t pool_hdl, > > info->name = pool->s.name; > info->shm = pool->s.flags.user_supplied_shm ? > - pool->s.pool_shm : ODP_SHM_NULL; > + pool->s.pool_shm : ODP_SHM_INVALID; > info->params.buf_size = pool->s.params.buf_size; > info->params.buf_align = pool->s.params.buf_align; > info->params.num_bufs = pool->s.params.num_bufs;
diff --git a/platform/linux-generic/include/api/odp_buffer_pool.h b/platform/linux-generic/include/api/odp_buffer_pool.h index 4da5f84..8380ac1 100644 --- a/platform/linux-generic/include/api/odp_buffer_pool.h +++ b/platform/linux-generic/include/api/odp_buffer_pool.h @@ -117,7 +117,7 @@ typedef struct odp_buffer_pool_info_t { odp_shm_t shm; /**< handle of shared memory area supplied by application to contain buffer pool, or - ODP_SHM_NULL if this pool is + ODP_SHM_INVALID if this pool is managed by ODP */ odp_buffer_pool_param_t params; /**< pool parameters */ } odp_buffer_pool_info_t; diff --git a/platform/linux-generic/odp_buffer_pool.c b/platform/linux-generic/odp_buffer_pool.c index 4c8b037..2d2aba2 100644 --- a/platform/linux-generic/odp_buffer_pool.c +++ b/platform/linux-generic/odp_buffer_pool.c @@ -401,7 +401,7 @@ int odp_buffer_pool_info(odp_buffer_pool_t pool_hdl, info->name = pool->s.name; info->shm = pool->s.flags.user_supplied_shm ? - pool->s.pool_shm : ODP_SHM_NULL; + pool->s.pool_shm : ODP_SHM_INVALID; info->params.buf_size = pool->s.params.buf_size; info->params.buf_align = pool->s.params.buf_align; info->params.num_bufs = pool->s.params.num_bufs;
Change odp_buffer_pool_info() output to use ODP_SHM_INVALID instead of ODP_SHM_NULL. Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> --- platform/linux-generic/include/api/odp_buffer_pool.h | 2 +- platform/linux-generic/odp_buffer_pool.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)