sds: add cfl_sds_snprintf

[nokute78]

This patch is porting flb_sds_snprintf and its test code. https://github.com/fluent/fluent-bit/blob/v2.1.3/src/flb_sds.c#L477

The discussion of this function https://github.com/fluent/fluent-bit/pull/4361#discussion_r758531036

[leonardo-albertovich]

If I understand that conversation correctly, the reason why we have flb_sds_snprintf is because flb_sds_printf appends the formatted string to the flb_sds_t instance right? Please correct me if I'm wrong.

If that's the case, then what we should do is take this opportunity to improve the clarity of the situation by renaming the existing cfl_sds_printf to cfl_sds_cat_printf or cfl_sds_printf_cat and then defining a new function named cfl_sds_printf which uses cfl_sds_len_set to truncate the destination buffer before writing the formatted data.

Of course in this case we would have to check all of our code to issue the appropriate updates but as far as I can see the only code that relies on that behavior is the ctraces text encoder which would be trivial to update (and not critical by any means).

This is not just me being pedantic but rather making the most of this opportunity to make our code clearer both when it comes to what the naming conveys and by minimizing code duplication.

[nokute78]

If I understand that conversation correctly, the reason why we have flb_sds_snprintf is because flb_sds_printf appends the formatted string to the flb_sds_t instance right?

Yes it is the reason.

we should do is take this opportunity to improve the clarity of the situation by renaming the existing cfl_sds_printf

I don't know how many codes will be affected.

In my understanding, fluent-bit will replace to call cflsds* API in the future, is it right ? Currently, so many codes of fluent-bit uses flb_sds_printf. I'm afraid of drastic changing API ..

taka@taka-VirtualBox:~/git/fluent-bit$ pwd
/home/taka/git/fluent-bit
taka@taka-VirtualBox:~/git/fluent-bit$ find . -name "*.[ch]" |xargs grep flb_sds_printf | wc
    169     905   15782

[leonardo-albertovich]

It's true that a lot of code uses flb_sds_printf but not a lot of code uses cfl_sds_printf so we can make that first step and leave a warning or something so when we make the system wide change it doesn't cause trouble.

[nokute78]

How about creating another PR to rename cfl_sds_printf ?

This patch is to adding cfl_sds_snprintf and this API is intended to have the same arguments and return values as snprintf. The difference is the first argument type is cfl_sds_t *.

I think cfl_sds_printf is not proper name for this API since user may expect outputting to stdout.

[leonardo-albertovich]

I would rather hold off and make all the changes at once since otherwise we'd be opening the window for the proliferation of the use of something that shouldn't be used otherwise.

[nokute78]

then defining a new function named cfl_sds_printf which uses cfl_sds_len_set to truncate the destination buffer before writing the formatted data.

In my understanding, the function prototype is not changed and it means breaking change of cfl_sds_printf. It is expected that the effects of mis-calls of cfl_sds_cat_printf and cfl_sds_printf are unlikely to be apparent.

It is hard for me to guarantee proper renaming.

[leonardo-albertovich]

We could guarantee it by searching through the code bases that use the library (which is a glorified grep) but if that's not possible then we need to drop the old name and choose two new names, test it and deal with whatever breaks.

[nokute78]

if that's not possible then we need to drop the old name and choose two new names, test it and deal with whatever breaks

We will need this choosing when updating cfl on fluent-bit and renaming flb_sds_* APIs to cfl_sds_* APIs on fluent-bit. I'm concerned about latter replacing and that's why I asked

In my understanding, fluent-bit will replace to call cflsds* API in the future, is it right ?

There are 169 lines using flb_sds_printf. e.g. in_otel uses flb_sds_printf to construct HTTP response. https://github.com/fluent/fluent-bit/blob/v2.1.4/plugins/in_opentelemetry/opentelemetry_prot.c#L81

Currently there is no test code for in_otel at v2.1.4. https://github.com/fluent/fluent-bit/tree/v2.1.4/tests/runtime

For backward compatibility, I recommend keeping cfl_sds_printf or defines such function/macro to mitigate breaking change.

cfl_sds_t cfl_sds_printf(cfl_sds_t *sds, const char *fmt, ...)
{
#ifdef DEBUG // or something
    printf("%s this function is obsolete. Please call cfl_sds_cat_printf or ...\n", __FUNCTION__);
#endif
    // call cfl_sds_cat_printf();
   ...
}

[leonardo-albertovich]

It's easier to rip the band-aid once and mindfully pay the price than to waste this opportunity and make it harder for whomever takes on this task in the future.

[edsiper]

If I understand that conversation correctly, the reason why we have flb_sds_snprintf is because flb_sds_printf appends the formatted string to the flb_sds_t instance right? Please correct me if I'm wrong.

If that's the case, then what we should do is take this opportunity to improve the clarity of the situation by renaming the existing cfl_sds_printf to cfl_sds_cat_printf or cfl_sds_printf_cat and then defining a new function named cfl_sds_printf which uses cfl_sds_len_set to truncate the destination buffer before writing the formatted data.

I agree with that approach. actually if we look at the original SDS library that I stole from Redis we found similar logic:

https://github.com/redis/redis/blob/unstable/src/sds.c#L544

not sure about migrating old code, but definitely we should encourage best practices for new code that gets in.

yes, changing this here will require some fixes in fluent bit otel plugin, but it seems pretty straightforward

@nokute78 pls confirm if you can work on the requested changes.