Refactor config.c

[pseregiet]

Refactor of config.c

• fixed 2 memory leaks. Strings for pre/post commands weren't freed

• config file is read line by line instead of loading full file to a buffer. Lets us use standard C functions for getting a line instead of spliting manually on \n

• once a config file is parsed, search for more files is stoped. Config is simple enough where "overlays" seem pointless and might only confuse the user why his config is overwriten by some files he forgot about

• Program is now able to load files from ~/.config/. fopen will not be able to open a path that uses the "~~" symbol. $HOME has to be read

• strings with many paths separated by : are tokenized using more standard way

• all static ("private") functions are at the bottom of the file. That's just cosmetic and up to you how you like it.

Also a small change to the output.c / output.h

• config structure is no longer copied, only a pointer is stored. The structure was never modified by output.c functions, so I'm not sure why it was done so in the first place.

[matanui159]

fixed 2 memory leaks. Strings for pre/post commands weren't freed

This is actually a good catch. It should probably destroy all strings either by finding options using configString, or adding a destructor per option.

config file is read line by line instead of loading full file to a buffer. Lets us use standard C functions for getting a line instead of spliting manually on \n

I see you are using fgets with a 4KB buffer. What happens if the line is shorter than that? The function to parse the line only handles a single line.

once a config file is parsed, search for more files is stoped. Config is simple enough where "overlays" seem pointless and might only confuse the user why his config is overwriten by some files he forgot about

This feature should stay. Especially since now there is a global config file with defaults. I also have a user-config with my overrides and also use the relative file for debugging overrides.

Program is now able to load files from ~/.config/. fopen will not be able to open a path that uses the "~~" symbol. $HOME has to be read

This was already working and tested (see util/path.c). This rewrite doesn't handle the case where the XDG_CONFIG_ variables contain ~.

strings with many paths separated by : are tokenized using more standard way

strtok is very dangerous, not multithread safe and you can't start another strtok iteration within another.

config structure is no longer copied, only a pointer is stored. The structure was never modified by output.c functions, so I'm not sure why it was done so in the first place.

I did this because the config is stack allocated and thus I could not be certain if the pointer would change or not. A bit pedantic but better safe than sorry.

[pseregiet]

fgets copies till it finds new line or the maximum number is reached. The buffer will not overflow, but the line will be capped at 4kb. I think it's acceptable. Any path cannot be longer than 4096 bytes anyway (filesystem limit), and that's the only thing that could potentially be that long. I guess the command too, but whoever wants to write a big script should just write it to a .sh file and call bash with it.

As for reading many config files. This comes down to preference and of course you have the final decision. Changing it to read all the files is as easy as removing the early return in the function. However, considering all the values have a default already in the binary a global config seems pointless. Any program I ever used only uses one config and it tries to read the user paths first, ending with /etc/ only if it didn't find anything else.

I've missed that you already replace ~ with $HOME, I'll rework the code to use that function.

strtok might not be thread safe, but the code does not use more than one thread to parse the config, so this is fine. However, if that's not acceptable by you, strtok_r can be used instead, which is thread safe.

And finally the thread data. Generally speaking you are right, passing pointer to stack allocated object is a bad idea, but since it was created in main it's guaranteed to stay. Otherwise the Destroy call at the end would crash as well. Anyway, config structure could also be global. It is set only once at the start of the program and everything else is just reading it. I think that's justifiable and safe use of global even in multithreaded application.

[pseregiet]

I've pushed a new change

• strtok_r is used. it's thread safe

• ~/ is fixed up in all paths

• all config files are read, not just one.

[matanui159]

Sorry I didn't comment earlier but I don't understand what the point of this PR is? Now that the features are now the same what benefit is there? Style-wise I prefer the original (although that does come down to personal taste) and there was nothing wrong with it.

[pseregiet]

Heh, true. Originally I just thought the reading of many config files was weird as well as the way the paths are constructed, which prompted me to refactor. But it doesn't matter. Just add these 2 rsMemoryDestroy and it's gonna be ok :)