core: fix configuration type cast issue on big endian systems #8904
+5
−5
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rightblank
requested review from
edsiper,
leonardo-albertovich,
fujimotos and
koleini
as code owners
|
Hi, @cosmo0920 @edsiper , could you please help to review this PR, it fixes the issues reported in #8828 |
rightblank
changed the title
rightblank
commented
Jun 17, 2024
src/flb_config_map.c
Outdated
| @@ -649,10 +649,10 @@ int flb_config_map_set(struct mk_list *properties, struct mk_list *map, void *co | |||
| } | |||
| else if (m->type == FLB_CONFIG_MAP_TIME) { | |||
| m_i_num = (int *) (base + m->offset); | |||
| *m_i_num = m->value.val.s_num; | |||
| *m_i_num = m->value.val.i_num; | |||
There was a problem hiding this comment.
The value is stored in the first 4 byte of val, get it using s_num will add 4 bytes of zero on the higher address in memory, this gives different result on big endian systems and little endian systems, this can be proved using the below code
#include <stdio.h>
#include <stddef.h> // Include this header for size_t
int main() {
printf("Size of size_t: %zu bytes\n", sizeof(size_t)); // 8
printf("Size of int: %zu bytes\n\n", sizeof(int)); // 4
int a[2] = {1, 0};
int *i_num = (int *)a;
printf("Value of i_num: %d\n\n", *i_num);
size_t *s_num = (size_t *)a;
printf("Value of s_num: %zu\n", *s_num);
printf("Cast s_num as int: %d\n", (int)(*s_num));
return 0;
}
The output of the code on x86(little endian system):
Size of size_t: 8 bytes
Size of int: 4 bytes
Value of i_num: 1
Value of s_num: 1
Cast s_num as int: 1
On s390s system(big endian):
Size of size_t: 8 bytes
Size of int: 4 bytes
Value of i_num: 1
Value of s_num: 4294967296
Cast s_num as int: 0
There was a problem hiding this comment.
The s_num 4294967296 in the output of big endian system equals to 1<<32
There was a problem hiding this comment.
Please revert this.
rightblank
commented
Jun 17, 2024
|
Hi, @cosmo0920 @edsiper, could you please help to review this PR? I added 2 code snippet to show the issues. It fixes two types configuration failure issues that only appear on big endian systems,
|
|
Could you ensure DCO, that is, adding |
Hi, @cosmo0920, could you please help to review again? I am from IBM and can assure DCO is complied, signed-off has been added to the commit. |
|
Hi, @cosmo0920, could you help to get this PR merged? |
cosmo0920
previously approved these changes
Jun 28, 2024
There was a problem hiding this comment.
I believe that this change should be nice. However, I didn't have BE byteorder systems. All of my having platforms are LE, i.e. x86_64 (Windows, Linux), aarch64 Linux on Chromebook and Windows on ARM64, and Linux on riscv64 . Before getting merged, should we confirm this PR effects on somewhere else of s390x systems on cloud?
Hi, @cosmo0920, the test log and the valgrind output in the PR description could help to double confirm this is done on s390x,
but if you really want to, you can use the LinuxONE Community Cloud. The usage is free for Open-Source Developers: |
|
@cosmo0920 This is a really important change for us, so we'd like to see it merged as soon as feasibly possible. Do you think the test log and /cc @edsiper @fujimotos @koleini @leonardo-albertovich @agup006 |
leonardo-albertovich
approved these changes
Aug 14, 2024
|
Hi, @edsiper @leonardo-albertovich @cosmo0920 @pwhelan,
And below is the test output after rebasing the code changes in above 5 PRs to master, currently there are 7 cases in failure. test output |
|
These are the preliminary results of the tests in s390 : I'll inspect each one of these while the arm tests run locally and follow up my comment with some insight on why these failed and if it's related to the PR or not. |
|
@leonardo-albertovich |
|
I don't think that's the lua issue because I configured the project using |
|
Plugins / tests and other files manually reviewed :
PRs opened to address the identified issues :
These PRs should be merged before this one or any of the related ones because they have no side effects. Please do not make any modifications to this or any of the related PRs. Thanks. |
|
base code has been updated. @rightblank please fix the conflict that has been generated so we can run CI and proceed to the merge. |
Done. @edsiper |
Signed-off-by: YingJie Fu <fyjbj@cn.ibm.com>
|
thanks everyone for helping to troubleshoot and fix this bug :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #8828
It fixes two types configuration failure issues that only appear on big endian systems,
rotate_waitintailplugin.read_from_headintailplugin andlowercaseinsystemdplugin.Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.