Add error handling for ini_parse and handler functions of inih library

[panosfol]

This is wrong. The way this works will produce errors.

We should check if fields are populated after parsing our configuration, ensuring the process for database replication works etc.

There are some points i'd like to discuss before continuing with this PR. First the way the Documentation suggests we use the handler function is that we return 0 if no match was found, like so:

if (CHECK_SECTION("postgres")) {
        if(MATCH("postgres", "enabled")) {
                if (strcmp(value, "true") == 0)
                        ini_config->postgres_config->enabled = true;
                else
                        ini_config->postgres_config->enabled = false;
                } else {
                        return 0;
                }
        }

However there is a problem with this, because our if statements are seperated (I assume it was done in favor of clarity) In order for this to work we either have to:

• Make one if statement and check in the end if no field from the .ini file was matched, and return 0
• Or go to the final ifstatement and place there the else return 0 clause, with the risk of being less clear on what exactly its purpose is.

Second, Postgres has some default values that uses if none are provided (port = 5432 for example). Some users might want to leave empty the fields to take advantage of that. If we check if our fields are populated, and not proceed if there are not, then we remove the default mechanic of Postgres. Should I proceed anyway?

[charmitro]

This is wrong. The way this works will produce errors.

We should check if fields are populated after parsing our configuration, ensuring the process for database replication works etc.

There are some points i'd like to discuss before continuing with this PR. First the way the Documentation suggests we use the handler function is that we return 0 if no match was found, like so:

if (CHECK_SECTION("postgres")) { if(MATCH("postgres", "enabled")) { if (strcmp(value, "true") == 0) ini_config->postgres_config->enabled = true; else ini_config->postgres_config->enabled = false; } else { return 0; } }

The function is called multiple times because it is iterating. I'm not sure if it works the way you think. You can test it and find out.

If what you're saying is not the case, we should just proceed with checking afterwards if we populated everything and so on.

However there is a problem with this, because our if statements are seperated (I assume it was done in favor of clarity) In order for this to work we either have to:

• Make one if statement and check in the end if no field from the .ini file was matched, and return 0
• Or go to the final if statement and place there the else return 0 clause, with the risk of being less clear on what exactly its purpose is.

Second, Postgres has some default values that uses if none are provided (port = 5432 for example). Some users might want to leave empty the fields to take advantage of that. If we check if our fields are populated, and not proceed if there are not, then we remove the default mechanic of Postgres. Should I proceed anyway?

If other are populated, port should automatically be "5432", assuming the user let it be the default. Search Postgres for other fields that have a default value and do the same.

[panosfol]

This is wrong. The way this works will produce errors. We should check if fields are populated after parsing our configuration, ensuring the process for database replication works etc. There are some points i'd like to discuss before continuing with this PR. First the way the Documentation suggests we use the handler function is that we return 0 if no match was found, like so: if (CHECK_SECTION("postgres")) { if(MATCH("postgres", "enabled")) { if (strcmp(value, "true") == 0) ini_config->postgres_config->enabled = true; else ini_config->postgres_config->enabled = false; } else { return 0; } } The function is called multiple times because it is iterating. I'm not sure if it works the way you think. You can test it and find out. If what you're saying is not the case, we should just proceed with checking afterwards if we populated everything and so on.

Yes I understand it is iterating thats why it needs one if statement. The way we have it right now, in the second iteration, it returns parsing error, because it didn't match the enabled field, and therefore enters the else return 0 section of the first `if statement.

However there is a problem with this, because our if statements are seperated (I assume it was done in favor of clarity) In order for this to work we either have to: Make one if statement and check in the end if no field from the .ini file was matched, and return 0 Or go to the final if statement and place there the else return 0 clause, with the risk of being less clear on what exactly its purpose is. Second, Postgres has some default values that uses if none are provided (port = 5432 for example). Some users might want to leave empty the fields to take advantage of that. If we check if our fields are populated, and not proceed if there are not, then we remove the default mechanic of Postgres. Should I proceed anyway? If other are populated, port should automatically be "5432", assuming the user let it be the default. Search Postgres for other fields that have a default value and do the same.

So if I understand this correctly, we should stop the process and produce an error only when the fields that do NOT have a default value from postgres are not populated, right?

[charmitro]

Yes I understand it is iterating thats why it needs one if statement. The way we have it right now, in the second iteration, it returns parsing error, because it didn't match the enabled field, and therefore enters the else return 0 section of the first `if statement.

As I said, try and see. I'll review your logic around it when it is ready-for-review.

So if I understand this correctly, we should stop the process and produce an error only when the fields that do NOT have a default value from postgres are not populated, right?

We assume port has the default value provided by Postgres. As it is not enough for the replication procedure, we exit if the required variables(all of them I think(?)) are missing.

[panosfol]

Yes I understand it is iterating thats why it needs one if statement. The way we have it right now, in the second iteration, it returns parsing error, because it didn't match the enabled field, and therefore enters the else return 0 section of the first `if statement.

As I said, try and see. I'll review your logic around it when it is ready-for-review.

So if I understand this correctly, we should stop the process and produce an error only when the fields that do NOT have a default value from postgres are not populated, right?

We assume port has the default value provided by Postgres. As it is not enough for the replication procedure, we exit if the required variables(all of them I think(?)) are missing.

Postgres provides a default variable for each one we use in our config, even if 0 variables are provided it can still work in some cases. Should we mimic that behavour too and let the user proceed if no variables are given?

[charmitro]

Postgres provides a default variable for each one we use in our config, even if 0 variables are provided it can still work in some cases. Should we mimic that behavour too and let the user proceed if no variables are given?

Only if postgres.enabled is true.

[charmitro]

@panosfol Don't forget to update your git config on the machine you're committing from because we get a

Signed-off-by: Panagiotis Foliadis <pfoliadis@Debian-bookworm-latest-amd64-base>

You can do this with:

git config --global user.email "john@doe.com"
git config --global user.name "John Doe"