Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

out_stackdriver: fixed a wrong data type used for two boolean options #9222

Merged
merged 1 commit into from
Aug 15, 2024

Conversation

leonardo-albertovich
Copy link
Collaborator

No description provided.

@@ -183,8 +183,8 @@ struct flb_stackdriver {
flb_sds_t log_name_key;
flb_sds_t http_request_key;
int http_request_key_size;
bool autoformat_stackdriver_trace;
bool test_log_entry_format;
int autoformat_stackdriver_trace;
Copy link
Contributor

@igorpeshansky igorpeshansky Aug 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this specifically because of the use of FLB_CONFIG_MAP_BOOL? Or some general guidance against using the bool type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remembered something about the C90 standard being preferred (bool was introduced in C99) but I couldn't find a reference to it in fluent-bits coding style (or the apache coding style).

In this case it's purely limited to FLB_CONFIG_MAP_BOOL (see PR #8904 and the related work).

Copy link

@Charles1000Chen Charles1000Chen Aug 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leonardo-albertovich As we can see that the current fluent-bit code is already C99 standard because we are using the 'inline', 'long long int'... which are introduced from C99 standard. Just curious why we use int to storing data of FLB_CONFIG_MAP_BOOL instead of one-byte data type like bool or char? The one-byte data type can work well in both LE(x86) and BE(s390x) systems. In fluent_config_map.h, we declare char to store the value of FLB_CONFIG_MAP_BOOL.

struct flb_config_map_val {
    union {
        int i_num;                    /* FLB_CONFIG_MAP_INT */
        char boolean;                 /* FLB_CONFIG_MAP_BOOL */
        double d_num;                 /* FLB_CONFIG_MAP_DOUBLE */
        size_t s_num;                 /* FLB_CONFIG_MAP_SIZE */
        flb_sds_t str;                /* FLB_CONFIG_MAP_STR */
        struct mk_list *list;         /* FLB_CONFIG_MAP_CLIST and FLB_CONFIG_MAP_SLIST */
        struct cfl_variant *variant;  /* FLB_CONFIG_MAP_VARIANT */
    }

But in src/flb_config_map.c 8904 and plugins, we turn to use int to store data of FLB_CONFIG_MAP_BOOL, it's confusing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm good point, it's confusing. You are right, we are in C99 based on our use of inline.

Since there are several patches around this right now, I would suggest to continue using int and for Fluent Bit v3.2 move formally to bool (we will move to v3.2 dev shortly)

@edsiper edsiper merged commit db9ceb5 into master Aug 15, 2024
45 checks passed
@edsiper edsiper deleted the leonardo-master-stackdriver_property_type_fix branch August 15, 2024 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants