Re: [PATCH] mempool: adjust name string size in related data types

[Zoltan Kiss <zoltan.kiss@linaro.org>]

On 19/07/16 16:37, Olivier Matz wrote:
> Hi Zoltan,
>
> On 07/19/2016 04:37 PM, Zoltan Kiss wrote:
>> A recent fix brought up an issue about the size of the 'name' fields:
>>
>> 85cf0079 mem: avoid memzone/mempool/ring name truncation
>>
>> These relations should be observed:
>>
>> RTE_RING_NAMESIZE <= RTE_MEMZONE_NAMESIZE - strlen(RTE_RING_MZ_PREFIX)
>> RTE_MEMPOOL_NAMESIZE <= RTE_RING_NAMESIZE - strlen(RTE_MEMPOOL_MZ_PREFIX)
>>
>> Setting all of them to 32 hides this restriction from the application.
>> This patch increases the memzone string size to accomodate for these
>> prefixes, and the same happens with the ring name string. The ABI needs to
>> be broken to fix this API issue, this way doesn't break applications
>> previously not failing due to the truncating bug now fixed.
>>
>> Signed-off-by: Zoltan Kiss <zoltan.kiss@schaman.hu>
>
> I agree it is a problem for an application because it cannot know what
> is the maximum name length. On the other hand, breaking the ABI for this
> looks a bit overkill. Maybe we could reduce RTE_MEMPOOL_NAMESIZE and
> RTE_RING_NAMESIZE instead of increasing RTE_MEMZONE_NAMESIZE? That way,
> we could keep the ABI as is.

But that would break the ABI too, wouldn't it? Unless you keep the array 
the same size (32 bytes) by using RTE_MEMZONE_NAMESIZE.
And even then, the API breaks anyway. There are applications - I have at 
least some - which use all 32 bytes to store the name. Decrease that 
would cause headache to change the naming scheme, because it's a 30 
character long id, and chopping the last few chars would cause name 
collisions and annoying bugs.

>
> It would even be better to get rid of this static char[] for the
> structure names and replace it by an allocated const char *. I didn't
> check it's feasible for memzones. What do you think?

It would work too, but I don't think it would help a lot. We would still 
need max sizes for the names. Storing them somewhere else won't help us 
in this problem.

>
> In any case, I think it's a bit late for 16.07 for this kind of fix.
>
> Regards,
> Olivier
>