Settings Module - settings_line_val_read() returning -EINVAL instead of 0 for deleted setting entries

[DeclanTraill]

When using the Settings module we found that when we delete a settings entry, upon reset and settings load, it was being reloaded with the previous value prior to the deleted entry.

Upon investigation I found that the reason for this was that the settings_line_val_read() function was returning -EINVAL when it read the deleted (NULL) entry, rather than a settings size of 0, so our code was not loading the NULL entry but retaining the previous version of the setting.

I have made a suggested change (see attached image for code before & after the fix) to that function and it seems to fix the problem here. Could you please see if this is a fix that should be made?

Regards,
Declan Traill

[aescolar]

CC @nvlsianpu

[nvlsianpu]

@DeclanTraill Thanks for the bug registration!

[Laczen]

@DeclanTraill, I agree with the patch submitted in #17446, but it might be easier to detect a delete by using the len that is provided in the h_set routine. This len should already be zero, so you can detect it early instead of doing it using the read function. It is a bit strange to do a read of a deleted item. Not all backends will give you this information, nvs for example will not call any h_set routine when a variable has been deleted.

[nvlsianpu]

@DeclanTraill What is the reason behind that you are using base64 encoding? I'm asking because was thinking about depreciate this as seams to be not needed and only over complicates the implementation.

[nwsetec]

Hi @nvlsianpu I'm working on the same project as Declan. We are using base64 encoding as we have products in the field running Zephyr 1.13 and are planning to update them to 1.14 (which I believe was using base64 only) and this was the easiest path we could see. This raises something I was wondering about, can the settings subsystem work over an nffs file without base64 decoding?

[DeclanTraill]

Hi Jehudi, But it is in the h_set routine where we were getting the length returned as -22 (-EINVAL) rather than the length of 0. This is what was causing the problem. I can’t see where the length is given to the h_set routine without calling settings_val_read_cb() We could call: size_t settings_line_val_get_len(off_t val_off, void *read_cb_ctx) to get the length, but this effectively does a read to determine the length anyhow. The parameters passed into h_set are a list of name strings and  void *value_ctx Are you saying that the length is already in this information somewhere?  If so, where? Regards, Declan From: Jehudi Maes <notifications@github.com> Sent: Wednesday, 10 July 2019 7:21 PM To: zephyrproject-rtos/zephyr <zephyr@noreply.github.com> Cc: DeclanTraill <declan.traill@setec.com.au>; Mention <mention@noreply.github.com> Subject: Re: [zephyrproject-rtos/zephyr] Settings Module - settings_line_val_read() returning -EINVAL instead of 0 for deleted setting entries (#17415) @DeclanTraill, I agree with the patch submitted in #17446, but it might be easier to detect a delete by using the len that is provided in the h_set routine. This len should already be zero, so you can detect it early instead of doing it using the read function. It is a bit strange to do a read of a deleted item. Not all backends will give you this information, nvs for example will not call any h_set routine when a variable has been deleted. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

…

-------- Email from setec.com.au does not necessarily represent the official policy of SETEC Pty Ltd. See http://www.setec.com.au/emaildisclaimer20060629.html for details.

[Laczen]

Hi Jehudi, But it is in the h_set routine where we were getting the length returned as -22 (-EINVAL) rather than the length of 0. This is what was causing the problem. I can’t see where the length is given to the h_set routine without calling settings_val_read_cb() We could call: size_t settings_line_val_get_len(off_t val_off, void *read_cb_ctx) to get the length, but this effectively does a read to determine the length anyhow. The parameters passed into h_set are a list of name strings and  void *value_ctx Are you saying that the length is already in this information somewhere?  If so, where? Regards, Declan

Hi Declan, in the h_set routine the second parameter that gets passed is the length of the data:
int (*h_set)(const char *key, size_t len, settings_read_cb read_cb, void *cb_arg);

[nwsetec]

@Laczen our project is currently using v1.14-branch LTS Zephyr so h_set callback doesn't have that len parameter. cc @DeclanTraill

[Laczen]

@nwsetec, I see, ok in that case there is no possibility to use len. There have been quite some changes in the settings module since 1.14 LTS,

[nvlsianpu]

Hi @nvlsianpu I'm working on the same project as Declan. We are using base64 encoding as we have products in the field running Zephyr 1.13 and are planning to update them to 1.14 (which I believe was using base64 only) and this was the easiest path we could see. This raises something I was wondering about, can the settings subsystem work over an nffs file without base64 decoding?

Yes, base64 encoding is not required anymore - I kept it just for backward compatibility. Be aware that settings record format for FS backend changes without base64 encoding:
<key>=<base64_encoded_data>/0 => <length><key>=<raw_data>

[nwsetec]

Hi @ioannisg @nvlsianpu,
Unless another fix has gone into the v1.14-branch then the #17446 fix for this bug should be merged for the 1.14.1 release. I don't believe Declan will have the time to provide the test request in PR comment.
Thanks

[jenmwms]

Hi @nwsetec @ioannisg @nvlsianpu
Can you please provide an update on this?
Thanks

[nwsetec]

Hi @jenmwms the #17446 PR fixes the issue, I've checked the master branch and the issue still exists, so PR needs to be reviewed and if all good merged.