[#422] Improve the `restart_server` function

[T1t4m1un]

Functionality:

Implement restart_server function for daemon with conditional restarts and warnings for configuration changes, including TLS updates.

Document:

/**
 * Compares the TLS configurations of two servers to determine if they are identical.
 *
 * This function assesses whether the source and destination servers have matching TLS configurations, 
 * which is crucial for ensuring secure communication protocols are uniformly applied across server interactions.
 *
 * @param s1 Pointer to the first server structure to compare.
 * @param s2 Pointer to the second server structure to compare.
 * @return Returns true if both servers have identical TLS configurations, false if they differ.
 */
static bool is_same_tls(struct server* s1, struct server* s2);

/**
 * Checks if two boolean values are same. If not, prints a line in the log
 *
 * @param name The name of the configuration option.
 * @param e The existing boolean value.
 * @param n The new boolean value.
 * @return 1 if a restart is required, 0 otherwise.
 */
static int restart_bool(char* name, bool e, bool n);

Test Case Verification:

1. Follow the documentation to configure the pgagroal daemon and pgagroal-cli using the provided demo
2. Initiate the pgagroal
3. Update the pgagroal.conf file under the [primary] section to enable TLS by setting tls = on
4. Execute pgagroal-ctl conf reload to apply the new
5. Confirm the restart requirement in /tmp/pgagroal.log, which should log an entry similar to:

2024-03-25 16:55:04 INFO  configuration.c:2479 Restart required for Server <primary>, parameter <tls> - Existing false New true

[fluca1978]

@T1t4m1un the is_same_tls function has the right name, what Jeper was suggesting is to make the code more robust by reversing the condition checks: test explicitly if the tls settings are the same, then return true, otherwise return false. Your implementation at the moment does the opposite: checks if the tls are not the same, otherwise returns (by default) true.

With regard to strcmp, I know we are using strcmp in many places, but we should start using strncmp for safety reasons.

[T1t4m1un]

@T1t4m1un the is_same_tls function has the right name, what Jeper was suggesting is to make the code more robust by reversing the condition checks: test explicitly if the tls settings are the same, then return true, otherwise return false. Your implementation at the moment does the opposite: checks if the tls are not the same, otherwise returns (by default) true.

With regard to strcmp, I know we are using strcmp in many places, but we should start using strncmp for safety reasons.

Thank you very much for your patient reply! Please take a look my latest commit🥺

[fluca1978]

Please reverse the true and false value checks as Jesper asked, then add the note about the need for restart into the documentation and your name to the authors file.

[T1t4m1un]

Please reverse the true and false value checks as Jesper asked, then add the note about the need for restart into the documentation and your name to the authors file.

Thank you for your recognition of me!

I'm editing the documentation but have some specific features want to confirm with you.

In our current implementation, we only check the config in server section. We won't check whether the host name or TLS config is same as former one in the [pgagroal] section. Is this the feature we expected?

# provided config same as `getting started`
pgagroal-cli conf set tls on # success, we won't check the config not in server section
pgagroal-cli conf set server.primary.tls on # failed, we do check the config in server `primary` section

Thank you for your answer!

[jesperpedersen]

The TLS in the [pgagroal] is to secure pgagroal itself, so "server side".

The TLS in the server sections are to support connecting to secure PostgreSQL instances.

[T1t4m1un]

The TLS in the [pgagroal] is to secure pgagroal itself, so "server side".

The TLS in the server sections are to support connecting to secure PostgreSQL instances.

Thank you for your answer! I emphasized it in my documentation. Looking forward to your review~

[T1t4m1un]

Sorry for such careless mistake. I've fixed it and add some short comments.

I haven't change the return type of restart_bool, I'm looking forward for your opinion on it:

Because all the exsisting restart_XXX functions return int. It's for code style consistency.

[fluca1978]

Sorry for such careless mistake. I've fixed it and add some short comments.

No problem, but better test your code before pushing for review.

I haven't change the return type of restart_bool, I'm looking forward for your opinion on it:

The idea should be this: the is_same_xxx functions return a bool, the restart_xxx functions return and int.

Please, meld all the commits into a single one and force push (or push to another branch) so that we can get the last look at it.

[fluca1978]

@jesperpedersen PTAL seems fine to me

[jesperpedersen]

Rebased, and merged.

Thanks for your contribution !