pgaskin
commented
3 years ago @NiLuJe, can you see if you can reproduce the issue you reported (https://github.com/pgaskin/NickelMenu/pull/98#issuecomment-738240153) with this PR?
Closed pgaskin closed 3 years ago
pgaskin
commented
3 years ago @NiLuJe, can you see if you can reproduce the issue you reported (https://github.com/pgaskin/NickelMenu/pull/98#issuecomment-738240153) with this PR?
NiLuJe
commented
3 years ago Bogus extra options (e.g., the icons, which aren't supported in this branch) don't generate any warnings that I can see, so lemme try something sneakier...
NiLuJe
commented
3 years ago Seems to be a bit of buffer-chaining issue:
Added a bogus entry @ L 23
menu_item : foo : bar : baz
And, without touching the config file, just opening the NM menu over and over:
(c.f., the ... error one)
NiLuJe
commented
3 years ago Commenting out the offending entry doesn't break the chain (meaning I still get a single Config Error menu entry).
NiLuJe
commented
3 years ago To be clear, I'm updating the config file live, on device, via nano. (And closing it).
NiLuJe
commented
3 years ago (And breaking it earlier doesn't help either, but I can't really tell you what the log says, because by this point it's just a repetition of scan for config files: ^^).
Dec 4 01:28:21 nickel: (NickelMenu) checking for config updates (src/nickelmenu.cc:277)
Dec 4 01:28:21 nickel: (NickelMenu) global: scanning for config files (src/config.c:856)
Dec 4 01:28:21 nickel: (NickelMenu) config: skipping /mnt/onboard/.adds/nm/. because it's a special file (src/config.c:43)
Dec 4 01:28:21 nickel: (NickelMenu) config: skipping /mnt/onboard/.adds/nm/.. because it's a special file (src/config.c:43)
Dec 4 01:28:21 nickel: (NickelMenu) ... error: scan for config files: scan for config files: scan for config files: scan for config files: scan for config files: scan for config files: scan for config files: scan for config files: scan for config files: scan for config files: p
Dec 4 01:28:21 nickel: (NickelMenu) global: freeing old config and replacing with error item (src/config.c:861)
Dec 4 01:28:21 nickel: (NickelMenu) revision = 13 (src/nickelmenu.cc:279)
Dec 4 01:28:21 nickel: (NickelMenu) building menu (src/nickelmenu.cc:281)
Dec 4 01:28:21 nickel: (NickelMenu) adding item 'Config Error'... (src/nickelmenu.cc:306)
Dec 4 01:28:21 nickel: (NickelMenu) showing menu (src/nickelmenu.cc:355)Bogus extra options (e.g., the icons, which aren't supported in this branch) don't generate any warnings that I can see, so lemme try something sneakier...
That's by design.
Seems to be a bit of buffer-chaining issue:
That's odd. I'll look into it further tomorrow.
Edit: @NiLuJe, what does it say if you add NM_LOG("TEST: %s\n", nm_err_peek()); here?
Edit 1: If that shows the old error, does adding NM_ERR_SET(NULL); here fix it? If so, I think I know why this is happening.
NiLuJe
commented
3 years ago First patch:
Dec 4 03:19:50 nickel: (NickelMenu) checking for config updates (src/nickelmenu.cc:277)
Dec 4 03:19:50 nickel: (NickelMenu) global: scanning for config files (src/config.c:858)
Dec 4 03:19:50 nickel: (NickelMenu) TEST: (null) (src/config.c:105)
Dec 4 03:19:50 nickel: (NickelMenu) config: skipping /mnt/onboard/.adds/nm/. because it's a special file (src/config.c:43)
Dec 4 03:19:50 nickel: (NickelMenu) config: skipping /mnt/onboard/.adds/nm/.. because it's a special file (src/config.c:43)
Dec 4 03:19:50 nickel: (NickelMenu) global: changes detected (src/config.c:868)
Dec 4 03:19:50 nickel: (NickelMenu) global: parsing new config (src/config.c:871)
Dec 4 03:19:50 nickel: (NickelMenu) config: reading config file /mnt/onboard/.adds/nm/doc (src/config.c:251)
Dec 4 03:19:50 nickel: (NickelMenu) config: reading config file /mnt/onboard/.adds/nm/generators (src/config.c:251)
Dec 4 03:19:50 nickel: (NickelMenu) config: reading config file /mnt/onboard/.adds/nm/kfmon (src/config.c:251)
Dec 4 03:19:50 nickel: (NickelMenu) config: reading config file /mnt/onboard/.adds/nm/koreader (src/config.c:251)
Dec 4 03:19:50 nickel: (NickelMenu) config: reading config file /mnt/onboard/.adds/nm/niluje (src/config.c:251)
Dec 4 03:19:50 nickel: (NickelMenu) config: reading config file /mnt/onboard/.adds/nm/plato (src/config.c:251)
Dec 4 03:19:50 nickel: (NickelMenu) config: reading config file /mnt/onboard/.adds/nm/test (src/config.c:251)
Dec 4 03:19:50 nickel: (NickelMenu) ... error: file /mnt/onboard/.adds/nm/test: line 23: parse menu_item: field 2: unknown location 'baz' (src/config.c:383) (src/config.c:270) (src/config.c:875)
Dec 4 03:19:50 nickel: (NickelMenu) global: freeing old config and replacing with error item (src/config.c:876)
Dec 4 03:19:50 nickel: (NickelMenu) revision = 1 (src/nickelmenu.cc:279)
Dec 4 03:19:50 nickel: (NickelMenu) building menu (src/nickelmenu.cc:281)
Dec 4 03:19:50 nickel: (NickelMenu) adding item 'Config Error'... (src/nickelmenu.cc:306)
Dec 4 03:19:50 nickel: (NickelMenu) showing menu (src/nickelmenu.cc:355)
Dec 4 03:19:58 nickel: (NickelMenu) checking for config updates (src/nickelmenu.cc:277)
Dec 4 03:19:58 nickel: (NickelMenu) global: scanning for config files (src/config.c:858)
Dec 4 03:19:58 nickel: (NickelMenu) TEST: parse config files: file /mnt/onboard/.adds/nm/test: line 23: parse menu_item: field 2: unknown location 'baz' (src/config.c:383) (src/config.c:270) (src/config.c:879) (src/config.c:105)
Dec 4 03:19:58 nickel: (NickelMenu) config: skipping /mnt/onboard/.adds/nm/. because it's a special file (src/config.c:43)
Dec 4 03:19:58 nickel: (NickelMenu) config: skipping /mnt/onboard/.adds/nm/.. because it's a special file (src/config.c:43)
Dec 4 03:19:58 nickel: (NickelMenu) ... error: parse config files: file /mnt/onboard/.adds/nm/test: line 23: parse menu_item: field 2: unknown location 'baz' (src/config.c:383) (src/config.c:270) (src/config.c:879) (src/config.c:862)
Dec 4 03:19:58 nickel: (NickelMenu) global: freeing old config and replacing with error item (src/config.c:863)
Dec 4 03:19:58 nickel: (NickelMenu) revision = 2 (src/nickelmenu.cc:279)
Dec 4 03:19:58 nickel: (NickelMenu) building menu (src/nickelmenu.cc:281)
Dec 4 03:19:58 nickel: (NickelMenu) adding item 'Config Error'... (src/nickelmenu.cc:306)
Dec 4 03:19:59 nickel: (NickelMenu) showing menu (src/nickelmenu.cc:355)
Dec 4 03:20:05 nickel: (NickelMenu) checking for config updates (src/nickelmenu.cc:277)
Dec 4 03:20:05 nickel: (NickelMenu) global: scanning for config files (src/config.c:858)
Dec 4 03:20:05 nickel: (NickelMenu) TEST: scan for config files: parse config files: file /mnt/onboard/.adds/nm/test: line 23: parse menu_item: field 2: unknown location 'baz' (src/config.c:383) (src/config.c:270) (src/config.c:879) (src/config.c:866) (src/config.c:105)
Dec 4 03:20:05 nickel: (NickelMenu) config: skipping /mnt/onboard/.adds/nm/. because it's a special file (src/config.c:43)
Dec 4 03:20:05 nickel: (NickelMenu) config: skipping /mnt/onboard/.adds/nm/.. because it's a special file (src/config.c:43)
Dec 4 03:20:05 nickel: (NickelMenu) ... error: scan for config files: parse config files: file /mnt/onboard/.adds/nm/test: line 23: parse menu_item: field 2: unknown location 'baz' (src/config.c:383) (src/config.c:270) (src/config.c:879) (src/config.c:866) (src/config.c:862)
Dec 4 03:20:05 nickel: (NickelMenu) global: freeing old config and replacing with error item (src/config.c:863)
Dec 4 03:20:05 nickel: (NickelMenu) revision = 3 (src/nickelmenu.cc:279)
Dec 4 03:20:05 nickel: (NickelMenu) building menu (src/nickelmenu.cc:281)
Dec 4 03:20:05 nickel: (NickelMenu) adding item 'Config Error'... (src/nickelmenu.cc:306)
Dec 4 03:20:06 nickel: (NickelMenu) showing menu (src/nickelmenu.cc:355)Second one doesn't build?
In file included from src/config.c:16:0:
src/config.c: In function 'nm_config_files':
src/util.h:46:21: error: expected ')' before string constant
nm_err_set((fmt " (%s:%d)"), ##__VA_ARGS__, __FILE__, __LINE__);
^
src/config.c:99:5: note: in expansion of macro 'NM_ERR_SET'
NM_ERR_SET(NULL);
^
src/util.h:46:21: error: too many arguments for format [-Werror=format-extra-args]
nm_err_set((fmt " (%s:%d)"), ##__VA_ARGS__, __FILE__, __LINE__);
^
src/config.c:99:5: note: in expansion of macro 'NM_ERR_SET'
NM_ERR_SET(NULL);
^
cc1: all warnings being treated as errorsnm_err_set(NULL); works, though:
Message is up-to-date, and breaking the config differently or fixing it works :)
pgaskin
commented
3 years ago Second one doesn't build?
nm_err_set(NULL);works, though:
Looks like I mixed up the function and macro when typing the reply from my phone ;) .
Message is up-to-date, and breaking the config differently or fixing it works :)
That's good. So it is what I thought it was, which was a regression during the error handling and config parsing refactors where the error handling wasn't updated for the new behaviour. nm_config_files should have cleared the previous error on success (alternatively, callers of nm_global_config_update could have done it, but that's breaking abstraction).
I'm going to make sure everything else is correct and add this fix.
I guess the reason why this bug didn't show up for most people is because it only manifests itself if actions (including the error message) haven't been run in-between, since nm_menu_item_do clears errors.
P.S. You can use <details> and </details> with an optional <summary> and </summary> inside to collapse your logs. Put them on new lines with blank lines above and below each tag so it doesn't mess up the markdown parsing within.
NiLuJe
commented
3 years ago Yeah, I realized I'd never actually ran the actual "Config error" entry. I kinda never do since I usually have the log scrolling right in my face ;p.
But, in case it matters: the actual content of the popup is also sane after the fix ;).
pgaskin
commented
3 years ago Everything appears to be correct now. I'm going to merge this.
Thanks for finding this regression @NiLuJe!
This fixes a regression in ee0eb7ccf735ad26593cb5a4804b746e273469b4 (#47) which caused vague error messages to be returned instead of specific error messages from
nm_config_parse__line_*. These functions should have returnedtrueon error, but were returningNULL(i.e.false) instead, causingnm_config_parseto treat it as an unknown line type instead of returning the specific issue with the line. This regression does not cause any issues other than vague error messages being returned for certain errors during config parsing.In addition, this fixes another regression discovered by @NiLuJe where configuration errors would persist in rare situations due to
nm_config_filesnot clearing existing errors as expected. This issue does not happen if the config error menu item is pressed to see the error message sincenm_menu_item_doclears errors.fixes #99