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.

Sign up for GitHub

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
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions plugins/out_stackdriver/stackdriver.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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)

int test_log_entry_format;

flb_sds_t stackdriver_agent;

Expand Down
Loading