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

refactor: fix const char * warnings #239

Merged
merged 3 commits into from
Sep 5, 2022
Merged

Conversation

aloisklink
Copy link
Contributor

Converts some char * arrays into const char * arrays to fix compiler invalid const qualifier discardation errors.

E.g. converts: char * example[] = {...}; to const char *

This also may fix a thread-safety bug in ipaddr_modify().
get_prefix() modifies the input string temporarily.
However, even though the function changes the string back the original value, since edgesec uses multithreading, this may have caused an error.

@aloisklink aloisklink added the refactor Refactoring code label Aug 23, 2022
@codecov
Copy link

codecov bot commented Aug 23, 2022

Codecov Report

Merging #239 (7995a0e) into main (eadb275) will increase coverage by 0.64%.
The diff coverage is 84.05%.

❗ Current head 7995a0e differs from pull request most recent head 57ea668. Consider uploading reports for the commit 57ea668 to get more accurate results

@@            Coverage Diff             @@
##             main     #239      +/-   ##
==========================================
+ Coverage   47.89%   48.53%   +0.64%     
==========================================
  Files         110      113       +3     
  Lines       17828    16264    -1564     
==========================================
- Hits         8539     7894     -645     
+ Misses       9289     8370     -919     
Impacted Files Coverage Δ
src/utils/nl.c 10.30% <50.00%> (+8.99%) ⬆️
tests/utils/test_nl.c 92.00% <92.00%> (ø)
src/utils/net.c 75.72% <100.00%> (+8.18%) ⬆️
tests/utils/test_wrap_log_error.c 100.00% <100.00%> (ø)
tests/utils/wrap_log_error.c 100.00% <100.00%> (ø)
src/utils/allocs.h 75.00% <0.00%> (-5.00%) ⬇️
tests/radius/ip_addr.c 75.00% <0.00%> (-5.00%) ⬇️
src/dhcp/dhcp_service.c 64.28% <0.00%> (-4.47%) ⬇️
src/dns/mdns_list.c 71.15% <0.00%> (-3.85%) ⬇️
src/dns/command_mapper.c 70.58% <0.00%> (-3.10%) ⬇️
... and 92 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Converts some `char *` arrays into `const char *` arrays to fix
compiler invalid const qualifier discardation errors.

E.g. converts: `char * example[] = {...};` to `const char *`

This also may fix a thread-safety bug in `ipaddr_modify()`.
`get_prefix()` modifies the input string temporarily.
However, even though the function changes the string back the original
value, since edgesec uses multithreading, this may have caused an
error.
return 0;
}

static int teardown(void **state) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you test if the interface is actually removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need :)

I've only added a basic test that checks whether nl_set_interface_ip() fails if the interface doesn't exist, so there's actually no interface.

I couldn't figure out how to add a test that checks whether it actually works (I'm guessing we'd need to do a bunch of mocks).

expect_function_call(log_error);
expect_string(__wrap_log_levels, error_message,
"nl_set_interface_ip error: ipaddr_modify failed with -1");
assert_int_equal(nl_set_interface_ip(ptr->context, test_nl_interface_name,
Copy link
Contributor

Choose a reason for hiding this comment

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

To complete the test we need to check if the right IP was assigned by using the function nl_get_interfaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test currently only checks for failure if the interface doesn't exist, so the right IP is never assigned.

I'm not 100% sure how to actually test if this function works (since tests need to run on CI too, and ideally without sudo access), but I guess it will take a lot of mocks.

Currently just tests that `nl_set_interface_ip()`
throws an error that says:

ipaddr_modify error: could not find interface 'test_nl_interface_name'

for an interface that doesn't exist.
@aloisklink aloisklink merged commit 86b5e8c into main Sep 5, 2022
@aloisklink aloisklink deleted the refactor/fix-const-warning branch September 5, 2022 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactoring code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants