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

Tidies up the uci_wrt.c file #235

Merged
merged 12 commits into from
Sep 2, 2022
Merged

Tidies up the uci_wrt.c file #235

merged 12 commits into from
Sep 2, 2022

Conversation

aloisklink
Copy link
Contributor

@aloisklink aloisklink commented Aug 12, 2022

Draft until #234 is merged.

Tidies up the uci_wrt.c file.

I've added two new functions:

  • uwrt_set_properties()
  • uwrt_add_list_properties()

They look like the following:

/**
 * @brief Set multiple OpenWRT UCI properties at once
 * @param[in] ctx UCI context. The context ptr will be modified.
 * @param[in] properties Array of properties to set. Warning, strings may be
 * modified by UCI.
 * @retval  0 Success
 * @retval -1 Error
 */
int uwrt_set_properties(struct uci_context *ctx, UT_array *properties) {
  for (char *const *prop = (char **)utarray_front(properties); prop != NULL;
       prop = (char **)utarray_next(properties, prop)) {

    if (uwrt_set_property(ctx, *prop) < 0) {
      log_error("uwrt_set_property fail for %s", *prop);
      return -1;
    }
  }
  return 0;
}

Essentially, they just call uwrt_set_property/uwrt_add_list_property multiple times.

This means:

  • Less code reuse!
  • Code Coverage is happy since there are less branches.
  • Smaller code.
  • Bad news is that it's slightly slower.
    (I don't think the compiler is smart enough to optimise utarray loops)

I've also quickly added a simple test for uci_wrt, just to confirm that my changes don't cause any errors.

Removes a bunch of duplicated error handling branches
by adding the new functions `uwrt_set_properties()`
and `uwrt_set_list_properties()`,
which call `uwrt_set_[list_]property()` in a loop.
Moves it next to `uwrt_delete_property()` and adds some
doxygen doc string.
Simplify uci_wrt.c code by using `uwrt_delete_properties()`
snprintf is a bit more secure, as it will help prevent buffer
overflows.
The function `uwrt_delete_properties()` always returns 0,
as it ignores all errors.

Because of this, we should make it return `void`, since checking
the return value is pointless.
Basically just calls all the public firewall functions
and checks if they return 0.
@codecov
Copy link

codecov bot commented Aug 12, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@dd377a7). Click here to learn what that means.
The diff coverage is 94.52%.

❗ Current head 53ff135 differs from pull request most recent head c41922d. Consider uploading reports for the commit c41922d to get more accurate results

@@           Coverage Diff           @@
##             main     #235   +/-   ##
=======================================
  Coverage        ?   47.90%           
=======================================
  Files           ?      110           
  Lines           ?    16206           
  Branches        ?        0           
=======================================
  Hits            ?     7764           
  Misses          ?     8442           
  Partials        ?        0           
Impacted Files Coverage Δ
src/utils/uci_wrt.c 75.12% <94.40%> (ø)
tests/utils/test_uci_wrt.c 100.00% <100.00%> (ø)

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

Copy link
Contributor

@mereacre mereacre left a comment

Choose a reason for hiding this comment

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

Is it still in the draft mode?

@aloisklink aloisklink changed the base branch from main to build/add-clang-support September 2, 2022 12:37
@aloisklink aloisklink changed the base branch from build/add-clang-support to main September 2, 2022 12:37
@aloisklink aloisklink marked this pull request as ready for review September 2, 2022 12:38
@aloisklink
Copy link
Contributor Author

aloisklink commented Sep 2, 2022

Is it still in the draft mode?

Now that #234 is merged, I'll open it up as ready!

@aloisklink aloisklink merged commit f445f47 into main Sep 2, 2022
@aloisklink aloisklink deleted the refactor-uci-wrt branch September 2, 2022 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants