This is ready for testing.

In particular, I'd like someone to try and break the update mechanism (try and make it incorrectly detect an update or lack thereof, also ensure the items are in the correct order, make sure it still works correctly after a USBMS session), I want to make sure I didn't leave any edge cases (no menu items, etc) unhandled, and I would like another pair of eyes on the pointer/memory handling in nm_global_* in init.c (to make sure I free things properly and swap things out under the right conditions).

In addition, you shouldn't be able to cause it to crash at any point (e.g. I tested that it wouldn't crash if the config files were modified while a menu was open and an item was pressed, and it worked fine because we only check for config updates when the menu is opened).

Also, @NiLuJe, can you ensure that your KFMon generator works with KFMon running, with KFMon changes (you'll need to touch a config file for now, but I'd be happy to hear ideas to do it automatically), without KFMon running, and with KFMon unresponsive?

P.S. A code review would also be appreciated.

I do have one idea for automatic updates for KFMon: In another PR, I could implement generator updates and the KFMon one could check the mtime of the socket (which KFMon can touch every time there is a change).

I'm trying to figure my way through the code, but I have to admit, first impressions are... convoluted. Mainly around the nm_global_config_update and nm_global_config_items

More thoughts when I can (hopefully) figure most things out.

The code in config.c seems fine to me. What perplexes me is the global config management. To me, it seems a mess of global functions operating on a mess of global variables.

I would have thought a better way would be to have a single global struct that contains all the required state, with the required updating functions updating it directly, preferably via a pointer parameter (to make it clear what's being modified).

The menu ejecting code could then just check needed flags, and any error messages etc.

What am I missing?

Just had another thought. You might need to go back through the config parser code and check for memory leaks, now that config parsing is no longer a one-time affair.

(I'm specifically thinking about returning on error without freeing the calloc'ed nm_menu_item_t variable.)

I won't have time to give this a proper look-see until this week-end, but it's on top of the list ;).

What perplexes me is the global config management. To me, it seems a mess of global functions operating on a mess of global variables.

Yes, it is somewhat convoluted. I ended up on the current design after thinking of how I can keep pointers valid for as long as possible, free stuff as soon as possible, and be the most efficient.

The first thing I did was to split the file scanning from the config. The other option I was considering was to add a file/date field to each config item (or just the first one from each file), but I realized that would limit future flexibility, it isn't that clean, and nothing uses it (or should use it) except during the initial parsing. This led to nm_config_files which returns a nm_config_file_t, which, for efficiency (we append quite a bit and don't know the size ahead of time, but we loop in order), was a linked list.

Then, I was thinking about how to update it in a way which would allow returning errors while leaving the original one untouched, and also handle freeing the memory. This led to nm_config_files_update, which is relatively straightforward (note that the bit after the loop which checks op || np is to check if the sizes are different, as one would still point to an item after the loop exits while the other is null at the end of the list).

Afterwards, I had to figure out the best way to manage the config and errors with the least code duplication, specifically around the error items (I wanted to create those in one place during the parsing rather than in the menu for each error). This led to the 4 variables: nm_global_menu_config_files (for comparing against without affecting the config), nm_global_menu_config (so we can free it later), nm_global_menu_config_items (because we needed somewhere to store the pre-generated error items, which meant we couldn't just call nm_config_get_menu in the injection hook), and nm_global_menu_config_n (same reason). And, to be on the safe side, I made those private to init.c so other files can't touch it by accident, and added nm_global_config_items to get the items, and nm_global_config_update to update them.

For nm_global_config_update, I wanted to reduce the code duplication in error handling, and wanted to have the simplest conditions possible for freeing (i.e. avoid lots of nested if statements for each condition). This led to nm_global_config_replace, which just frees whatever will be replaced if it isn't NULL, and then replaces the items with either the error message or the new menu items. This also has the advantage of ensuring that there won't be any condition in which the menu items are invalid after the update function returns.

That's basically how I ended up with the current design.

I'll have a look at the config parsing itself and see if I can simplify the memory management in it (probably by making the struct on the stack first, then only allocating them memory on success rather than beforehand).

I've basically rewritten the config parser over the last 3 commits. It should be faster, more stable, more efficient, easier to extend, and won't leak memory anywhere (even when errors occur). The only downside is that it is twice the lines of code.

I've already done some tests with it, but while testing the config reloading, also try out different config errors to make sure they are still handled properly.

If you have any suggestions for improving the config parser further, I'd be happy to hear them.

And that's it for tonight, I think ;).

(I've also implemented and tested the mtime-on-kfmon-socket thing on KFMon's side, and that works beautifully :). Also made the config order alphabetical there, too).

Also, random easy refactor idea: move the config filtering to the actual scandir filter feature? (That essentially means a smaller alloc for nl).

(And get rid of the logging on . & .. in the process ;)).

generated items were inserted in reverse order (because the list is actually linked in reverse?).

Yes, thanks for catching that. The list used to be linked in reverse until I refactored the config parser. Then, that made the menu item adapter go in the wrong order, which, as you noticed, I fixed. But, I tested the generators before that fix, making me forget about the generator code.

I'll probably apply your first fix, as I feel it's a bit more concise than the second.

Apart from that, no adverse effects with the KFMon generator.
...

👍

Also made the config order alphabetical there, too

I noticed. Is there a particular case which is broken with alphasort that made you write an adapter for strcmp instead?

Also, random easy refactor idea: move the config filtering to the actual scandir filter feature? (That essentially means a smaller alloc for nl).

I can't remember why I decided against it, but upon looking at it again, I don't see why not.

Did you happen to test the config errors yet?

Did you happen to test the config errors yet?

Hmm, which config errors? ^^

EDIT: The Clang SA stuff? If so, yep, the fixes did the trick, and I think I agree with you on the false-positives left ;).

I noticed. Is there a particular case which is broken with alphasort that made you write an adapter for strcmp instead?

alphasort takes pointers to a dirent pointer, I'm using the BSD fts_* API instead of scandir (for, err, some reason? :D), so the function needs to take pointers to an FTSENT pointer instead ;).

Hmm, which config errors? ^^

The usual ones (just to make sure I didn't mess them up during the refactor). I've already tested some of them.

The usual ones (just to make sure I didn't mess them up during the refactor). I've already tested some of them.

Besides the action/generator fields no longer being validated, I haven't noticed anything new.

Bogus args are handled on a per-action basis, IIRC, so that shouldn't have been affected.
I tested the empty config case, that worked.

Not quite sure what else could go wrong ;).

I've tested the error handling with:

chain_success:dbg_syslog:bad
bad
bad:bad
menu_item:bad
menu_item:main:bad
menu_item:main:test:bad
menu_item:main:test:dbg_syslog:ok
chain_bad:bad
chain_success:bad
chain_success:dbg_syslog:ok
generator:bad
generator:main:kfmon
chain_success:dbg_syslog:bad
menu_item:main:test:dbg_syslog:ok
chain_failure:dbg_syslog:ok
chain_success:dbg_syslog:ok
chain_always:dbg_syslog:ok

I deleted the first line each time I tested it.

I think this is pretty much ready now. The parser emits errors properly, and I think I've fixed all regressions from the refactor. I've also tested all the examples and my own config. I'll probably merge this, and I might post a beta version on MR later this week for testing.

Another random thought: at boot, config parsing is bracketed between the failsafe mechanism.

That's no longer the case when updating configs. Should we care?

(I say that because I did manage to busy-loop nickel when breaking the linked list stuff ^^).

That's no longer the case when updating configs. Should we care?

I originally thought of that (hence the now-reverted instant failsafe destruction commit), but I decided against it, as it would mean I need a mutex to ensure the failsafe isn't already running, and if it was going to crash, it would do it during the initial parsing, which would trigger the failsafe. And, if it did happen to cause issues only after the first parse (i.e. when the menu is opened), NM can be uninstalled manually by connecting via USB or telnet without using the menu. Does this seem reasonable?

@geek1011: Nope, sounds good.

In any case, if an uncaught bug does cause an issue during a config update, Nickel will softlock, which means sickel will reboot the device. Since the device was essentially unusable, the configs won't have been touched, at which point they'll trigger the exact same issue during the newt boot, except this time it'll be during the failsafe window, so, everybody wins ;).

It essentially boils down to simply needing two reboots instead of one to "fix" it by self-destruct.

EDIT: Which is what you just said :D. It's late ^^.