>-----Original Message-----
>From: Jerin Jacob <jerinjac...@gmail.com>
>Sent: Wednesday 5 May 2021 11:25
>To: David Marchand <david.march...@redhat.com>
>Cc: Power, Ciara <ciara.po...@intel.com>; Richardson, Bruce
><bruce.richard...@intel.com>; jer...@marvell.com; dev@dpdk.org;
>tho...@monjalon.net
>Subject: Re: [dpdk-dev] [PATCH] telemetry: remove internal symbol from
>public header
>
>On Wed, May 5, 2021 at 2:14 PM David Marchand
><david.march...@redhat.com> wrote:
>>
>> On Tue, May 4, 2021 at 6:19 PM Power, Ciara <ciara.po...@intel.com>
>wrote:
>> >
>> > >-----Original Message-----
>> > >From: Richardson, Bruce <bruce.richard...@intel.com>
>> > >Sent: Tuesday 4 May 2021 13:14
>> > >To: jer...@marvell.com
>> > >Cc: Power, Ciara <ciara.po...@intel.com>; dev@dpdk.org;
>> > >tho...@monjalon.net
>> > >Subject: Re: [PATCH] telemetry: remove internal symbol from public
>> > >header
>> > >
>> > >On Mon, May 03, 2021 at 10:04:28PM +0530, jer...@marvell.com wrote:
>> > >> From: Jerin Jacob <jer...@marvell.com>
>> > >>
>> > >> Remove TELEMETRY_MAX_CALLBACKS symbol from public
>rte_telemetry.h
>> > >> header file.
>> > >>
>> > >> Signed-off-by: Jerin Jacob <jer...@marvell.com>
>> > >> ---
>> > >Acked-by: Bruce Richardson <bruce.richard...@intel.com>
>> >
>> > Thanks,
>> > Acked-by: Ciara Power <ciara.po...@intel.com>
>>
>> I agree this define should be hidden.
>>
>> Just, what do you think of using a dynamic allocation and remove the
>> limitation entirely?
>
>I think, that may be better. Probably this patch can go in rc2.
>
>The dynamic feature can be pushed to the next release.
>
>I will leave it to the maintainers to decide. I am fine with any scheme.
>
+1 for the dynamic allocation idea.
I am ok with using this small fix patch for now and making the change to
dynamic allocation at a later stage,
if it isn't suitable at this point in the release cycle.
>>
>>
>> diff --git a/lib/telemetry/rte_telemetry.h
>> b/lib/telemetry/rte_telemetry.h index 031db9e968..8776998b54 100644
>> --- a/lib/telemetry/rte_telemetry.h
>> +++ b/lib/telemetry/rte_telemetry.h
>> @@ -10,8 +10,6 @@
>> #ifndef _RTE_TELEMETRY_H_
>> #define _RTE_TELEMETRY_H_
>>
>> -/** Maximum number of telemetry callbacks. */ -#define
>> TELEMETRY_MAX_CALLBACKS 64
>> /** Maximum length for string used in object. */ #define
>> RTE_TEL_MAX_STRING_LEN 64
>> /** Maximum length of string. */
>> @@ -285,7 +283,7 @@ typedef void * (*handler)(void *sock_id);
>> * @return
>> * -EINVAL for invalid parameters failure.
>> * @return
>> - * -ENOENT if max callbacks limit has been reached.
>> + * -ENOMEM for mem allocation failure.
>> */
>> __rte_experimental
>> int
>> diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
>> index 68b479e0e4..6baba57ec2 100644
>> --- a/lib/telemetry/telemetry.c
>> +++ b/lib/telemetry/telemetry.c
>> @@ -59,7 +59,7 @@ static uint32_t logtype;
>> rte_log_ptr(RTE_LOG_ ## l, logtype, "TELEMETRY: "
>> __VA_ARGS__)
>>
>> /* list of command callbacks, with one command registered by default
>> */ -static struct cmd_callback callbacks[TELEMETRY_MAX_CALLBACKS];
>> +static struct cmd_callback *callbacks;
>> static int num_callbacks; /* How many commands are registered */
>> /* Used when accessing or modifying list of command callbacks */
>> static rte_spinlock_t callback_sl = RTE_SPINLOCK_INITIALIZER; @@
>> -70,15 +70,21 @@ static uint16_t v2_clients; int
>> rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const
>> char *help) {
>> + struct cmd_callback *new_callbacks;
>> int i = 0;
>>
>> if (strlen(cmd) >= MAX_CMD_LEN || fn == NULL || cmd[0] != '/'
>> || strlen(help) >= MAX_HELP_LEN)
>> return -EINVAL;
>> - if (num_callbacks >= TELEMETRY_MAX_CALLBACKS)
>> - return -ENOENT;
>>
>> rte_spinlock_lock(&callback_sl);
>> + new_callbacks = realloc(callbacks, sizeof(callbacks[0]) *
>> (num_callbacks + 1));
>> + if (new_callbacks == NULL) {
>> + rte_spinlock_unlock(&callback_sl);
>> + return -ENOMEM;
>> + }
>> + callbacks = new_callbacks;
>> +
>> while (i < num_callbacks && strcmp(cmd, callbacks[i].cmd) > 0)
>> i++;
>> if (i != num_callbacks)
>>
>>
>> And there is a race to fix in list_commands() (which accesses the
>> callbacks array without taking the lock).
>>
>> --
>> David Marchand
>>
Good catch, I can send a fix patch for this list commands function. Thanks
David.
- Ciara
