Conversation
| g_free(options->certificate_file); | ||
| g_free(options->certificate_chain_file); | ||
| g_free(options->root_directory); | ||
| inf_ip_address_free(options->listen_address); |
There was a problem hiding this comment.
This would need a NULL check.
There was a problem hiding this comment.
According to malloc(3) free(NULL) does nothing, therefore the check is not strictly required as far as I understand.
There was a problem hiding this comment.
But this is not free, it is inf_ip_address_free, which does not like a NULL argument. Or at least it's not documented to. It uses g_slice_free under the hood, which apparently handles NULL fine, but in general the *_free functions in libinfinity don't.
There was a problem hiding this comment.
I was told that it is usual for functions that free memory to handle NULL themselves, therefore it might be a good idea to just do the same for the libinfinity free functions if they need it.
There was a problem hiding this comment.
Understood, but it's more important to stay consistent within this PR. If I'm not mistaken some glib free functions such as g_object_unref don't handle NULL arguments either.
|
Thanks for the patch, this looks very solid to me! The only thing that's missing is support for config reloading. Infinoted reloads the config file when it receives SIGHUP, so that config changes can be applied without restarting the server. The code for that is in Another note: Some systems don't listen automatically on IPv4 if a IPv6 socket is created, which is why the existing code was trying to create both sockets (it's a setting somewhere in /sys or /proc, I forgot, and (some versions of?) Windows do it, too). This "automatically-also-listen-on-IPv4" functionality would now be lost in case that setting is turned off and one specifies an IPv6 address in the options. That could be fixed by allowing to specify a list of IP addresses to listen on (or just an IPv4 and IPv6 one separately), but I'd happily merge the PR without that. |
|
@1uks 1uks told me that the configuration reload does not work for him right now. Would it be ok for you to just change the reload code to not close and re-open the sockets or maybe remove the feature completely? I am not sure about its use case. If the server connection is closed/re-opened anyhow, then the service could be restarted complety instead. I the typical use case I know is when using old-style log files with logrotating and making services write to the new log file. But I am not sure that this applies here - and it would not require re-opening the sockets. |
|
It is only the listening socket(s) that are re-opened; already existing connections are not terminated. So there is some value in that feature as opposed to just restarting the service. I agree it has always been a bit fragile. I'd expect this to be fairly simple. Will try to have a quick look tonight myself, and if it doesn't work out quickly let's go without for now. |
|
I pushed this to master and just added a simple check for the config reloading to show an error message if one tries to change the listen address. |
|
Thank you for merging it so fast! Do you have any plans for a new release soon? Could you maybe spin a release when PR #16 is merged? |
|
I had some other things in mind for a 0.7 release, but I don't think I'll get to those anytime soon. So yes, will see if I can do that this weekend. |
|
Thank you |
|
I'm sorry, didn't get to it this weekend. Will do it soonish, though. |
4 participants
These changes allow users to specify the IP address to listen on.