Skip to content

Conversation

@nvlsianpu
Copy link
Contributor

Whenever a new key-value is about to be stored, the settings
perform check whether the value really changes. This check
after #9521 patch should work differently as \0 is not
the value terminator anymore. Because of above any value which
starts from \0 will be treated mistakenly as a NULL.

This patch uses check-length callback instead read-callback which
fix the issue and simplify the code a little.

Signed-off-by: Andrzej Puzdrowski [email protected]

Copy link
Member

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

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

LGTM

@jhedberg
Copy link
Member

Gives me these build warnings:

/Users/jhedberg/src/zephyr/subsys/settings/src/settings_store.c: In function 'settings_dup_check_cb':
/Users/jhedberg/src/zephyr/subsys/settings/src/settings_store.c:114:6: warning: unused variable 'rc' [-Wunused-variable]
  int rc;
      ^~
/Users/jhedberg/src/zephyr/subsys/settings/src/settings_store.c:113:7: warning: unused variable 'temp' [-Wunused-variable]
  char temp;
       ^~~~
[217/228] Linking C executable zephyr/zephyr_prebuilt.elf

@jhedberg
Copy link
Member

@nvlsianpu this solves the issue I had with mesh_shell, thanks for fixing it! I'll wait for the compilation warnings to be fixed before approving, though.

Copy link
Member

@jhedberg jhedberg left a comment

Choose a reason for hiding this comment

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

Giving my approval so that this can be merged as soon as @nvlsianpu fixes the build warnings (which are now also causing CI to fail).

Whenever a new key-value is about to be stored, the settings
perform check whether the value really changes. This check
after zephyrproject-rtos#9521 patch should work differently as `\0` is not
the value terminator anymore. Because of above any value which
starts from \0 will be treated mistakenly as a NULL.

This patch uses check-length callback instead read-callback which
fix the issue and simplify the code a little.

Signed-off-by: Andrzej Puzdrowski <[email protected]>
@nvlsianpu nvlsianpu force-pushed the bugfix/setting-dup-check branch from 58116fa to aa37f35 Compare December 12, 2018 08:42
@jhedberg
Copy link
Member

@nvlsianpu any update on fixing the compilation warnings? It'd good to get this merged ASAP.

@codecov-io
Copy link

Codecov Report

Merging #12026 into master will decrease coverage by 0.09%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #12026     +/-   ##
=========================================
- Coverage   48.05%   47.96%   -0.1%     
=========================================
  Files         281      281             
  Lines       43412    43399     -13     
  Branches    10404    10403      -1     
=========================================
- Hits        20863    20815     -48     
- Misses      18400    18432     +32     
- Partials     4149     4152      +3
Impacted Files Coverage Δ
subsys/logging/log_output.c 59.81% <0%> (-9.59%) ⬇️
subsys/logging/log_core.c 69.86% <0%> (-4.81%) ⬇️
misc/printk.c 81.81% <0%> (-3.13%) ⬇️
include/logging/log_msg.h 89.09% <0%> (-1.82%) ⬇️
include/logging/log_core.h 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f704f91...aa37f35. Read the comment docs.

@nvlsianpu
Copy link
Contributor Author

@jhe green now

@carlescufi carlescufi merged commit 523acef into zephyrproject-rtos:master Dec 12, 2018
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.

4 participants